Merged
Conversation
release memory Log: release memory Bug: https://pms.uniontech.com/bug-view-282985.html
优化文本编辑器事件处理与1043编译问题 Bug: https://pms.uniontech.com/bug-view-307387.html Log: 优化文本编辑器事件处理与1043编译问题
…n#349) - Change MIME type detection to use content-based matching for more accurate file type recognition - Add application/x-troff-man MIME type to supported file types list This improves file type detection accuracy by examining file contents rather than relying solely on file extensions. Log: improve MIME type detection and add man page support Bug: https://pms.uniontech.com//bug-view-314393.html
修复关闭窗口保存提示 Bug: https://pms.uniontech.com/bug-view-315439.html Log: 修复关闭窗口保存提示
Reviewer's GuideAdds delayed heap memory trimming, a save-all-files workflow on window close, improved screen selection for shortcuts, better MIME detection, and tab bar UX tweaks such as middle-click close and accepting drag-and-drop, plus a safety check in auto backup when no windows exist. Sequence diagram for delayed heap memory trimmingsequenceDiagram
participant Window
participant StartManager
participant QBasicTimer
participant SystemAllocator
Note over Window,StartManager: delayMallocTrim is invoked after wrapper removal, tabbar reload, and before app quit.
Window->>StartManager: delayMallocTrim()
activate StartManager
alt m_FreeMemTimer not active
StartManager->>QBasicTimer: start(1000, this)
else already active
StartManager->>StartManager: do nothing
end
deactivate StartManager
QBasicTimer-->>StartManager: timerEvent(e) after 1000ms
activate StartManager
StartManager->>StartManager: check e->timerId() == m_FreeMemTimer.timerId()
alt matches FreeMemTimer
StartManager->>QBasicTimer: stop()
StartManager->>SystemAllocator: malloc_trim(0)
end
deactivate StartManager
Updated class diagram for Window, StartManager, and TabbarclassDiagram
class Window {
// Existing responsibilities omitted
+void backupFile()
+bool closeAllFiles()
+bool saveAllFiles()
+void removeWrapper(QString filePath, bool isDelete)
+void displayShortcuts()
+void checkTabbarForReload()
+void closeEvent(QCloseEvent *e)
}
class StartManager {
+static StartManager* instance()
+void autoBackupFile()
+void slotCloseWindow()
+void slotDelayBackupFile()
+void delayMallocTrim()
+void timerEvent(QTimerEvent *e)
QList~Window*~ m_windows
bool m_bIsTagDragging
QBasicTimer m_FreeMemTimer
Window *pFocusWindow
}
class Tabbar {
+Tabbar(QWidget *parent)
+bool eventFilter(QObject *object, QEvent *event)
+void dropEvent(QDropEvent *event)
+void resizeEvent(QResizeEvent *event)
+QSize tabSizeHint(int index) const
+QSize minimumTabSizeHint(int index) const
+QSize maximumTabSizeHint(int index) const
+void setAcceptDrops(bool on)
+void setDragable(bool dragable)
+void setVisibleAddButton(bool visible)
+void setTabsClosable(bool closable)
+void setTabPalette(QString normalColor, QString selectedColor)
+signal tabCloseRequested(int index)
}
class EditWrapper {
+bool isDraftFile() const
+bool isModified() const
+bool isTemFile() const
+bool saveFile()
+void saveDraftFile(QString newFilePath)
}
Window --> Tabbar : uses
Window --> EditWrapper : manages m_wrappers
StartManager --> Window : manages m_windows
StartManager --> QBasicTimer : uses m_FreeMemTimer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note
详情{
"src/widgets/window.cpp": [
{
"line": " QString key = \"base/enable\";",
"line_number": 388,
"rule": "S106",
"reason": "Var naming | 64f28539d9"
}
]
} |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
Window::saveAllFiles(), the loopfor (int i = wrappers.count() - 1; i > 0; i--)skips index 0 and usesm_wrappersinstead of the copiedwrappers; consider iterating down toi >= 0and consistently using a single source of truth (e.g.,m_wrappersonly) to avoid missing the first tab and potential stale-map issues. - The early
return falsepaths insaveAllFiles()for cases like emptyfilePathor missingwrapperwill abort the entire save-all operation on one bad tab; if the intent is to save as many files as possible, switching these tocontinue(and maybe logging) would make the behavior more robust. - In
StartManager::timerEvent, the existing auto-backup logic (m_pTimer) now runs for every timer event, includingm_FreeMemTimer; it would be safer to structure this with separateif/else ifbranches pertimerIdso the malloc-trim timer doesn't inadvertently trigger or modify backup-related timers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Window::saveAllFiles()`, the loop `for (int i = wrappers.count() - 1; i > 0; i--)` skips index 0 and uses `m_wrappers` instead of the copied `wrappers`; consider iterating down to `i >= 0` and consistently using a single source of truth (e.g., `m_wrappers` only) to avoid missing the first tab and potential stale-map issues.
- The early `return false` paths in `saveAllFiles()` for cases like empty `filePath` or missing `wrapper` will abort the entire save-all operation on one bad tab; if the intent is to save as many files as possible, switching these to `continue` (and maybe logging) would make the behavior more robust.
- In `StartManager::timerEvent`, the existing auto-backup logic (`m_pTimer`) now runs for every timer event, including `m_FreeMemTimer`; it would be safer to structure this with separate `if/else if` branches per `timerId` so the malloc-trim timer doesn't inadvertently trigger or modify backup-related timers.
## Individual Comments
### Comment 1
<location> `src/widgets/window.cpp:2942-2946` </location>
<code_context>
return true;
}
+bool Window::saveAllFiles()
+{
+ qInfo() << "begin saveAllFiles()";
+ QMap<QString, EditWrapper *> wrappers = m_wrappers;
+ for (int i = wrappers.count() - 1; i > 0; i--) {
+ const QString &filePath = m_tabbar->fileAt(i);
+ // 避免异常情况重入时当前已无标签页的情况
</code_context>
<issue_to_address>
**issue (bug_risk):** Loop skips index 0, so the first tab is never processed during save-all.
Because the loop uses `i > 0`, index 0 is never visited, so the first tab is never checked or saved in `saveAllFiles()`. This can leave the leftmost file unsaved when closing. Please update the loop (e.g., `i >= 0` with a `wrappers.count() > 0` guard) or otherwise ensure index 0 is handled.
</issue_to_address>
### Comment 2
<location> `src/widgets/window.cpp:2958-2967` </location>
<code_context>
+ bool bIsBackupFile = false;
</code_context>
<issue_to_address>
**suggestion:** Backup and normal file branches duplicate logic and `bIsBackupFile` is unnecessary.
`bIsBackupFile` is only set from `wrapper->isTemFile()` and both branches run the same `if (!wrapper->saveFile()) { saveAsFile(); }` logic, so it doesn’t change behavior. Consider removing `bIsBackupFile` and merging the branches into a single `else` for simpler, clearer code.
Suggested implementation:
```cpp
bool isDraftFile = wrapper->isDraftFile();
bool isModified = wrapper->isModified();
if (isModified) {
```
`bIsBackupFile` is likely used later in this same function to decide between a “backup file” branch and a “normal file” branch around the `saveFile` / `saveAsFile` calls. To fully implement your comment:
1. Find the `if (bIsBackupFile)` or similar conditional later in this function.
2. Replace that `if/else` with a single branch that executes the shared logic:
- Call `wrapper->saveFile()`.
- If it returns `false`, call `saveAsFile()` (or the existing equivalent).
3. Remove any remaining references to `bIsBackupFile` in this function, since the flag is now gone and behavior is unified.
</issue_to_address>
### Comment 3
<location> `src/controls/tabbar.cpp:751-755` </location>
<code_context>
}
}
-
+ if (mouseEvent->button() == Qt::MidButton) {
+ emit tabCloseRequested(tabAt(QPoint(mouseEvent->x(), mouseEvent->y())));
+ return true;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Middle-click close should guard against `tabAt` returning -1.
`tabAt(...)` may return -1 if the middle click is outside any tab, and that value will be propagated via `tabCloseRequested`. Some consumers may not handle negative indexes correctly. Please store the result of `tabAt(...)` in a local variable, emit the signal only when the index is >= 0, and use `mouseEvent->pos()` instead of constructing a `QPoint` from `x()`/`y()` manually.
```suggestion
}
if (mouseEvent->button() == Qt::MidButton) {
const int index = tabAt(mouseEvent->pos());
if (index >= 0) {
emit tabCloseRequested(index);
}
return true;
}
```
</issue_to_address>
### Comment 4
<location> `src/startmanager.cpp:176-177` </location>
<code_context>
QFileInfo fileInfo;
m_qlistTemFile.clear();
listBackupInfo = Settings::instance()->settings->option("advance.editor.browsing_history_temfile")->value().toStringList();
+ if (m_windows.isEmpty()) {
+ return;
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** Early return when `m_windows` is empty changes backup behavior for stored temp files.
This early return prevents `autoBackupFile` from reconciling `listBackupInfo` when there are no open windows, which may leave backup records stale if prior behavior depended on that cleanup. If the change is intentional, consider moving the early return before loading `listBackupInfo` to avoid the now-unnecessary read.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+2958
to
+2967
| bool bIsBackupFile = false; | ||
|
|
||
| if (wrapper->isTemFile()) { | ||
| bIsBackupFile = true; | ||
| } | ||
|
|
||
| if (isModified) { | ||
| DDialog *dialog = createDialog(tr("Do you want to save this file?"), ""); | ||
| int res = dialog->exec(); | ||
|
|
There was a problem hiding this comment.
suggestion: Backup and normal file branches duplicate logic and bIsBackupFile is unnecessary.
bIsBackupFile is only set from wrapper->isTemFile() and both branches run the same if (!wrapper->saveFile()) { saveAsFile(); } logic, so it doesn’t change behavior. Consider removing bIsBackupFile and merging the branches into a single else for simpler, clearer code.
Suggested implementation:
bool isDraftFile = wrapper->isDraftFile();
bool isModified = wrapper->isModified();
if (isModified) {
bIsBackupFile is likely used later in this same function to decide between a “backup file” branch and a “normal file” branch around the saveFile / saveAsFile calls. To fully implement your comment:
- Find the
if (bIsBackupFile)or similar conditional later in this function. - Replace that
if/elsewith a single branch that executes the shared logic:- Call
wrapper->saveFile(). - If it returns
false, callsaveAsFile()(or the existing equivalent).
- Call
- Remove any remaining references to
bIsBackupFilein this function, since the flag is now gone and behavior is unified.
deepin pr auto review我来对这段代码进行审查，从语法逻辑、代码质量、性能和安全几个方面进行分析：
QString mimeType = QMimeDatabase().mimeTypeForFile(filepath, QMimeDatabase::MatchMode::MatchContent).name();改进建议：
setAcceptDrops(true);和 if (mouseEvent->button() == Qt::MidButton) {
emit tabCloseRequested(tabAt(QPoint(mouseEvent->x(), mouseEvent->y())));
return true;
}改进建议：
void StartManager::delayMallocTrim()
{
if (!m_FreeMemTimer.isActive()) {
m_FreeMemTimer.start(1000, this);
}
}改进建议：
bool Window::saveAllFiles()
{
// ... 保存所有文件的逻辑
}改进建议：
总体来说，这些修改主要是功能增强，但还需要在错误处理、性能优化和安全性方面做更多工作。建议优先处理安全性相关的问题，然后逐步完善其他方面。 |
lzwind
approved these changes
Dec 18, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, pppanghu77 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 |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Summary by Sourcery
Add window-wide save-all behavior on close, improve resource cleanup timing, and refine tab interaction and file handling behavior.
New Features:
Bug Fixes:
Enhancements: