Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make egui-toast "threading-friendly" (adding new toast does not require Toasts instance) #12

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

PingPongun
Copy link

@PingPongun PingPongun commented Sep 20, 2023

  • replaced vector Toasts.added_toasts with global crossbeam_channel TOASTS_CHANNEL
  • adding new toast does not require Toasts instance. Can be done directly from Toast (this allows pushing new toasts from any place in code including other threads and async functions)
  • ! Toasts::add() is marked depreciated (internaly it's now toast.push() )
  • ! Toasts is marked #[must_use] to enforce using .show() function
  • added some convenience functions to Toast
  • added ToastTrait to use convenience functions with user defined options
  • version bumped to 0.9.0

Now there are 3 ways to add new toast (4 if counting depreciated Toasts::add()):

//1.
Toast::warning("Hello, World!"); // OR Toast::error, Toast::info, Toast::success
 
//2.
Toast::create( 
    "Hello, World!",
    ToastKind::Warning,
    ToastOptions::default()
        .duration_in_seconds(5.0)
        .show_progress(true)
);

//3.
Toast {
    text: "Hello, World!".into(),
    kind: ToastKind::Warning,
    options: ToastOptions::default()
        .duration_in_seconds(5.0)
        .show_progress(true)
}.push();

@urholaukkarinen
Copy link
Owner

I like the idea in general, but I would like to keep Toast fairly simple and have all logic for showing and configuring toasts in Toasts; Toast { ... }.push() feels a bit unintuitive to me. I would prefer keeping Toasts::add(toast) or Toasts::enqueue(toast). It can an associated function, now that Toasts instance is not required for adding toasts.

Removing the Toasts instance requirement does make it so that you cannot have multiple toast lists, but I don't know if that is a real use case. If that is needed, I guess it could be accomplished with some "toast queue id" to identify which toasts should go where.

@PingPongun
Copy link
Author

  1. I didn't want to change Toasts::add signature to not break compatibility (maybe even marking it depreciated was too far going action), adding Toasts::enqueue(toast) would be ok here (, but I still personally don't like it, as it still requires creating Toast instance and that's too much writing for my taste, as in my opinion, in most situations it's not necessary to set options per toast).
  2. I have to agree about Toast { ... }.push() (and I guess Toast::create(...) is even more unintuitive, so maybe both of them should be merged and made private & removed from examples), but that's exactly why I added wrapper functions like Toast::info("text") or Toast::custom(ID, "text"), intended to be used by end-user instead (similarly to log::info!(), ...).
  3. As Toast::info("text") doesn't take ToastOptions, it uses some predefined options. User can use ToastTrait where it's needed to have custom options, or even create few toast categories e.g. Toast::info("2 seconds"); ToastFast::info("1s"); ToastLong::info("5s");, when info toast with different times is needed.
  4. That kind of configuration that is available from ToastTrait was already in Toast (from ToastOptions). And the only new function(skipping wrappers) in Toast is push() which is a one-line fn that pushes toast to queue, the rest of logic is still in Toasts.
  5. Mentioned wrappers (Toast::info("text")) are in my opinion the most convenient option. If you prefer, they can be moved from Toast to Toasts(, but configurability in form of ToastTrait would be harder to fit there).
  6. "cannot have multiple toast lists" : I am fully aware of that, but I also was not able to find use case example here.

@SupernaviX
Copy link

Sorry to revive an old PR, I just wanted to mention that I have a use case for multiple toast lists. My app has multiple windows, and I want to be able to send different toasts to different windows. I can meet my needs with the existing library tho, just using channels to send toasts to the right place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants