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

WidgetId revision, part 2 #265

Merged
merged 15 commits into from
Jan 6, 2022
Merged

WidgetId revision, part 2 #265

merged 15 commits into from
Jan 6, 2022

Conversation

dhardy
Copy link
Collaborator

@dhardy dhardy commented Jan 3, 2022

Part 2 following #264.

There was a significant question here: what format widget identifiers should take and what properties are required. This was already considered in #264, but to complete part 2 the design document was revised. Notable excerpts follow.

We must choose whether event handling happens via (selective) broadcast or targeted search. Previously targeted search was chosen for performance scaling, but in retrospect this was a poor goal. Detailed review found a couple of subtle issues:

  • With a broadcast approach, it is less trivial to test whether an event was unused because its target didn't handle the event or because no target was found. The difference is important for "event stealing", e.g. being able to click-and-drag on a label to scroll.
  • Adapting SendEvent::send to use selective-broadcast increases complexity slightly, but isn't a big deal.
  • If targeted events are sent via broadcast, this could easily be adapted to send actual broadcast events — though currently we don't need this, and perceived uses such as animations should be restricted to visible widgets, so this is probably not useful. Shared-data updates could perhaps use this instead of "update handles", but these probably should also be restricted to visible widgets (no need to update an invisible radio-box — until it becomes visible).
  • To make Tab navigation work correctly from a hidden item in a view list (which represents only visible items), we need to know the full path of the current navigation target (since the view list cannot iterate over all invisible items looking for a matching hash). This implies either WidgetId should always represent the full path, or we need a separate "identifier" system for Tab-navigation.
  • Using a "path over indices" makes the identifier robust over most modifications, excluding insertion/deletion of children in a list from positions other than the end. To get around this, we can allow ListView and similar widgets to not use sequential identifiers and recommend that the plain List not be used for insertion/deletion from the middle, though this is not ideal.

With this in mind, three options for WidgetId are apparent:

  1. Represent the path, allocating when necessary, but never freeing (memory leaks)
  2. Represent the path, allocating when necessary, and using a reference count (thus WidgetId is not Copy)
  3. Represent a hash, optionally with partial path information, and use selective-broadcast for event sending

It is possible that new WidgetIds would be generated frequently (e.g. scrolling a large ListView), thus option (1) is not a good idea. While having WidgetId: Copy is (very) nice, clones remain cheap with option (2), and implementation turned out to be less intrusive than feared.

Note: due to reference counting, WidgetId is now !Send and !Sync. This could be fixed with atomic reference counting, but seems unnecessary: the UI is usually restricted to a single thread (other threads can still wake subscribed widgets via update handles).

Unsolved from #264 :

  • "PartialEq for WidgetId panics on invalid id": this revealed a subtle bug where input event handling could happen before newly-created widgets are configured. While this had no serious side-effects, it is unintended and could perhaps cause annoying-to-debug issues. Thus, panic on invalid id remains.
  • "Paths are never deallocated": fixed
  • "Using WidgetId::opt_from_u64 with a bad value could cause a panic": this method is now marked unsafe (since it could now cause a seg-fault). It could be removed entirely (no remaining usage), but is left for now.

@dhardy dhardy merged commit 95dacec into master Jan 6, 2022
@dhardy dhardy mentioned this pull request Feb 1, 2022
5 tasks
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.

1 participant