-
Notifications
You must be signed in to change notification settings - Fork 78
fix: The "Do not keep tabs" feature is not working. #403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: The "Do not keep tabs" feature is not working. #403
Conversation
不保留标签页功能未生效,因为历史上的一条提交引入了错误代码导致提前返回了,导致历史文件记录没有被清空。 revert 9ce896a. Bug: https://pms.uniontech.com/bug-view-341299.html
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes the "Do not keep tabs" feature by always loading and persisting the browsing history list regardless of the startup setting, and by removing an early return that prevented clearing/saving history when no windows were open, plus adding logging for easier debugging. Sequence diagram for updated autoBackupFile history persistencesequenceDiagram
participant StartManager
participant Settings
participant QSettings
StartManager->>StartManager: autoBackupFile()
StartManager->>StartManager: m_qlistTemFile.clear()
StartManager->>Settings: instance()
Settings-->>StartManager: settings
StartManager->>QSettings: option(advance.editor.browsing_history_temfile)
QSettings-->>StartManager: current_history_list
StartManager->>StartManager: iterate m_windows and build m_qlistTemFile
StartManager->>StartManager: qInfo history file counts
StartManager->>QSettings: setValue(m_qlistTemFile)
StartManager->>StartManager: saveBookmark()
Sequence diagram for loading history in StartManager constructorsequenceDiagram
participant StartManager
participant Settings
participant QSettings
StartManager->>StartManager: StartManager(parent)
StartManager->>Settings: instance()
Settings-->>StartManager: settings
StartManager->>QSettings: option(advance.editor.browsing_history_temfile)
QSettings-->>StartManager: saved_history_list
StartManager->>StartManager: m_qlistTemFile = saved_history_list
StartManager->>StartManager: initBookmark()
Updated class diagram for StartManager history handlingclassDiagram
class StartManager {
-QStringList m_qlistTemFile
-QList_Window_ m_windows
+StartManager(parent: QObject)
+autoBackupFile()
+initBookmark()
+saveBookmark()
}
class Settings {
+static Settings instance()
+QSettings settings
}
StartManager --> Settings : uses
StartManager --> QSettings : reads_and_writes_history
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这个git diff进行代码审查:
改进建议:
// 直接加载历史文件列表,因为后续逻辑中会根据实际需要判断是否使用这些文件
m_qlistTemFile = Settings::instance()->settings->option("advance.editor.browsing_history_temfile")->value().toStringList();
// 如果没有窗口打开,直接保存空列表
if (m_windows.isEmpty()) {
Settings::instance()->settings->option("advance.editor.browsing_history_temfile")->setValue(QStringList());
return;
}
qInfo() << __func__ << "Saving" << m_qlistTemFile.size() << "files to browsing history";
// 在访问设置时添加错误处理
auto settingsOption = Settings::instance()->settings->option("advance.editor.browsing_history_temfile");
if (!settingsOption) {
qWarning() << __func__ << "Failed to access browsing history setting";
return;
}这些修改建议主要是为了提高代码的健壮性和可维护性,同时保持代码的性能和安全性。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/startmanager.cpp:222` </location>
<code_context>
}
//将json串列表写入配置文件
+ qInfo() << __func__ << "history file counts:" << m_qlistTemFile.size();
Settings::instance()->settings->option("advance.editor.browsing_history_temfile")->setValue(m_qlistTemFile);
// 备份书签信息
</code_context>
<issue_to_address>
**suggestion:** Consider reducing verbosity or using a more appropriate log level for frequent backup logs.
If autoBackupFile() is called frequently, logging with qInfo() every time can clutter production logs. Prefer a debug category (e.g. qCDebug(...)) so it’s disabled in release, or only log when the count changes or is non-zero to keep logs useful but concise.
Suggested implementation:
```cpp
//将json串列表写入配置文件
// 使用调试日志并仅在有历史文件时输出,避免频繁备份时生产日志过于冗长
if (!m_qlistTemFile.isEmpty()) {
qDebug() << __func__ << "history file counts:" << m_qlistTemFile.size();
}
Settings::instance()->settings->option("advance.editor.browsing_history_temfile")->setValue(m_qlistTemFile);
```
- Ensure that qDebug() is acceptable in your logging policy for debug/diagnostic output. In many Qt projects, qDebug is either compiled out or filtered in release builds, which helps keep production logs concise.
- If your codebase uses QLoggingCategory for more granular control, you may want to replace qDebug() here with qCDebug(yourCategory) and define/configure that category consistently with the rest of the project.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lichaofan2008, max-lvs 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 |
|
/merge |
不保留标签页功能未生效,因为历史上的一条提交引入了错误代码导致提前返回了,导致历史文件记录没有被清空。 revert 9ce896a.
Bug: https://pms.uniontech.com/bug-view-341299.html
Summary by Sourcery
Fix tab history handling so temporary file records are always updated and respect the "Do not keep tabs" behavior.
Bug Fixes: