feat: add indicator display control #2979
Merged
+48
−20
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
Reviewer's GuideAdds a command-based mechanism to control whether the page highlight indicator is shown during navigation, and wires this through the DccManager, DccApp active item signal, QML right/settings views, and the power plugin’s scheduled-shutdown pages to avoid flicker on quick transitions. Sequence diagram for page navigation with optional indicator displaysequenceDiagram
actor User
participant GeneralPage_repeatDays as GeneralPage_repeatDays
participant DccApp as DccApp
participant DccManager as DccManager
participant DccRightView as DccRightView_panel
participant DccSettingsView as DccSettingsView_panel
participant obj as obj
User->>GeneralPage_repeatDays: Change shutdownRepetition quickly
activate GeneralPage_repeatDays
GeneralPage_repeatDays->>GeneralPage_repeatDays: onVisibleChanged visible=true
GeneralPage_repeatDays->>GeneralPage_repeatDays: showTime = now
deactivate GeneralPage_repeatDays
User->>GeneralPage_repeatDays: Open repeatDaysEdit quickly
activate GeneralPage_repeatDays
GeneralPage_repeatDays->>DccApp: showPage(dccObj, indicator=false)
deactivate GeneralPage_repeatDays
activate DccApp
DccApp->>DccManager: doShowPage(obj, cmd)
deactivate DccApp
activate DccManager
DccManager->>DccManager: indicatorShown = isIndicatorShown(cmd)
alt indicatorShown and cmd not empty
DccManager-->>obj: active(cmd)
DccManager-->>DccRightView: activeItemChanged(parentItem, true)
DccManager-->>DccSettingsView: activeItemChanged(parentItem, true)
else indicatorShown is false or cmd empty
DccManager-->>DccRightView: activeItemChanged(parentItem, indicatorShown)
DccManager-->>DccSettingsView: activeItemChanged(parentItem, indicatorShown)
end
deactivate DccManager
activate DccRightView
DccRightView->>DccRightView: panel.item = item
DccRightView->>DccRightView: panel.isIndicatorShown = isIndicatorShown
DccRightView->>DccRightView: Timer onTriggered
alt panel.isIndicatorShown
DccRightView->>DccRightView: Update panel geometry
DccRightView->>DccRightView: panel.visible = cnt & 1
else indicator hidden
DccRightView->>DccRightView: Skip geometry and visibility update
DccRightView->>DccRightView: panel.visible remains false
end
deactivate DccRightView
activate DccSettingsView
DccSettingsView->>DccSettingsView: Same handling as DccRightView
deactivate DccSettingsView
Class diagram for updated DccManager and indicator controlclassDiagram
class DccManager {
- QSet~QString~ m_hideModule
+ void waitShowPage(QString url, QDBusMessage message)
+ void doShowPage(DccObject *obj, QString cmd)
+ bool eventFilter(QObject *watched, QEvent *event)
+ bool isIndicatorShown(QString cmd) const
+ QVector~DccObject*~ findObjects(QString url, bool onlyRoot, bool one)
+ DccObject * findParent(DccObject *obj)
+ void saveSize()
+ void activeItemChanged(QQuickItem *item, bool isIndicatorShown)
}
class DccRightView_Panel {
+ var item
+ int cnt
+ bool isIndicatorShown
+ int x
+ int y
+ int width
+ int height
+ bool visible
+ Timer timer
}
class DccSettingsView_Panel {
+ var item
+ int cnt
+ bool isIndicatorShown
+ int x
+ int y
+ int width
+ int height
+ bool visible
+ Timer timer
}
class GeneralPage_repeatDays {
+ real showTime
+ bool visible
+ void onVisibleChanged()
}
class GeneralPage_repeatDaysEdit {
+ bool forceShow
+ real showTime
+ bool visible
+ void onVisibleChanged()
}
DccManager ..> DccRightView_Panel : emits activeItemChanged
DccManager ..> DccSettingsView_Panel : emits activeItemChanged
DccRightView_Panel --> DccManager : depends_on_indicator_state
DccSettingsView_Panel --> DccManager : depends_on_indicator_state
GeneralPage_repeatDays --> DccManager : uses_showPage_indicator_false
GeneralPage_repeatDaysEdit --> DccManager : uses_showPage_indicator_false
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 - I've found 2 issues, and left some high level feedback:
- In the new QML
Component.onCompletedhandlers (for the repeat combo and repeatDaysEdit), the localtimevariable is never used and can be removed to simplify the code. - The
isIndicatorShown(const QString &cmd)check only matches the exact string "indicator=false"; ifcmdcan contain multiple parameters or different formatting, consider parsing it or using a more robust check (e.g. searching for a key/value pair) so indicator control is consistent. - The change in
waitShowPagefromif (cmd.isEmpty()) show();toif (!isIndicatorShown(cmd) || cmd.isEmpty()) show();subtly alters when the window is shown; it may be worth revalidating the intended behavior when commands other thanindicator=falseare passed to avoid regressions in window visibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new QML `Component.onCompleted` handlers (for the repeat combo and repeatDaysEdit), the local `time` variable is never used and can be removed to simplify the code.
- The `isIndicatorShown(const QString &cmd)` check only matches the exact string "indicator=false"; if `cmd` can contain multiple parameters or different formatting, consider parsing it or using a more robust check (e.g. searching for a key/value pair) so indicator control is consistent.
- The change in `waitShowPage` from `if (cmd.isEmpty()) show();` to `if (!isIndicatorShown(cmd) || cmd.isEmpty()) show();` subtly alters when the window is shown; it may be worth revalidating the intended behavior when commands other than `indicator=false` are passed to avoid regressions in window visibility.
## Individual Comments
### Comment 1
<location> `src/plugin-power/qml/GeneralPage.qml:241-250` </location>
<code_context>
}
DccObject {
+ property real showTime: 0
name: "repeatDays"
parentName: "power/general/shutdownGroup"
</code_context>
<issue_to_address>
**nitpick:** The timing logic for `showTime` likely wants to use a consistent timestamp variable instead of multiple `new Date().getTime()` calls, and avoid the unused `time` variable.
In `Component.onCompleted`, `var time = new Date().getTime()` is declared but not used, and `new Date().getTime()` is called again in the condition. This is slightly inefficient and can introduce minor timing inconsistencies with `showTime`. Please either reuse `time` in the condition (e.g. `if (dccData.model.shutdownRepetition !== 3 && time - dccObj.showTime < 300)`) or remove the local variable if it’s unnecessary.
</issue_to_address>
### Comment 2
<location> `src/plugin-power/qml/GeneralPage.qml:293-297` </location>
<code_context>
}
DccObject {
id: repeatDaysEditObject
property bool forceShow: false
+ property real showTime: 0
name: "repeatDaysEdit"
</code_context>
<issue_to_address>
**issue (bug_risk):** `repeatDaysEditObject.showTime` is never used and the code still references `dccObj.showTime`, which looks inconsistent with the new property.
`repeatDaysEditObject.showTime` is set in `onVisibleChanged`, but the inner `Component.onCompleted` still uses `dccObj.showTime`. That leaves the new property unused and keeps the timing check tied to `dccObj`’s visibility. If the timing is meant to follow `repeatDaysEditObject` becoming visible, update the condition to use `repeatDaysEditObject.showTime`; otherwise remove the new property to avoid dead code.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/plugin-power/qml/GeneralPage.qml
Outdated
Comment on lines
241
to
250
| property real showTime: 0 | ||
| name: "repeatDays" | ||
| parentName: "power/general/shutdownGroup" | ||
| visible: dccData.model.scheduledShutdownState | ||
| displayName: qsTr("Repeat") | ||
| weight: 3 | ||
| pageType: DccObject.Editor | ||
| onVisibleChanged: { | ||
| if (visible) { | ||
| showTime = new Date().getTime() |
There was a problem hiding this comment.
nitpick: The timing logic for showTime likely wants to use a consistent timestamp variable instead of multiple new Date().getTime() calls, and avoid the unused time variable.
In Component.onCompleted, var time = new Date().getTime() is declared but not used, and new Date().getTime() is called again in the condition. This is slightly inefficient and can introduce minor timing inconsistencies with showTime. Please either reuse time in the condition (e.g. if (dccData.model.shutdownRepetition !== 3 && time - dccObj.showTime < 300)) or remove the local variable if it’s unnecessary.
Comment on lines
293
to
298
| property bool forceShow: false | ||
| property real showTime: 0 | ||
| name: "repeatDaysEdit" | ||
| parentName: "power/general/shutdownGroup" | ||
| visible: (dccData.model.scheduledShutdownState && dccData.model.shutdownRepetition === 3) |
There was a problem hiding this comment.
issue (bug_risk): repeatDaysEditObject.showTime is never used and the code still references dccObj.showTime, which looks inconsistent with the new property.
repeatDaysEditObject.showTime is set in onVisibleChanged, but the inner Component.onCompleted still uses dccObj.showTime. That leaves the new property unused and keeps the timing check tied to dccObj’s visibility. If the timing is meant to follow repeatDaysEditObject becoming visible, update the condition to use repeatDaysEditObject.showTime; otherwise remove the new property to avoid dead code.
caixr23
force-pushed
the
BUG-291067
branch
7 times, most recently
from
12681de to
9fe09a8
Compare
mhduiy
approved these changes
Jan 31, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy 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 |
1. Added isIndicatorShown() method to detect "indicator=false" command 2. Modified doShowPage() to conditionally show indicators based on command 3. Updated activeItemChanged signal to include indicator visibility state 4. Enhanced QML components with isIndicatorShown property and conditional rendering 5. Added timing control in power plugin to prevent indicator flickering during quick page transitions Log: Added control for indicator display during page navigation Influence: 1. Test page navigation with and without "indicator=false" parameter 2. Verify indicator visibility during quick page transitions 3. Check power plugin's repeat days and custom time settings 4. Test that normal page navigation still shows indicators correctly 5. Verify that indicator hiding doesn't affect other UI functionality feat: 增加指示器显示控制功能 1. 添加 isIndicatorShown() 方法检测 "indicator=false" 命令 2. 修改 doShowPage() 方法根据命令条件性显示指示器 3. 更新 activeItemChanged 信号包含指示器可见性状态 4. 增强 QML 组件添加 isIndicatorShown 属性和条件渲染 5. 在电源插件中添加时序控制防止快速页面切换时的指示器闪烁 Log: 新增页面导航时的指示器显示控制 Influence: 1. 测试带和不带 "indicator=false" 参数的页面导航 2. 验证快速页面切换时的指示器可见性 3. 检查电源插件的重复天数和自定义时间设置 4. 测试正常页面导航仍正确显示指示器 5. 验证指示器隐藏不影响其他 UI 功能 PMS: BUG-291067
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
deepin pr auto review这份代码审查主要针对提供的 git diff，我会从语法逻辑、代码质量、代码性能和代码安全四个方面进行分析。 1. 语法逻辑优点：
问题与建议：
2. 代码质量优点：
问题与建议：
3. 代码性能优点：
问题与建议：
4. 代码安全问题与建议：
总结与改进代码示例建议修改 // searchmodel.cpp
#include <QUrl>
#include <QUrlQuery>
// ... 在 data 函数中 ...
case SearchModel::SearchUrlRole: {
QUrl url(data->sourceUrl());
if (url.isValid()) {
QUrlQuery query(url);
// 只有当不存在该参数时才添加，避免重复
if (!query.hasQueryItem("indicator")) {
query.addQueryItem("indicator", "true");
url.setQuery(query);
}
}
return url.isValid() ? url.toString() : data->sourceUrl();
}建议定义常量以避免 Magic String： // dccmanager.h
class DccManager : public QObject {
// ...
private:
static const QString INDICATOR_CMD_FLAG;
// ...
};
// dccmanager.cpp
const QString DccManager::INDICATOR_CMD_FLAG = "indicator=true";
bool DccManager::isIndicatorShown(const QString &cmd) const
{
return cmd == INDICATOR_CMD_FLAG;
}关于 QML 的建议： 总体评价： |
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.
Log: Added control for indicator display during page navigation
Influence:
feat: 增加指示器显示控制功能
Log: 新增页面导航时的指示器显示控制
Influence:
PMS: BUG-291067
Summary by Sourcery
Add configurable control over the right-side selection indicator during page navigation and apply it to prevent flicker in power settings pages.
New Features:
Bug Fixes:
Enhancements: