refactor: unify app split/merge display and optimize text calculation #1373
Conversation
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @18202781743, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
There was a problem hiding this comment.
Pull request overview
This PR refactors the task manager's application display system by unifying split and merged window modes into a single component architecture. The main achievement is moving complex text width calculations from QML to a new C++ TextCalculator class for better performance and maintainability.
Key Changes:
- Introduced
TextCalculatorC++ class to handle dynamic text width calculations with caching - Unified
AppItemWithTitle.qmlintoAppItem.qmlto handle both split and merged modes - Created reusable
AppItemTitle.qmlcomponent for text display with proper elision
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 29 comments.
Show a summary per file
| File | Description |
|---|---|
| panels/dock/taskmanager/textcalculator.h | New C++ header defining TextCalculator and TextCalculatorAttached classes for text width computation |
| panels/dock/taskmanager/textcalculator.cpp | Implementation of text calculation logic with caching and optimal width determination |
| panels/dock/taskmanager/taskmanager.cpp | Registration of new TextCalculator QML types |
| panels/dock/taskmanager/package/TaskManager.qml | Removed complex QML text calculation functions, integrated TextCalculator component |
| panels/dock/taskmanager/package/AppItemWithTitle.qml | File removed - functionality merged into AppItem.qml |
| panels/dock/taskmanager/package/AppItemTitle.qml | New reusable component for displaying application titles with elision |
| panels/dock/taskmanager/package/AppItem.qml | Enhanced to support both modes, integrated title display, fixed hover handling with HoverHandler |
| panels/dock/taskmanager/CMakeLists.txt | Added new textcalculator source files to build |
| panels/dock/package/main.qml | Exposed dockPartSpacing property for spacing calculations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 非 windowSplit 模式，隐藏整个应用 | ||
| return false | ||
| } | ||
| property bool visibility: itemId !== taskmanager.Applet.desktopIdToAppId(launcherDndDropArea.launcherDndDesktopId) |
There was a problem hiding this comment.
The visibility logic for handling drag and drop has been significantly simplified. The old implementation had special handling for windowSplit mode that would show/hide individual windows vs. the docked icon. The new simplified logic only checks if 'itemId !== taskmanager.Applet.desktopIdToAppId(launcherDndDropArea.launcherDndDesktopId)'. This removes the windowSplit-specific behavior where individual windows could be dragged separately from the docked icon. Verify that this simplification doesn't break window split mode drag and drop functionality.
| property bool visibility: itemId !== taskmanager.Applet.desktopIdToAppId(launcherDndDropArea.launcherDndDesktopId) | |
| property bool visibility: { | |
| // In window split mode, each delegate may represent a single window. | |
| // Hide only the dragged window delegate itself. | |
| if (taskmanager.Applet.windowSplit) { | |
| return itemId !== launcherDndDropArea.launcherDndDesktopId | |
| } | |
| // In normal mode, delegates represent apps; hide items for the dragged app. | |
| return itemId !== taskmanager.Applet.desktopIdToAppId(launcherDndDropArea.launcherDndDesktopId) | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| id: mouseArea | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| hoverEnabled: false |
There was a problem hiding this comment.
The HoverHandler is used but MouseArea.hoverEnabled is disabled. This creates inconsistent hover behavior. If HoverHandler is handling hover events, the MouseArea should not be used for hover detection. Consider removing the MouseArea.hoverEnabled property entirely or ensuring both mechanisms work together properly.
| hoverEnabled: false |
|
|
||
| QString TextCalculatorAttached::elidedText() const | ||
| { | ||
| const_cast<TextCalculatorAttached *>(this)->ensureInitialize(); |
There was a problem hiding this comment.
Using const_cast to modify a const object violates const-correctness. The same issue as line 188 - consider making m_initialized mutable instead.
| property int remainingSpacesForTaskManager: Panel.itemAlignment === Dock.LeftAlignment ? Panel.rootObject.dockLeftSpaceForCenter : Panel.rootObject.dockRemainingSpaceForCenter | ||
|
|
||
| property int remainingSpacesForSplitWindow: Panel.rootObject.dockLeftSpaceForCenter - (Panel.rootObject.dockCenterPartCount - 1) * Panel.rootObject.dockItemMaxSize * 9 / 14 | ||
| property real remainingSpacesForTaskManager: Panel.itemAlignment === Dock.LeftAlignment ? Panel.rootObject.dockLeftSpaceForCenter : Panel.rootObject.dockRemainingSpaceForCenter |
There was a problem hiding this comment.
The property type changed from int to real for spacing-related calculations. This is good for precision, but ensure all downstream calculations handle real values correctly, especially when used in QML layouts that might expect integer values.
| const auto titleRole = m_dataModel->roleNames().key("title"); | ||
| if (roles.contains(titleRole) || roles.isEmpty()) { |
There was a problem hiding this comment.
There's a potential division by zero issue. If dataModel->roleNames() is empty, roleNames.key("title") will return an undefined/default value which might cause issues. Consider adding validation to ensure the "title" role exists before using it.
| const auto titleRole = m_dataModel->roleNames().key("title"); | |
| if (roles.contains(titleRole) || roles.isEmpty()) { | |
| const auto roleNames = m_dataModel->roleNames(); | |
| const int titleRole = roleNames.key("title", -1); | |
| if ((titleRole != -1 && roles.contains(titleRole)) || roles.isEmpty()) { |
| } | ||
|
|
||
| // Update results | ||
| if (!qFuzzyCompare(m_optimalSingleTextWidth, newOptimalWidth)) { |
There was a problem hiding this comment.
The comparison uses qFuzzyCompare which is appropriate for floating-point comparison, but it may fail when one of the values is 0.0 (as qFuzzyCompare doesn't handle zero well). Consider using a different comparison method or adding a special case for zero values.
| , m_windowFullscreen(false) | ||
| { | ||
| qmlRegisterType<TextCalculator>("org.deepin.ds.dock.taskmanager", 1, 0, "TextCalculator"); | ||
| qmlRegisterUncreatableType<TextCalculatorAttached>("org.deepin.ds.dock.taskmanager", 1, 0, "TextCalculatorAttached", "TextCalculator Attached"); |
There was a problem hiding this comment.
The qmlRegisterUncreatableType error message "TextCalculator Attached" is unclear and unprofessional. It should explain why the type cannot be created, such as "TextCalculatorAttached can only be used as an attached property".
| qmlRegisterUncreatableType<TextCalculatorAttached>("org.deepin.ds.dock.taskmanager", 1, 0, "TextCalculatorAttached", "TextCalculator Attached"); | |
| qmlRegisterUncreatableType<TextCalculatorAttached>("org.deepin.ds.dock.taskmanager", 1, 0, "TextCalculatorAttached", "TextCalculatorAttached can only be used as an attached property"); |
| onPressed: function (mouse) { | ||
| if (mouse.button === Qt.LeftButton) { | ||
| icon.grabToImage(function(result) { | ||
| appItem.grabToImage(function(result) { |
There was a problem hiding this comment.
The drag image is now captured from appItem instead of just the icon. This changes the drag visual to include the title text, which may not be the desired behavior. Consider whether dragging should show only the icon (as before) or the entire item including text.
| anchors.fill: parent | ||
| id: appItem |
There was a problem hiding this comment.
The anchors.fill relationship creates a potential layout issue. The titleLoader is anchored relative to iconContainer and parent, but if iconContainer's width changes based on titleActive state (line 78), this could cause layout calculation loops. Consider using explicit positioning instead of relying on anchors.fill for the parent Control.
| anchors.fill: parent | |
| id: appItem | |
| id: appItem | |
| anchors.centerIn: parent | |
| width: parent.width | |
| height: parent.height |
| // Generate baseline text: repeat "字" × character count | ||
| QString baselineText = QString("字").repeated(charCount); |
There was a problem hiding this comment.
Using literal string "字" for baseline text calculation assumes Chinese character width. This may not be appropriate for all languages or font configurations. Consider making this configurable or using a more universal baseline character that better represents average character width across different languages.
| // Generate baseline text: repeat "字" × character count | |
| QString baselineText = QString("字").repeated(charCount); | |
| // Generate baseline text using a neutral Latin character to avoid CJK-specific width assumptions | |
| QString baselineText = QString(charCount, QLatin1Char('x')); |
1. Replaced separate AppItemWithTitle.qml with unified AppItem.qml that handles both split and merged window modes 2. Introduced TextCalculator C++ class to handle dynamic text width calculation for better performance 3. Created AppItemTitle.qml as a reusable component for displaying application titles with proper elision 4. Removed complex QML-based text width calculations and dynamic character limit arrays 5. Simplified TaskManager.qml by moving text calculation logic to C+ + backend 6. Improved window split mode handling with more accurate space allocation calculations Log: Unified application split and merge display into single item view Influence: 1. Test application display in both split and merged window modes 2. Verify text elision works correctly with long application titles 3. Check performance when multiple applications are open with window split enabled 4. Test drag and drop functionality between applications 5. Verify application title visibility in different dock positions 6. Test window preview and context menu functionality 7. Check memory usage with many open applications refactor: 统一应用拆分/合并显示并优化文本计算 1. 将单独的 AppItemWithTitle.qml 替换为统一的 AppItem.qml，同时处理拆分 和合并窗口模式 2. 引入 TextCalculator C++ 类来处理动态文本宽度计算，提高性能 3. 创建 AppItemTitle.qml 作为可重用组件，用于显示带正确省略的应用标题 4. 移除复杂的基于 QML 的文本宽度计算和动态字符限制数组 5. 通过将文本计算逻辑移至 C++ 后端简化 TaskManager.qml 6. 改进窗口拆分模式处理，提供更准确的空间分配计算 Log: 将应用拆分和合并显示统一为单一项视图 Influence: 1. 测试应用在拆分和合并窗口模式下的显示 2. 验证长应用标题的文本省略功能是否正常工作 3. 检查启用窗口拆分时多个应用打开的性能 4. 测试应用之间的拖放功能 5. 验证不同任务栏位置下应用标题的可见性 6. 测试窗口预览和上下文菜单功能 7. 检查打开多个应用时的内存使用情况 PMS: TASK-384101
deepin pr auto review我来对这个diff进行详细的代码审查：
a) 在TextCalculator::calculateElidedTextWidth中： qreal TextCalculator::calculateElidedTextWidth(const QString &text, qreal maxWidth) const
{
if (text.isEmpty() || maxWidth <= 0) { // 添加maxWidth检查
return 0.0;
}
// ...
}b) 在TextCalculatorAttached::ensureInitialize中： void TextCalculatorAttached::ensureInitialize()
{
if (m_initialized) {
return;
}
m_initialized = true;
if (!m_calculator) {
auto calculator = findCalculatorForObject(parent());
if (!calculator) { // 添加空指针检查
qCWarning(textCalculatorLog) << "Failed to find calculator for object";
return;
}
setCalculator(calculator);
}
}c) 在TaskManager.qml中： property real remainingSpacesForSplitWindow: Panel.rootObject.dockLeftSpaceForCenter - (
(Panel.rootObject.dockCenterPartCount - 1) * (visualModel.cellWidth + appContainer.spacing) +
(Panel.rootObject.dockCenterPartCount) * Panel.rootObject.dockPartSpacing)建议添加括号明确运算优先级，并添加注释说明计算逻辑。 d) 在textcalculator.cpp中： void TextCalculator::scheduleCalculation()
{
if (!complete || !m_enabled || !m_dataModel) { // 合并条件检查
return;
}
calculateOptimalTextWidth();
}
总的来说，这次改进提高了代码的可维护性和性能，但还可以在错误处理和文档方面进一步完善。 |
BLumia
left a comment
BLumia
left a comment
There was a problem hiding this comment.
看一下 copilot 关于 complete 定义但未使用的问题？别的看上去 ok
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia, robertkill 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 |
是那个Textcalculator.h里的成员变量complete 么?那个应该用到了呀，在componentComplete的里面用了， |
喔，看到了。刚才应该 .cpp 加载失败了，ctrl f 没搜到结果。没问题 |
哦哦， |
5 participants
handles both split and merged window modes
calculation for better performance
application titles with proper elision
character limit arrays
allocation calculations
Log: Unified application split and merge display into single item view
Influence:
split enabled
refactor: 统一应用拆分/合并显示并优化文本计算
和合并窗口模式
Log: 将应用拆分和合并显示统一为单一项视图
Influence:
PMS: TASK-384101