chore: enhance logging and error handling in file manager components #2930
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 GuideThis PR systematically enhances logging and error handling across the file manager titlebar plugin by inserting detailed log statements and guard clauses to validate inputs, pointers, and states, improving traceability, consistency, and robustness. Sequence diagram for enhanced error handling in password change flowsequenceDiagram
actor User
participant DPCConfirmWidget
participant AccessControlInter
participant FileUtils
participant DaemonServiceIFace
User->>DPCConfirmWidget: Clicks Save (onSaveBtnClicked)
DPCConfirmWidget->>DPCConfirmWidget: Validate oldPwdEdit, newPwdEdit, repeatPwdEdit (log if empty)
alt Validation fails
DPCConfirmWidget-->>User: Show error tooltip (log reason)
else Validation passes
DPCConfirmWidget->>DPCConfirmWidget: checkNewPassword(), checkRepeatPassword() (log failures)
alt Password validation fails
DPCConfirmWidget-->>User: Show error tooltip (log reason)
else Password validation passes
DPCConfirmWidget->>AccessControlInter: isValid()
alt AccessControlInter invalid
DPCConfirmWidget-->>User: Show critical log, abort
else AccessControlInter valid
DPCConfirmWidget->>FileUtils: encryptString(oldPwd)
DPCConfirmWidget->>FileUtils: encryptString(newPwd)
DPCConfirmWidget->>AccessControlInter: asyncCall(changePwd, oldPassEnc, newPassEnc)
AccessControlInter-->>DPCConfirmWidget: onPasswordChecked(result)
alt Success
DPCConfirmWidget-->>User: Emit sigConfirmed (log success)
else Failure
DPCConfirmWidget-->>User: Show warning log, show error tooltip
end
end
end
end
Sequence diagram for improved error handling in server connection dialogsequenceDiagram
actor User
participant ConnectToServerDialog
participant DialogManagerInstance
participant QUrl
participant Model
User->>ConnectToServerDialog: Clicks Connect
ConnectToServerDialog->>ConnectToServerDialog: Check serverComboBox text (log if empty)
alt Empty server address
ConnectToServerDialog-->>User: Close dialog, log warning
else Not empty
ConnectToServerDialog->>ConnectToServerDialog: getCurrentUrlString()
ConnectToServerDialog->>QUrl: fromUserInput(url)
ConnectToServerDialog->>ConnectToServerDialog: updateCollections(url, false)
alt Invalid URL (host empty)
ConnectToServerDialog->>DialogManagerInstance: showErrorDialog
ConnectToServerDialog-->>User: Log warning
else Valid URL
ConnectToServerDialog-->>User: Proceed with connection (log info)
end
end
Class diagram for enhanced logging and error handling in OptionButtonBox and related classesclassDiagram
class OptionButtonBox {
+setViewOptionsButton(button)
+setListViewButton(button)
+setIconViewButton(button)
+updateOptionButtonBox(parentWidth)
+onUrlChanged(url)
}
class OptionButtonBoxPrivate {
+updateCompactButton()
+setViewMode(mode)
+loadViewMode(url)
+switchMode(mode)
+initializeUi()
}
OptionButtonBox --> OptionButtonBoxPrivate
OptionButtonBoxPrivate --> ViewOptionsButton
OptionButtonBoxPrivate --> DToolButton
OptionButtonBoxPrivate --> DConfigManager
OptionButtonBoxPrivate --> Application
OptionButtonBoxPrivate --> TitleBarEventCaller
OptionButtonBoxPrivate --> QUrl
OptionButtonBoxPrivate : +currentMode
OptionButtonBoxPrivate : +compactButton
OptionButtonBoxPrivate : +iconViewButton
OptionButtonBoxPrivate : +listViewButton
OptionButtonBoxPrivate : +treeViewButton
OptionButtonBoxPrivate : +viewOptionsButton
OptionButtonBoxPrivate : +currentUrl
OptionButtonBoxPrivate : +hBoxLayout
OptionButtonBoxPrivate : +listViewEnabled
OptionButtonBoxPrivate : +iconViewEnabled
OptionButtonBoxPrivate : +treeViewEnabled
OptionButtonBoxPrivate : +isCompactMode
OptionButtonBoxPrivate : +buttonGroup
OptionButtonBoxPrivate : +switchMode(mode)
OptionButtonBoxPrivate : +loadViewMode(url)
OptionButtonBoxPrivate : +updateCompactButton()
OptionButtonBoxPrivate : +setViewMode(mode)
OptionButtonBoxPrivate : +initializeUi()
Class diagram for enhanced logging in DPCConfirmWidget (password dialog)classDiagram
class DPCConfirmWidget {
+initLibrary()
+checkRepeatPassword()
+checkNewPassword()
+checkPasswdComplexity(pwd, msg)
+onEditingFinished()
+onSaveBtnClicked()
+onPasswordChecked(result)
}
DPCConfirmWidget --> DeepinPwdCheck
DPCConfirmWidget --> AccessControlInter
DPCConfirmWidget --> FileUtils
DPCConfirmWidget : +oldPwdEdit
DPCConfirmWidget : +newPwdEdit
DPCConfirmWidget : +repeatPwdEdit
DPCConfirmWidget : +deepinPwCheck
DPCConfirmWidget : +getPasswdLevel
DPCConfirmWidget : +errToString
DPCConfirmWidget : +accessControlInter
DPCConfirmWidget : +showToolTips(msg, widget)
DPCConfirmWidget : +setEnabled(bool)
DPCConfirmWidget : +sigConfirmed()
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 @Kakueeen - I've reviewed your changes - here's some feedback:
- Consider consolidating the many repetitive null-pointer checks and fmWarning/fmDebug calls into helper functions or macros to reduce boilerplate and improve maintainability.
- Standardize log message format (capitalization, punctuation, including context identifiers) and ensure consistent use of fmInfo/fmWarning/fmCritical levels to avoid confusion and log noise.
- Audit the overall log verbosity—too many debug statements in hot code paths can flood logs and impact performance; consider dialing back or grouping lower-priority messages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider consolidating the many repetitive null-pointer checks and fmWarning/fmDebug calls into helper functions or macros to reduce boilerplate and improve maintainability.
- Standardize log message format (capitalization, punctuation, including context identifiers) and ensure consistent use of fmInfo/fmWarning/fmCritical levels to avoid confusion and log noise.
- Audit the overall log verbosity—too many debug statements in hot code paths can flood logs and impact performance; consider dialing back or grouping lower-priority messages.
## Individual Comments
### Comment 1
<location> `src/plugins/filemanager/dfmplugin-titlebar/views/optionbuttonbox.cpp:44` </location>
<code_context>
{
- if (!compactButton)
+ if (!compactButton) {
+ fmWarning() << "Compact button is null, cannot update";
return;
+ }
</code_context>
<issue_to_address>
Consider using a more specific log level for non-critical UI state.
'Warning' may be too severe here; use 'Debug' or 'Info' if the missing element is expected in some cases to avoid unnecessary log noise.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (!compactButton) {
fmWarning() << "Compact button is null, cannot update";
return;
}
=======
if (!compactButton) {
fmDebug() << "Compact button is null, cannot update";
return;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/plugins/filemanager/dfmplugin-titlebar/views/optionbuttonbox.cpp:60` </location>
<code_context>
compactButton->setIcon(QIcon::fromTheme("dfm_viewlist_tree"));
break;
default:
+ fmWarning() << "Unknown view mode for compact button:" << int(currentMode);
break;
}
</code_context>
<issue_to_address>
Unknown view mode should be handled more explicitly.
Consider adding an assertion or fallback to handle unknown view modes and avoid silent failures.
</issue_to_address>
### Comment 3
<location> `src/plugins/filemanager/dfmplugin-titlebar/views/crumbbar.cpp:436` </location>
<code_context>
if (!d->crumbController) {
- fmWarning("No controller found when trying to call DFMCrumbBar::updateCrumbs() !!!");
- fmDebug() << "updateCrumbs (no controller) : " << url;
+ fmCritical() << "Cannot update crumbs: no controller available for URL" << url.toString();
return;
}
</code_context>
<issue_to_address>
Critical log for missing crumb controller may be too severe.
If this is expected in normal scenarios, consider lowering the log level or handling it more gracefully.
</issue_to_address>
### Comment 4
<location> `src/plugins/filemanager/dfmplugin-titlebar/views/viewoptionsbutton.cpp:71` </location>
<code_context>
DToolButton::setVisible(visible);
- if (!DConfigManager::instance()->value(kViewDConfName, kDisplayPreviewVisibleKey).toBool())
+ if (!DConfigManager::instance()->value(kViewDConfName, kDisplayPreviewVisibleKey).toBool()) {
+ fmDebug() << "Display preview is disabled in config, skipping preview visibility change";
return;
+ }
</code_context>
<issue_to_address>
Debug log may be unnecessary for expected configuration states.
If this branch is common, consider removing or reducing the log level to prevent excessive debug output.
</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
+43
to
+46
| if (!compactButton) { | ||
| fmWarning() << "Compact button is null, cannot update"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
suggestion: Consider using a more specific log level for non-critical UI state.
'Warning' may be too severe here; use 'Debug' or 'Info' if the missing element is expected in some cases to avoid unnecessary log noise.
Suggested change
| if (!compactButton) { | |
| fmWarning() << "Compact button is null, cannot update"; | |
| return; | |
| } | |
| if (!compactButton) { | |
| fmDebug() << "Compact button is null, cannot update"; | |
| return; | |
| } |
| @@ -55,14 +57,15 @@ void OptionButtonBoxPrivate::updateCompactButton() | |||
| compactButton->setIcon(QIcon::fromTheme("dfm_viewlist_tree")); | |||
| break; | |||
| default: | |||
| fmWarning() << "Unknown view mode for compact button:" << int(currentMode); | |||
There was a problem hiding this comment.
suggestion: Unknown view mode should be handled more explicitly.
Consider adding an assertion or fallback to handle unknown view modes and avoid silent failures.
| @@ -433,8 +433,7 @@ void CrumbBar::onHideAddrAndUpdateCrumbs(const QUrl &url) | |||
| if (updataFlag) | |||
| update(); | |||
| if (!d->crumbController) { | |||
| fmWarning("No controller found when trying to call DFMCrumbBar::updateCrumbs() !!!"); | |||
| fmDebug() << "updateCrumbs (no controller) : " << url; | |||
| fmCritical() << "Cannot update crumbs: no controller available for URL" << url.toString(); | |||
There was a problem hiding this comment.
suggestion: Critical log for missing crumb controller may be too severe.
If this is expected in normal scenarios, consider lowering the log level or handling it more gracefully.
| @@ -67,8 +67,10 @@ void ViewOptionsButton::switchMode(ViewMode mode, const QUrl &url) | |||
| void ViewOptionsButton::setVisible(bool visible) | |||
| { | |||
| DToolButton::setVisible(visible); | |||
| if (!DConfigManager::instance()->value(kViewDConfName, kDisplayPreviewVisibleKey).toBool()) | |||
| if (!DConfigManager::instance()->value(kViewDConfName, kDisplayPreviewVisibleKey).toBool()) { | |||
| fmDebug() << "Display preview is disabled in config, skipping preview visibility change"; | |||
There was a problem hiding this comment.
nitpick: Debug log may be unnecessary for expected configuration states.
If this branch is common, consider removing or reducing the log level to prevent excessive debug output.
- Improved logging statements across various modules, including connection dialogs and event handling, to provide clearer insights during operations. - Added checks for empty server addresses, invalid URLs, and null pointers to enhance error handling and user feedback. - Streamlined logging practices to ensure consistency and better maintainability throughout the file manager codebase. Log: This commit enhances the logging and error handling mechanisms in the file manager, improving overall visibility and robustness during operations.
- Improved logging statements to provide clearer insights during initialization, item insertion, and configuration changes. - Added checks for unsupported entries, empty paths, and null pointers to enhance error handling and user feedback. - Streamlined logging practices across various modules to ensure consistency and better maintainability. Log: This commit enhances the logging and error handling mechanisms in the ProtocolDeviceDisplayManager and related components, improving overall visibility and robustness during operations.
- Improved logging statements to provide clearer insights during sidebar operations, including item addition, removal, and updates. - Added checks for null items, invalid URLs, and other edge cases to enhance error handling and user feedback. - Streamlined logging practices across sidebar-related modules to ensure consistency and better maintainability. Log: This commit enhances the logging and error handling mechanisms in the sidebar components, improving overall visibility and robustness during operations.
Johnson-zs
approved these changes
Jun 20, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs, Kakueeen The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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: This commit enhances the logging and error handling mechanisms in the file manager, improving overall visibility and robustness during operations.
Summary by Sourcery
Enhance logging and error handling throughout the file manager title bar and related components to improve observability and prevent invalid operations.
Enhancements: