Fix scroll-to-0 regression in vertical panel - #770
Conversation
In openvanilla#755 we generalized the math in vertical panel scrolling and synced the code with OpenVanilla, but due to how McBopomofo sets the panel candidate differently, the PR caused a regression in that the vertical panel failed to scroll back to page 0 if a previous candidate selection session moved to a non-zero page. Postmortem. The generalized math has a code path that does nothing. However, we didn't account for the fact that McBopomofo repeatedly resets the candidate panel's delegate. Resetting delegate reloads the data, but when the delegate is set to nil, there are no actual rows, and the table view's selected row is reset to 0 while the scroll view is still set to where the non-zero page started (this is consistent with Cocoa's behavior). But because the selected row is now reset, when the new candidate data is loaded, the candidate panel thinks that no further action is needed (because it's already set to 0), and so no scrolling is needed. In the previous code, -scrollRowToVisible was always called in this case. We therefore need to make sure that -scrollRowToVisible is always called when the selected candidate index is set to 0. OpenVanilla is not affected due to how it interacts with the panel's delegate logic, but syncing this change is still desirable.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a regression in the vertical candidate panel where it failed to scroll to the top. The fix involves ensuring that a scroll to row 0 is always triggered when the selected index is set to 0. The analysis in the pull request description is thorough, and the code change is a direct and correct implementation of the proposed solution. The change is minimal and well-contained, effectively resolving the bug without introducing apparent side effects.
There was a problem hiding this comment.
Pull request overview
This PR fixes a scroll-to-zero regression introduced in PR #755 where the vertical candidate panel failed to reset its scroll position to page 0 when the delegate was reset after navigating to a non-zero page.
Key Changes
- Added an explicit check for
newRowIndex == 0to ensurescrollRowToVisibleis always called when scrolling to the first row - Updated the comment to reflect that scrolling is guaranteed for row 0, not just when never selected or moving by one
In #755 we generalized the math in vertical panel scrolling and synced the code with OpenVanilla, but due to how McBopomofo sets the panel candidate differently, the PR caused a regression in that the vertical panel failed to scroll back to page 0 if a previous candidate selection session moved to a non-zero page.
Postmortem
The generalized math has a code path that does nothing. However, we didn't account for the fact that McBopomofo repeatedly resets the candidate panel's delegate. Resetting delegate reloads the data, but when the delegate is set to
nil, there are no actual rows, and the table view's selected row is reset to 0 while the scroll view is still set to where the non-zero page started (this is consistent with Cocoa's behavior). But because the selected row is now reset, when the new candidate data is loaded, the candidate panel thinks that no further action is needed (because it's already set to 0), and so no scrolling is needed. In the previous code,-scrollRowToVisiblewas always called in this case. We therefore need to make sure that-scrollRowToVisibleis always called when the selected candidate index is set to 0.OpenVanilla is not affected due to how it interacts with the panel's delegate logic, but syncing this change is still desirable.