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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent ChildView from retaining an otherwise dropped view #1749

Merged
merged 5 commits into from
Oct 13, 2022

Conversation

as-cii
Copy link
Member

@as-cii as-cii commented Oct 13, 2022

Fixes #1748

Previously, ChildView used to hold a strong handle to the underlying view it wanted to render. That was because we wanted to prevent rendering a view that was already dropped. Retaining the view, however, has the downside of delaying the dropping of descendant views until a scene has been invalidated and re-rendered. In particular, the following could happen:

  • View X could contain and render a view Y that takes focus
  • View X gets dropped, note that view Y isn't dropped as part of this because the rendered element for X hasn't been dropped yet and it contains a ChildView pointing to Y
  • The presenter is invalidated and the scene gets re-rendered, causing Y to get dropped
  • Note that Y is still the focused element at this point, despite it not being rendered anywhere and its parent being gone.
  • If we dispatched a keystroke, we would try to traverse Y's hierarchy and panic 馃挜

There are several solutions that I considered and discarded when exploring the space of this problem:

  1. After invalidating the scene, check if there were more dropped entities before returning from MutableAppContext::flush_effects. What I don't love about this is that it might cause us to invalidate the presenter multiple times if e.g. X contains child view Y, and Y contains child view Z, etc.. I suppose it's still linear but something about the reentrancy nature of this approach feels uncanny.
  2. When traversing Y's hierarchy, simply don't panic if a parent view doesn't exist. I discarded this because it feels bad to leave some fundamental state in a dirty state, as it might violate some invariant elsewhere in the code.

Ultimately, I concluded that it is a programmer error to try to render a child view without holding it with a strong handle in the parent view. And, in general, holding strong references to views in elements seems like an anti-pattern. Therefore, instead of holding a strong handle in ChildView, we will store a weak view handle and avoid laying out/painting/etc. that view if it has been dropped already, logging an error to the console. This avoids the reentrancy of solution 1) and the incorrectness of solution 2) above.

It comes at the cost of potentially passing something that will be dropped before the ChildView has a chance to render, but it seems like those cases should be pretty rare and we will catch them with the newly-added log statement.

@as-cii as-cii merged commit f5db02a into main Oct 13, 2022
@as-cii as-cii deleted the child-view-panic branch October 13, 2022 14:45
SomeoneToIgnore added a commit that referenced this pull request Jul 14, 2023
Closes https://github.com/zed-industries/community/issues/75
Closes https://github.com/zed-industries/community/issues/1749

The PR 

* changes keybindings for `Editor && mode == auto_height` context:
before, `alt-enter` and `alt-shift-enter` added new lines in such
editors, including the one from buffer search.

New bindings are the same as in `Editor && mode == full` context.

* adds `search::SelectAllMatches` action and binds it to `Alt + Enter`
by default, to select all matches of a buffer search

The behavior mimics VSCode: we do not move the screen even if all
selections are out of the visible range (Cmd+G will navigate there) and
allow reselecting the results from both pane and search field, as long
as the search is not dismissed.

Release Notes:

- Added `search::SelectAllMatches` (`Alt + Enter` default) action to
place carets and select all buffer search results
([#75](https://github.com/zed-industries/community/issues/75),
[#1749](https://github.com/zed-industries/community/issues/1749)).
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.

Panic when holding cmd-shift-c
1 participant