chore: avoid using cell width to calculate target index for taskbar #1212
Conversation
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
避免使用固定的任务栏图标宽度来计算当前拖拽的目标项的索引. 这个修改旨在下面两点: 1. 为后续任务栏图标不固定长度做准备(图标+标题) 2. 为后续ItemModel解耦做准备,便于重构分支合入(重构分支使用DelegateModel 的 items 属性维护的顺序表示任务栏上的图标顺序,因而无法直接使用此处 原本的下标计算方式) (下一步是规避使用 ItemModel::moveTo() 接口) Log:
Reviewer's GuideThis PR replaces hardcoded cell-width index calculations in TaskManager with a ListView.indexAt-based helper, refactors drag & drop handlers to use the new method and moveTo calls, updates visualModel.items.move to rely on DelegateModel indices, and exposes indexAt in OverflowContainer to prepare for variable icon widths and model decoupling. Sequence diagram for drag-and-drop index calculation and moveTo updatesequenceDiagram
participant User as actor User
participant TaskManager
participant OverflowContainer
participant Applet
participant DataModel
User->>TaskManager: Drag or drop event (onPositionChanged/onDropped)
TaskManager->>OverflowContainer: findTargetIndexByPosition(Qt.point(x, y))
OverflowContainer->>OverflowContainer: indexAt(x, y)
OverflowContainer-->>TaskManager: targetIndex
TaskManager->>Applet: desktopIdToAppId(launcherDndDesktopId)
Applet-->>TaskManager: appId
TaskManager->>DataModel: moveTo(appId, targetIndex)
Class diagram for TaskManager and OverflowContainer drag-and-drop refactorclassDiagram
class TaskManager {
+findTargetIndexByPosition(dragPosition)
}
class OverflowContainer {
+indexAt(x, y)
}
class Applet {
+dataModel
+desktopIdToAppId(launcherDndDesktopId)
}
class DataModel {
+moveTo(appId, targetIndex)
}
TaskManager --> OverflowContainer : uses indexAt
TaskManager --> Applet : uses desktopIdToAppId, dataModel
Applet --> DataModel : has dataModel
TaskManager ..> DataModel : calls moveTo
Class diagram for visualModel.items.move refactor to DelegateModel indicesclassDiagram
class visualModel {
+items
}
class DelegateModel {
+itemsIndex
}
visualModel ..> DelegateModel : uses itemsIndex for move
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @BLumia - I've reviewed your changes - here's some feedback:
- In findTargetIndexByPosition, ensure you handle both horizontal and vertical modes correctly (e.g., swapping x/y for column layouts) so indexAt returns the expected index.
- Consider clamping the returned targetIndex within [0, model.count] as a safeguard against invalid indices from indexAt.
- The onEntered handler still references a TODO and unused code—cleanup or resolve this section to avoid dead code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In findTargetIndexByPosition, ensure you handle both horizontal and vertical modes correctly (e.g., swapping x/y for column layouts) so indexAt returns the expected index.
- Consider clamping the returned targetIndex within [0, model.count] as a safeguard against invalid indices from indexAt.
- The onEntered handler still references a TODO and unused code—cleanup or resolve this section to avoid dead code.
## Individual Comments
### Comment 1
<location> `panels/dock/taskmanager/package/TaskManager.qml:123` </location>
<code_context>
onEntered: function(drag) {
- visualModel.items.move((drag.source as AppItem).visualIndex, app.visualIndex)
+ // TODO: this is actually unused, should change the delegateRoot type from DropArea to Item later.
+ visualModel.items.move(drag.source.DelegateModel.itemsIndex, delegateRoot.DelegateModel.itemsIndex)
}
</code_context>
<issue_to_address>
Remove or address TODO comments in production code.
Consider removing the unused code or opening a tracking issue instead of leaving a TODO.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// TODO: this is actually unused, should change the delegateRoot type from DropArea to Item later.
visualModel.items.move(drag.source.DelegateModel.itemsIndex, delegateRoot.DelegateModel.itemsIndex)
=======
visualModel.items.move(drag.source.DelegateModel.itemsIndex, delegateRoot.DelegateModel.itemsIndex)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+123
to
+124
| // TODO: this is actually unused, should change the delegateRoot type from DropArea to Item later. | ||
| visualModel.items.move(drag.source.DelegateModel.itemsIndex, delegateRoot.DelegateModel.itemsIndex) |
There was a problem hiding this comment.
suggestion: Remove or address TODO comments in production code.
Consider removing the unused code or opening a tracking issue instead of leaving a TODO.
Suggested change
| // TODO: this is actually unused, should change the delegateRoot type from DropArea to Item later. | |
| visualModel.items.move(drag.source.DelegateModel.itemsIndex, delegateRoot.DelegateModel.itemsIndex) | |
| visualModel.items.move(drag.source.DelegateModel.itemsIndex, delegateRoot.DelegateModel.itemsIndex) |
18202781743
approved these changes
Aug 4, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
避免使用固定的任务栏图标宽度来计算当前拖拽的目标项的索引.
这个修改旨在下面两点:
(下一步是规避使用 ItemModel::moveTo() 接口)
Summary by Sourcery
Remove reliance on fixed cell widths for taskbar item reordering by using ListView.indexAt and DelegateModel indices
Enhancements: