Add WidgetPath and use it to fix widget focus.

[xStrom]

Background

I started looking into the widget id system after @sjoshid ran into some trouble with checking for descendants and talked about it on zulip.

The current way is to check a bloom filter, which gives false positives. Now that on its own isn't necessarily a problem, but that fact is easily forgotten, as indeed evidenced in druid code itself.

The has_focus methods for example use the bloom filter and thus can return true even if the widget nor its descendants actually have focus. This is not documented for has_focus.

This leads into a chain reaction, because now we get to TextBox which uses has_focus thinking it's safe and proceeds to paint its border and caret even when it's not actually focused. This issue could be solved perhaps with better documentation for has_focus and fixing internal widgets like TextBox to properly use is_focused instead.

However the issue runs deeper because the druid inner event passing system uses has_focus to determine whether to pass down key press events. This means that widgets that aren't actually focused are getting key press events. Widgets like TextBox have no is_focused guards before handling key press events, so this means that if there's a bloom filter collision, multiple textboxes will handle the key press inputs at once even though only one of them is focused. In theory this issue could also be solved by better documentation informing people that widgets can arbitrarily receive key press events.

That's a lot of documentation and random behavior though. Behavior that won't manifest under normal testing, but will manifest later for some customers using the final application and experiencing unexpected behavior. Behavior that the developer can't reproduce.

I think druid should have reliable and reproducible behavior whenever possible. It leads to easier debugging and fewer surprises. Thus I think that has_focus should always be correct, with no false positives.

The changes in this PR

• has_focus no longer returns false positives and can be relied on.
• Renamed the bloom filter's contains to may_contain so that its behavior is signaled even to people who don't have the documentation at hand, e.g. people glancing at changes on GitHub.
• Improved the bloom filter documentation to really drive home the false positive nature of it. Also added comments to locations where it is used.
• Focus cycling now works even when starting from a non-registered-for-focus widget that merely called request_focus. The cycle is still only between registered-for-focus widgets.
• Improved WidgetId documentation by adding info about how there's not necessarily a one-id to one-widget relationship.

The reliability of has_focus is achieved by introducing a new struct WidgetPath. It's basically a Vec<WidgetId> that describes the path that needs to be taken in the widget tree to reach the target. Its implementation uses a tiny bit of unsafe to guarantee fast code (even in debug builds!) by being less general than a full Vec thanks to WidgetId.

I know the reason a bloom filter is used for tracking children in BaseState is because the exponential nature of BaseState would cause memory exhaustion. Thus this PR doesn't replace the bloom filter completely. Instead it is right now used only for focus purposes. Although it can become useful for other future cases too.

Potential improvements

There is the question of the focus cycle (focus_chain) though. Right now focus_chain lives in BaseState and I just did a naive update there by replacing WidgetId with WidgetPath. This isn't too bad, but could be possibly better. I just don't know all the goals of the system.

Do widgets need to be able to change their position in the tree without re-registering for focus? Because if not, then this PR could perhaps be updated in a way that focus_chain would revert back to only knowing the leaf WidgetId, but there would be an additional payload generating the WidgetPath as a result of register_for_focus and then that WidgetPath would be stored in the window state as a HashMap. When a focus_chain widget needs to get focus, that goes through the window anyway, so the window could just do a lookup for the WidgetPath there. Then there would be no need to always store partial WidgetPaths in BaseState.

[luleyleo]

I got curious about the need of unsafe for performance.
Why not use Vec::extend_from_slice which seems to be pretty much what you have here with a ptr::write instead of copy_from_slice
Source: https://doc.rust-lang.org/stable/src/alloc/vec.rs.html#2138

[xStrom]

I didn't look too deeply into extend_from_slice because its docs state that this function will likely be deprecated.

The general reason I went with the unsafe code is because I wanted to avoid iterators. Iterator code can be optimized into memcpy in some cases in the release build, but won't probably happen in debug builds. There have also been cases with even the release optimizations being broken. Although in 2020 there are now a bunch of LLVM regression tests done by the Rust team, so it's a bit more reliable.

The extend special path for Copy slices seems worth looking more into though.

Overall my feeling here is that safe Rust code would be superior to unsafe usage, however not at the cost of performance, because this is a very uncomplex struct and the unsafe here isn't that unsafe.

[xStrom]

This PR has been superseded by other PRs.