Generalize the math in vertical panel scrolling - #755
Conversation
Summary of ChangesHello @lukhnos, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a limitation in the vertical scrolling mechanism of the candidate UI, as identified in issue #119. The changes generalize the underlying mathematical logic for scrolling, ensuring that the UI behaves correctly and consistently across various types of candidate index movements, not just single-step or single-page navigations. This enhancement improves the robustness and user experience of candidate selection. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
This fixes openvanilla#119 This is synced from openvanilla/McBopomofo#755 and a detailed analysis of the changes can be found there.
![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 refactors the vertical panel scrolling logic to be more generalized. The new implementation is clearer and appears to correctly handle different scrolling scenarios, such as single-line moves and page jumps. However, I've found a critical issue where a division by zero can occur if keyLabels is empty, leading to a crash. I've provided a code suggestion to fix this.
There was a problem hiding this comment.
Pull request overview
This PR refactors the vertical candidate panel scrolling logic to generalize the math beyond the original single-step and page-at-a-time movements. The change addresses an issue found in OpenVanilla where candidate index jumps by arbitrary amounts need to be handled correctly.
Key changes:
- Replaced conditional logic based on movement distance with page-index-based calculations
- Simplified scrolling by determining which page contains the target index and scrolling to show that page appropriately
The changes introduced in openvanilla#453 were designed for McBopomofo's use case where the candidate index is only moved by one or by a page. The issue found at openvanilla/openvanilla#119 showed that the math needed to be generalized. How tested: manually running through the scenarios mentioned in openvanilla#453 to make sure that the UI invariants are not changed.
7c1c420 to
6a52d99
Compare
This fixes openvanilla#119 This is synced from openvanilla/McBopomofo#755 and a detailed analysis of the changes can be found there.
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.
The changes introduced in #453 were designed for McBopomofo's use case where the candidate index is only moved by one or by a page. The issue found at openvanilla/openvanilla#119 showed that the math needed to be generalized.
How tested: manually running through the scenarios mentioned in #453 to make sure that the UI invariants are not changed.