fix(vnote): flush editor content before note save actions#377
Draft
dengzhongyuan365-dev wants to merge 1 commit into
Draft
fix(vnote): flush editor content before note save actions#377dengzhongyuan365-dev wants to merge 1 commit into
dengzhongyuan365-dev wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengzhongyuan365-dev 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 |
Reviewer's GuideThis PR ensures the rich text editor flushes and binds its content updates to a specific note ID and change serial before context menu and save actions, preventing stale or missing content when quickly saving or opening menus. Sequence diagram for serial-bound note update from editorsequenceDiagram
actor User
participant WebRichTextManager
participant VNoteMainManager
participant WebEngineView
participant webView
User->>WebRichTextManager: updateNote()
WebRichTextManager->>VNoteMainManager: needUpdateNote(noteId, serial)
VNoteMainManager-->>WebEngineView: needUpdateNote(noteId, serial)
WebEngineView->>webView: runJavaScript(getHtml)
webView-->>WebEngineView: result
WebEngineView->>VNoteMainManager: updateNoteWithResultForNote(noteId, serial, result)
VNoteMainManager->>WebRichTextManager: onUpdateNoteWithResult(note, serial, result)
WebRichTextManager->>WebRichTextManager: saveNoteWithResult(note, result)
WebRichTextManager-->>VNoteMainManager: finishedUpdateNote()
Sequence diagram for flushing editor before note context menu and savesequenceDiagram
actor User
participant ItemListView
participant MainWindow
participant WebEngineView
participant webView
participant VNoteMainManager
participant WebRichTextManager
User->>ItemListView: requestNoteContextMenu(...)
ItemListView-->>MainWindow: requestNoteContextMenu(noteIds, popupIndex, popupX, popupY)
MainWindow->>WebEngineView: flushCurrentNote(callback)
WebEngineView->>VNoteMainManager: currentNoteId()
WebEngineView->>VNoteMainManager: currentTextChangeSerial()
WebEngineView->>webView: runJavaScript(getHtml)
webView-->>WebEngineView: result
WebEngineView->>VNoteMainManager: flushNoteWithResultForNote(noteId, serial, result)
VNoteMainManager->>WebRichTextManager: flushNoteWithResult(note, serial, result)
WebRichTextManager->>WebRichTextManager: saveNoteWithResult(note, result)
WebRichTextManager->>WebRichTextManager: clearTextChangeState(noteId, serial)
WebRichTextManager-->>VNoteMainManager: finishedUpdateNote()
WebEngineView-->>MainWindow: callback()
MainWindow-->>ItemListView: popupNoteContextMenu(noteIds, popupIndex, popupX, popupY)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note
详情{
"src/common/VNoteMainManager.cpp": [
{
"line": " url = \"https://www.deepin.org/zh/agreement/privacy/\";",
"line_number": 1374,
"rule": "S35",
"reason": "Url link | 20e2eab189"
},
{
"line": " url = \"https://www.uniontech.com/agreement/privacy-cn\";",
"line_number": 1376,
"rule": "S35",
"reason": "Url link | 4850a00dd7"
},
{
"line": " url = \"https://www.deepin.org/en/agreement/privacy/\";",
"line_number": 1380,
"rule": "S35",
"reason": "Url link | 38d42f63bf"
},
{
"line": " url = \"https://www.uniontech.com/agreement/privacy-en\";",
"line_number": 1382,
"rule": "S35",
"reason": "Url link | f82409d3b5"
}
]
} |
![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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/gui/mainwindow/ItemListView.qml" line_range="86-94" />
<code_context>
+ if (popupIndex >= 0 && popupIndex < itemModel.count) {
+ var item = itemListView.itemAtIndex(popupIndex);
+ if (item) {
+ try {
+ noteCtxMenu.popup(item, popupX, popupY);
+ return;
+ } catch (e) {
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid silently swallowing exceptions when popping the context menu.
The empty `catch` around `noteCtxMenu.popup(item, popupX, popupY)` masks runtime errors and complicates debugging. Please at least log the exception (e.g., `console.error(e)`) or rethrow in debug builds so issues are visible while still allowing the fallback `noteCtxMenu.popup()` path when needed.
```suggestion
if (popupIndex >= 0 && popupIndex < itemModel.count) {
var item = itemListView.itemAtIndex(popupIndex);
if (item) {
try {
noteCtxMenu.popup(item, popupX, popupY);
return;
} catch (e) {
console.error("Failed to popup note context menu for index", popupIndex, "and noteIds", noteIds, e);
}
}
```
</issue_to_address>
### Comment 2
<location path="src/common/webrichetextmanager.cpp" line_range="238" />
<code_context>
qInfo() << "Note update with result finished";
}
+bool WebRichTextManager::clearTextChangeState(int noteId, quint64 serial)
+{
+ bool clearedUpdateRequest = false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared state-reset and serial-checking logic into small helper methods to simplify and clarify update handling.
You can reduce the added complexity by consolidating state management and serial checking into a couple of small helpers.
### 1. Centralize update state reset
Right now `onUpdateNoteWithResult` and `clearTextChangeState` both reset `m_updateInProgress`, `m_updateRequestNoteId`, and `m_updateRequestSerial`. You can encapsulate this and remove duplication:
```cpp
void WebRichTextManager::resetUpdateRequestState()
{
m_updateInProgress = false;
m_updateRequestNoteId = -1;
m_updateRequestSerial = 0;
}
```
Use it in `onUpdateNoteWithResult`:
```cpp
if (!data) {
qWarning() << "onUpdateNoteWithResult called with null data, skip";
resetUpdateRequestState();
emit finishedUpdateNote();
return;
}
// ...
if (data->noteId != m_textChangeNoteId || serial != m_textChangeSerial) {
qWarning() << "Discard outdated note update result, noteId:" << data->noteId
<< "serial:" << serial
<< "textChangeNoteId:" << m_textChangeNoteId
<< "textChangeSerial:" << m_textChangeSerial;
resetUpdateRequestState();
emit finishedUpdateNote();
return;
}
// ...
resetUpdateRequestState();
emit finishedUpdateNote();
```
And in `clearTextChangeState`:
```cpp
bool WebRichTextManager::clearTextChangeState(int noteId, quint64 serial)
{
bool clearedUpdateRequest = false;
if (noteId == m_textChangeNoteId && serial == m_textChangeSerial) {
m_textChange = false;
m_textChangeNoteId = -1;
}
if (noteId == m_updateRequestNoteId && serial == m_updateRequestSerial) {
resetUpdateRequestState();
clearedUpdateRequest = true;
}
return clearedUpdateRequest;
}
```
This keeps all behavior but makes it obvious that there is a single way to “end” an update request.
### 2. Separate “clear state” from “control flow” in `clearTextChangeState`
To avoid `clearTextChangeState` doubling as a control flag, you can split responsibilities while preserving the current external behavior.
First, make `clearTextChangeState` only mutate text-change-related state:
```cpp
void WebRichTextManager::clearTextChangeState(int noteId, quint64 serial)
{
if (noteId == m_textChangeNoteId && serial == m_textChangeSerial) {
m_textChange = false;
m_textChangeNoteId = -1;
}
}
```
Then add a dedicated helper for the update request:
```cpp
bool WebRichTextManager::clearUpdateRequestState(int noteId, quint64 serial)
{
if (noteId == m_updateRequestNoteId && serial == m_updateRequestSerial) {
resetUpdateRequestState();
return true;
}
return false;
}
```
Update `flushNoteWithResult` to explicitly decide when to emit:
```cpp
void WebRichTextManager::flushNoteWithResult(VNoteItem *data, quint64 serial, const QString &result)
{
if (!data) {
return;
}
if (saveNoteWithResult(data, result)) {
clearTextChangeState(data->noteId, serial);
if (clearUpdateRequestState(data->noteId, serial)) {
emit finishedUpdateNote();
}
}
}
```
This keeps all semantics intact but makes the control flow clearer: helpers only mutate state; the caller decides when to emit signals.
</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
+86
to
+94
| if (popupIndex >= 0 && popupIndex < itemModel.count) { | ||
| var item = itemListView.itemAtIndex(popupIndex); | ||
| if (item) { | ||
| try { | ||
| noteCtxMenu.popup(item, popupX, popupY); | ||
| return; | ||
| } catch (e) { | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid silently swallowing exceptions when popping the context menu.
The empty catch around noteCtxMenu.popup(item, popupX, popupY) masks runtime errors and complicates debugging. Please at least log the exception (e.g., console.error(e)) or rethrow in debug builds so issues are visible while still allowing the fallback noteCtxMenu.popup() path when needed.
Suggested change
| if (popupIndex >= 0 && popupIndex < itemModel.count) { | |
| var item = itemListView.itemAtIndex(popupIndex); | |
| if (item) { | |
| try { | |
| noteCtxMenu.popup(item, popupX, popupY); | |
| return; | |
| } catch (e) { | |
| } | |
| } | |
| if (popupIndex >= 0 && popupIndex < itemModel.count) { | |
| var item = itemListView.itemAtIndex(popupIndex); | |
| if (item) { | |
| try { | |
| noteCtxMenu.popup(item, popupX, popupY); | |
| return; | |
| } catch (e) { | |
| console.error("Failed to popup note context menu for index", popupIndex, "and noteIds", noteIds, e); | |
| } | |
| } |
| qInfo() << "Note update with result finished"; | ||
| } | ||
|
|
||
| bool WebRichTextManager::clearTextChangeState(int noteId, quint64 serial) |
There was a problem hiding this comment.
issue (complexity): Consider extracting shared state-reset and serial-checking logic into small helper methods to simplify and clarify update handling.
You can reduce the added complexity by consolidating state management and serial checking into a couple of small helpers.
1. Centralize update state reset
Right now onUpdateNoteWithResult and clearTextChangeState both reset m_updateInProgress, m_updateRequestNoteId, and m_updateRequestSerial. You can encapsulate this and remove duplication:
void WebRichTextManager::resetUpdateRequestState()
{
m_updateInProgress = false;
m_updateRequestNoteId = -1;
m_updateRequestSerial = 0;
}Use it in onUpdateNoteWithResult:
if (!data) {
qWarning() << "onUpdateNoteWithResult called with null data, skip";
resetUpdateRequestState();
emit finishedUpdateNote();
return;
}
// ...
if (data->noteId != m_textChangeNoteId || serial != m_textChangeSerial) {
qWarning() << "Discard outdated note update result, noteId:" << data->noteId
<< "serial:" << serial
<< "textChangeNoteId:" << m_textChangeNoteId
<< "textChangeSerial:" << m_textChangeSerial;
resetUpdateRequestState();
emit finishedUpdateNote();
return;
}
// ...
resetUpdateRequestState();
emit finishedUpdateNote();And in clearTextChangeState:
bool WebRichTextManager::clearTextChangeState(int noteId, quint64 serial)
{
bool clearedUpdateRequest = false;
if (noteId == m_textChangeNoteId && serial == m_textChangeSerial) {
m_textChange = false;
m_textChangeNoteId = -1;
}
if (noteId == m_updateRequestNoteId && serial == m_updateRequestSerial) {
resetUpdateRequestState();
clearedUpdateRequest = true;
}
return clearedUpdateRequest;
}This keeps all behavior but makes it obvious that there is a single way to “end” an update request.
2. Separate “clear state” from “control flow” in clearTextChangeState
To avoid clearTextChangeState doubling as a control flag, you can split responsibilities while preserving the current external behavior.
First, make clearTextChangeState only mutate text-change-related state:
void WebRichTextManager::clearTextChangeState(int noteId, quint64 serial)
{
if (noteId == m_textChangeNoteId && serial == m_textChangeSerial) {
m_textChange = false;
m_textChangeNoteId = -1;
}
}Then add a dedicated helper for the update request:
bool WebRichTextManager::clearUpdateRequestState(int noteId, quint64 serial)
{
if (noteId == m_updateRequestNoteId && serial == m_updateRequestSerial) {
resetUpdateRequestState();
return true;
}
return false;
}Update flushNoteWithResult to explicitly decide when to emit:
void WebRichTextManager::flushNoteWithResult(VNoteItem *data, quint64 serial, const QString &result)
{
if (!data) {
return;
}
if (saveNoteWithResult(data, result)) {
clearTextChangeState(data->noteId, serial);
if (clearUpdateRequestState(data->noteId, serial)) {
emit finishedUpdateNote();
}
}
}This keeps all semantics intact but makes the control flow clearer: helpers only mutate state; the caller decides when to emit signals.
Bind editor flush requests to note id and change serial. 将编辑器 flush 请求绑定到笔记 ID 和变更序号。 Reuse flushed note content before context menu and save actions. 右键菜单和保存动作前复用已同步的笔记内容。 Log: 修复新建笔记快速右键保存菜单置灰问题 PMS: BUG-366545 Influence: 新建或编辑笔记后立即右键、快捷键弹出菜单或保存时，会先同步编辑器最新内容，避免保存笔记菜单误置灰或导出旧内容。
dengzhongyuan365-dev
force-pushed
the
bug-fix-6-17
branch
from
c172a5e to
7f5723b
Compare
|
Note
详情{
"src/common/VNoteMainManager.cpp": [
{
"line": " url = \"https://www.deepin.org/zh/agreement/privacy/\";",
"line_number": 1374,
"rule": "S35",
"reason": "Url link | 20e2eab189"
},
{
"line": " url = \"https://www.uniontech.com/agreement/privacy-cn\";",
"line_number": 1376,
"rule": "S35",
"reason": "Url link | 4850a00dd7"
},
{
"line": " url = \"https://www.deepin.org/en/agreement/privacy/\";",
"line_number": 1380,
"rule": "S35",
"reason": "Url link | 38d42f63bf"
},
{
"line": " url = \"https://www.uniontech.com/agreement/privacy-en\";",
"line_number": 1382,
"rule": "S35",
"reason": "Url link | f82409d3b5"
}
]
} |
deepin pr auto review★ 总体评分：82分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // src/gui/mainwindow/ItemListView.qml
function popupNoteContextMenu(noteIds, popupIndex, popupX, popupY) {
if (!noteIds || noteIds.length === 0)
return;
VNoteMainManager.checkNoteVoice(noteIds);
VNoteMainManager.checkNoteText(noteIds);
if (popupIndex >= 0 && popupIndex < itemModel.count) {
var item = itemListView.itemAtIndex(popupIndex);
if (item) {
try {
noteCtxMenu.popup(item, popupX, popupY);
return;
} catch (e) {
// 修复：增加异常日志记录，避免静默吞没错误
console.error("Failed to popup context menu at specific item:", e.message);
}
}
}
noteCtxMenu.popup();
} |
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
2 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.
Bind editor flush requests to note id and change serial.
将编辑器 flush 请求绑定到笔记 ID 和变更序号。
Reuse flushed note content before context menu and save actions.
右键菜单和保存动作前复用已同步的笔记内容。
Log: 修复新建笔记快速右键保存菜单置灰问题
PMS: BUG-366545
Influence: 新建或编辑笔记后立即右键、快捷键弹出菜单或保存时，会先同步编辑器最新内容，避免保存笔记菜单误置灰或导出旧内容。
Summary by Sourcery
Bind editor flush requests and note update operations to a specific note ID and text change serial to avoid stale saves and ensure latest content is used for actions like context menus and save.
Bug Fixes:
Enhancements: