Revert "opt: auto reorder channels, close #822 (#1005)"#1055
Revert "opt: auto reorder channels, close #822 (#1005)"#1055looplj merged 1 commit intorelease/v0.9.xfrom
Conversation
Summary of ChangesHello, 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 serves to revert a previous optimization that aimed to automatically reorder channels. By undoing the changes from commit 0594f3a, it restores the channel ordering functionality to its state before the automatic reordering was implemented. This ensures that channel weights are managed through a more explicit, relative calculation rather than a broad redistribution. Highlights
Changelog
Activity
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
|
![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 reverts a previous optimization for channel reordering. The redistributeWeights function, which re-calculated all channel weights on every move, is removed. It is replaced by a calculateRelativeWeight function that calculates a new weight for a moved item based on its immediate neighbors. While this is more performant, the implementation of calculateRelativeWeight has a flaw where it can cause weight collisions when items are moved, which may lead to a degradation of the ordering logic over time.
| const calculateRelativeWeight = (prev?: number, next?: number) => { | ||
| if (prev == null && next == null) { | ||
| return clampWeight(1); | ||
| } | ||
| if (prev == null) { | ||
| return clampWeight((next ?? 0) + 1); | ||
| } | ||
| if (next == null) { | ||
| return clampWeight(prev - 1); | ||
| } | ||
| if (prev === next) { | ||
| return clampWeight(prev); | ||
| } | ||
| return clampWeight(Math.floor((prev + next) / 2)); | ||
| }; |
There was a problem hiding this comment.
The logic in calculateRelativeWeight can lead to weight collisions in several scenarios, which may degrade the ordering functionality over time as items clump together with the same weight.
- Middle items: When an item is moved between two items with consecutive integer weights (e.g., 10 and 9),
Math.floor((10 + 9) / 2)results in9, causing a collision with the lower-weighted item. - Top item: When moving an item to the top of the list, if the current top item already has
MAX_WEIGHT(100), the new item will also be assigned a weight of 100. - Bottom item: Similarly, when moving an item to the bottom, if the current last item has
MIN_WEIGHT(0), the new item will also get a weight of 0.
While this implementation might be an intentional trade-off (e.g., for performance, after reverting a full redistribution strategy), it's worth considering if these collisions are acceptable. Using floating-point weights would be a robust way to solve this, if feasible within the design.
1 participant
This reverts commit 0594f3a.