refactor(gui): replace GridRevealState with visibility-aware reveal#110
Merged
Conversation
Drop GridRevealState, force_reveal_selected_record, and suppress_grid_auto_reveal in favor of an idempotent reveal_selected_record that consults the new masonry::board_selected_card_is_visible helper. Mirrors the skip-if-already-visible semantics ListState already provides for list mode, so background re-renders no longer yank the viewport away from where the user is looking. Refs #109
gpui's ScrollHandle::scroll_to_item already implements 'skip if already visible' via ScrollStrategy::FirstVisible (see gpui-0.2.2 div.rs scroll_to_active_item). The board_selected_card_is_visible helper added in the previous commit was a less precise reimplementation built on estimated card heights, so it could disagree with the framework at the pixel level. Inline the grid arm of reveal_selected_record() to a direct scroll_to_item call and remove the helper, MasonryColumnSpec, the pure masonry_selected_card_fully_visible function, and the four supporting unit tests. Documents the discovery in SELECTION_REVEAL_ANALYSIS.md as a postscript so the reasoning is recoverable. Refs #109
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes
GridRevealStateand theforce_reveal_*/suppress_grid_auto_revealplumbing in favor of a single, idempotentreveal_selected_record()that mirrors the "skip if already visible" semantics list mode already gets fromListState::scroll_to_reveal_item. Eliminates a cross-event mutation (every wheel tick used to write to board state) and shrinksRopyBoard's state surface.Linked Issue
Closes #109
Changes
masonry::board_selected_card_is_visible()plus a pure helpermasonry_selected_card_fully_visible(..)driven by aMasonryColumnSpec, with unit tests covering visible / scrolled-away / scrolled-to-match / out-of-range cases.RopyBoard::reveal_selected_record()consult that helper in grid mode and only callgrid_scroll_handle.scroll_to_itemwhen the selection is actually off-screen.GridRevealState,UiState::grid_reveal,UiState::grid_auto_reveal_enabled(),force_reveal_selected_record(), andsuppress_grid_auto_reveal(). All seven previousforce_reveal_*call sites now invokereveal_selected_record()directly.on_scroll_wheelhandler on the masonry scroll container — wheel scrolling no longer mutates board state.docs/SELECTION_REVEAL_ANALYSIS.md, the first-principles analysis that motivates this refactor.Testing
scripts/precheck.shpasses locally (clippy, 428 unit tests, machete, i18n / icons / themes).test_selected_card_visible_when_inside_viewport,test_selected_card_not_visible_when_user_scrolled_away,test_selected_card_visible_after_user_scrolled_to_match_selection,test_selected_card_visibility_handles_empty_or_out_of_range).Self-Check
gpui-component(no new UI components)thiserror(no new error types)