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

Notification fixups #1640

Merged
merged 2 commits into from
Mar 16, 2021
Merged

Notification fixups #1640

merged 2 commits into from
Mar 16, 2021

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Mar 10, 2021

Two small fixes:

  • notifications submitted while handling a Notification are now allowed
  • notifications are no longer sent to the widget that originated them

cc @maan2003 does this fix that issue for you?

@cmyr cmyr added the S-needs-review waits for review label Mar 10, 2021
Copy link
Collaborator

@maan2003 maan2003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes the issue. But I have doubt about order of notifications.

Iets say a widget has notifications in order A B C . The widget doesn't handle A or C. When handling B, widget submits a notification B'.
The order for the parent will be A C B'.
But A B' C makes more sense to me.

druid/src/core.rs Outdated Show resolved Hide resolved
@cmyr
Copy link
Member Author

cmyr commented Mar 16, 2021

hmm, thinking through the orderings: your idea makes sense if we knew that newly added notifications were always replacements for other notifications, but I'm not sure we do? If a widget submits two new notifications in response to one old one, should they both be inserted at the position of the old one?

my gut feeling is that we shouldn't be relying on notification order; if two notifications cause interfering effects they should really be combined into a single notification?

I think I've talked myself into the implementation that I did here; it feels just slightly simpler. 🤷

@maan2003
Copy link
Collaborator

my gut feeling is that we shouldn't be relying on notification order; if two notifications cause interfering effects they should really be combined into a single notification?

Yes I feel the same :)

I think I've talked myself into the implementation that I did here; it feels just slightly simpler.

To me A B' C implementation feels simpler: just use parent_notifications inside inner_ctx. And it saves allocation 😉

I don't think the limitation against this made any sense.
@cmyr
Copy link
Member Author

cmyr commented Mar 16, 2021

my gut feeling is that we shouldn't be relying on notification order; if two notifications cause interfering effects they should really be combined into a single notification?

Yes I feel the same :)

I think I've talked myself into the implementation that I did here; it feels just slightly simpler.

To me A B' C implementation feels simpler: just use parent_notifications inside inner_ctx. And it saves allocation 😉

oh right, totally, that's much better. Thanks!

@cmyr cmyr merged commit 31c3974 into master Mar 16, 2021
@cmyr cmyr deleted the notification-fixups branch March 16, 2021 20:43
@maan2003 maan2003 removed the S-needs-review waits for review label May 3, 2021
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.

2 participants