fix: Opening a file without permission results in a blank page#451
Merged
lzwind merged 2 commits intolinuxdeepin:masterfrom Apr 23, 2026
Merged
fix: Opening a file without permission results in a blank page#451lzwind merged 2 commits intolinuxdeepin:masterfrom
lzwind merged 2 commits intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideReworks file readability checks to use an actual QFile open attempt and improves error messaging so permission issues show a clear, specific prompt instead of leaving a blank/ambiguous error tab. Sequence diagram for updated file open permission check in Window::addTabsequenceDiagram
actor User
participant Window
participant QFile
participant DMessageManager
User ->> Window: addTab(filepath, activeTab)
Window ->> Window: QFileInfo(filepath)
alt File does not exist
Window ->> Window: Skip permission check
else File exists
Window ->> QFile: open(ReadOnly)
alt Open succeeds
QFile -->> Window: success
Window ->> QFile: close()
Window ->> Window: continue with StartManager and FileLoadThread
else Open fails (e.g. no permission)
QFile -->> Window: errorString()
Window ->> DMessageManager: sendMessage(msgParent, icon, "You do not have permission to open %1")
DMessageManager -->> User: Permission warning toast/dialog
Window ->> Window: return (no tab opened)
end
end
Sequence diagram for updated permission-specific read error handling in EditWrapper::onReadAllocErrorsequenceDiagram
participant FileLoadThread
participant EditWrapper
participant QFileInfo
participant WarningNotice
participant DMessageManager
participant User
FileLoadThread ->> EditWrapper: onReadAllocError()
EditWrapper ->> EditWrapper: set m_bCanBeRestore
EditWrapper ->> EditWrapper: toggleReadOnlyMode(true)
EditWrapper ->> QFileInfo: QFileInfo(filePath())
QFileInfo -->> EditWrapper: exists(), isReadable()
alt File exists and is not readable
EditWrapper ->> WarningNotice: setMessage("You do not have permission to open %1")
else Other read error (too large, damaged, etc.)
EditWrapper ->> WarningNotice: setMessage("The file cannot be read, which may be too large or has been damaged!")
end
EditWrapper ->> WarningNotice: clearBtn()
EditWrapper ->> WarningNotice: show()
EditWrapper ->> DMessageManager: sendMessage(m_pTextEdit, WarningNotice)
DMessageManager -->> User: Display inline warning message
Updated class diagram for Window and EditWrapper permission handlingclassDiagram
class Window {
+void addTab(QString filepath, bool activeTab)
-QWidget *m_editorWidget
}
class EditWrapper {
+void onReadAllocError()
-QString filePath()
-QPlainTextEdit *m_pTextEdit
-WarningNotice *m_pWaringNotices
-bool m_bCanBeRestore
}
class DMessageManager {
+static DMessageManager* instance()
+void sendMessage(QWidget *parent, QIcon icon, QString message)
}
class WarningNotice {
+void setMessage(QString message)
+void clearBtn()
+void show()
}
Window --> DMessageManager : uses
EditWrapper --> DMessageManager : uses
EditWrapper --> WarningNotice : manages
EditWrapper --> QPlainTextEdit : editedWidget
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": 390,
"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 - I've found 2 issues, and left some high level feedback:
- Since QFileInfo::isReadable() is known to be unreliable (as noted in the new comment in Window::addTab), consider using the same QFile::open()-based permission check in EditWrapper::onReadAllocError instead of relying on isReadable() there as well.
- The permission error messages now sometimes use the full path (in Window::addTab) and sometimes only the file name (in EditWrapper::onReadAllocError); consider making this consistent so users see a uniform message format.
- The logic for checking readability (attempting to open the file and then showing a DMessageManager warning) is now duplicated in multiple places; consider extracting this into a shared helper to keep the behavior and messaging consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since QFileInfo::isReadable() is known to be unreliable (as noted in the new comment in Window::addTab), consider using the same QFile::open()-based permission check in EditWrapper::onReadAllocError instead of relying on isReadable() there as well.
- The permission error messages now sometimes use the full path (in Window::addTab) and sometimes only the file name (in EditWrapper::onReadAllocError); consider making this consistent so users see a uniform message format.
- The logic for checking readability (attempting to open the file and then showing a DMessageManager warning) is now duplicated in multiple places; consider extracting this into a shared helper to keep the behavior and messaging consistent and easier to maintain.
## Individual Comments
### Comment 1
<location path="src/widgets/window.cpp" line_range="682-687" />
<code_context>
+ // Verify the file is actually readable by attempting to open it.
+ // This is more reliable than QFileInfo::isReadable(), which can be incorrect
+ // in some edge cases (e.g. setuid processes, certain filesystems).
+ if (fileInfo.exists()) {
+ QFile testFile(filepath);
+ if (testFile.open(QIODevice::ReadOnly)) {
+ testFile.close();
+ } else {
+ qWarning() << "No permission to read file:" << filepath << "error:" << testFile.errorString();
+ QWidget *const msgParent = m_editorWidget->currentWidget() ? m_editorWidget->currentWidget() : this;
+ DMessageManager::instance()->sendMessage(msgParent, QIcon(":/images/warning.svg"),
+ QString(tr("You do not have permission to open %1")).arg(filepath));
+ return;
+ }
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Distinguish permission errors from other QFile open failures for clearer user feedback.
Right now any `testFile.open()` failure is reported as a permission error. That means unrelated issues (I/O errors, too many open files, etc.) surface the same "You do not have permission" message. Since you already log `testFile.errorString()`, please branch on `testFile.error()` and map only permission-related cases (e.g. `QFileDevice::PermissionError`) to the permission message, and use a more generic failure message for other error types.
</issue_to_address>
### Comment 2
<location path="src/editor/editwrapper.cpp" line_range="1648" />
<code_context>
}
- m_pWaringNotices->setMessage(tr("The file cannot be read, which may be too large or has been damaged!"));
+ const QFileInfo fileInfo(filePath());
+ if (fileInfo.exists() && !fileInfo.isReadable()) {
+ m_pWaringNotices->setMessage(tr("You do not have permission to open %1").arg(fileInfo.fileName()));
+ } else {
+ m_pWaringNotices->setMessage(tr("The file cannot be read, which may be too large or has been damaged!"));
+ }
m_pWaringNotices->clearBtn();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Permission detection here uses `QFileInfo::isReadable`, which may diverge from the more reliable check in `Window::addTab`.
In `Window::addTab`, readability is determined by actually opening the file with `QFile::open`, specifically because `QFileInfo::isReadable()` can be wrong in some edge cases. This branch reintroduces `fileInfo.isReadable()` for message selection, which can cause it to disagree with the logic that triggered `onReadAllocError()`, leading to inconsistent behavior and confusing messages.
Consider basing this branch on the same error information that caused `onReadAllocError()` (e.g., an error type passed into this handler), or at least reuse the `QFile::open`-based result so the permission message is consistent with the earlier check.
Suggested implementation:
```cpp
if (permissionDenied) {
const QFileInfo fileInfo(filePath());
m_pWaringNotices->setMessage(tr("You do not have permission to open %1").arg(fileInfo.fileName()));
} else {
m_pWaringNotices->setMessage(tr("The file cannot be read, which may be too large or has been damaged!"));
}
```
To fully implement this:
1. Update the signature of the function containing this block (likely `EditWrapper::onReadAllocError`) to accept a `bool permissionDenied` parameter (or a richer error enum, e.g. `FileOpenError error`, from which you derive `permissionDenied` inside this function).
2. At the call site in `Window::addTab` (or wherever this handler is invoked), determine `permissionDenied` based on the `QFile::open` result and `QFile::error()` (e.g. `file.error() == QFileDevice::PermissionError`) and pass that into this handler.
3. Update all other callers of this handler in the codebase to pass an appropriate `permissionDenied` value (likely `false` for non-permission-related errors).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| m_pWaringNotices->setMessage(tr("The file cannot be read, which may be too large or has been damaged!")); | ||
| const QFileInfo fileInfo(filePath()); |
There was a problem hiding this comment.
suggestion (bug_risk): Permission detection here uses QFileInfo::isReadable, which may diverge from the more reliable check in Window::addTab.
In Window::addTab, readability is determined by actually opening the file with QFile::open, specifically because QFileInfo::isReadable() can be wrong in some edge cases. This branch reintroduces fileInfo.isReadable() for message selection, which can cause it to disagree with the logic that triggered onReadAllocError(), leading to inconsistent behavior and confusing messages.
Consider basing this branch on the same error information that caused onReadAllocError() (e.g., an error type passed into this handler), or at least reuse the QFile::open-based result so the permission message is consistent with the earlier check.
Suggested implementation:
if (permissionDenied) {
const QFileInfo fileInfo(filePath());
m_pWaringNotices->setMessage(tr("You do not have permission to open %1").arg(fileInfo.fileName()));
} else {
m_pWaringNotices->setMessage(tr("The file cannot be read, which may be too large or has been damaged!"));
}
To fully implement this:
- Update the signature of the function containing this block (likely
EditWrapper::onReadAllocError) to accept abool permissionDeniedparameter (or a richer error enum, e.g.FileOpenError error, from which you derivepermissionDeniedinside this function). - At the call site in
Window::addTab(or wherever this handler is invoked), determinepermissionDeniedbased on theQFile::openresult andQFile::error()(e.g.file.error() == QFileDevice::PermissionError) and pass that into this handler. - Update all other callers of this handler in the codebase to pass an appropriate
permissionDeniedvalue (likelyfalsefor non-permission-related errors).
|
Note
详情{
"src/widgets/window.cpp": [
{
"line": " QString key = \"base/enable\";",
"line_number": 390,
"rule": "S106",
"reason": "Var naming | 64f28539d9"
}
]
} |
log: QFileInfo::isReadable() is unreliable in certain scenarios, causing files without permission to pass the permission check and create a tab, but then failing to open via FileLoadThread , leaving a blank tab and displaying an error message. Solution: Use QFile::open() to actually attempt to open the file for permission verification, and fix onReadAllocError() to display the correct prompt when a permission error occurs. pms: bug-339655
|
Note
详情{
"src/widgets/window.cpp": [
{
"line": " QString key = \"base/enable\";",
"line_number": 390,
"rule": "S106",
"reason": "Var naming | 64f28539d9"
}
]
} |
lzwind
approved these changes
Apr 23, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JWWTSL, lzwind 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 |
|
CLA Assistant Lite bot: You can retrigger this bot by commenting recheck in this Pull Request |
deepin pr auto review这段代码的修改主要是为了改进文件可读性检查的可靠性。以下是对这段代码的详细审查和改进意见： 1. 语法逻辑审查优点：
潜在问题：
2. 代码质量审查优点：
改进建议：
3. 代码性能审查
4. 代码安全审查
5. 综合改进建议以下是改进后的代码，主要修复了错误处理不完整的问题： // Verify the file is actually readable by attempting to open it.
// This is more reliable than QFileInfo::isReadable(), which can be incorrect
// in some edge cases (e.g. setuid processes, certain filesystems).
if (fileInfo.exists()) {
QFile testFile(filepath);
if (!testFile.open(QIODevice::ReadOnly)) {
if (testFile.error() == QFileDevice::PermissionsError) {
qWarning() << "No permission to read file:" << filepath << "error:" << testFile.errorString();
} else {
qWarning() << "Failed to open file:" << filepath << "error:" << testFile.errorString();
}
QWidget *const msgParent = m_editorWidget->currentWidget() ? m_editorWidget->currentWidget() : this;
DMessageManager::instance()->sendMessage(msgParent, QIcon(":/images/warning.svg"),
QString(tr("You do not have permission to open %1")).arg(filepath));
return;
}
testFile.close();
}改进点：
6. 其他建议
总的来说，这段代码的修改方向是正确的，主要需要改进的是错误处理的完整性。 |
|
Note
详情{
"src/widgets/window.cpp": [
{
"line": " QString key = \"base/enable\";",
"line_number": 389,
"rule": "S106",
"reason": "Var naming | 64f28539d9"
}
]
} |
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
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: QFileInfo::isReadable() is unreliable in certain scenarios, causing files without permission to pass the permission check and create a tab, but then failing to open via FileLoadThread
, leaving a blank tab and displaying an error message. Solution: Use QFile::open() to actually attempt to open the file for permission verification, and fix onReadAllocError() to display the correct prompt when a permission error occurs.
pms: bug-339655
Summary by Sourcery
Improve file readability checks to prevent opening tabs for files without read permissions and show accurate error messages when reads fail.
Bug Fixes: