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

Route id #1978

Merged
merged 9 commits into from
Sep 25, 2021
Merged

Route id #1978

merged 9 commits into from
Sep 25, 2021

Conversation

xarvic
Copy link
Collaborator

@xarvic xarvic commented Sep 18, 2021

Adds Notification::route

Christoph added 3 commits September 18, 2021 20:14
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
Signed-off-by: Christoph <[email protected]>
@xarvic xarvic linked an issue Sep 18, 2021 that may be closed by this pull request
@maan2003
Copy link
Collaborator

maan2003 commented Sep 21, 2021

so route only has id of the immediate child, not the full route. can you explain the usecase and how this will help? Also why not use Vec<WidgetId> for the widgets it traveled

@xarvic xarvic requested a review from cmyr September 21, 2021 15:18
druid/src/command.rs Outdated Show resolved Hide resolved
Co-authored-by: Manmeet Maan <[email protected]>
@xarvic
Copy link
Collaborator Author

xarvic commented Sep 22, 2021

so route only has id of the immediate child, not the full route. can you explain the usecase and how this will help? Also why not use Vec for the widgets it traveled

The immediate child ID is useful, since it is easily accessible from the current widget.
For an example, if we want to implement the correct scroll_to behavior for enum_switcher, we need to know whether the notification came from within the active widget, if not we should discard it.
In the linked issue are more examples.

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.

Thanks! makes sense.

It is nice to update the notifications test but not a blocker.

Christoph added 3 commits September 25, 2021 16:30
Signed-off-by: Christoph <[email protected]>
fix
Signed-off-by: Christoph <[email protected]>
fix
Signed-off-by: Christoph <[email protected]>
@xarvic xarvic merged commit 0223076 into linebender:master Sep 25, 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.

add WidgetID of subtree for Notification
2 participants