-
Notifications
You must be signed in to change notification settings - Fork 130
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
check exist preference dialog before open a new one (fix #644) #645
Conversation
Reviewer's Guide by SourceryThis pull request fixes a bug where the preference dialog could be opened multiple times. It modifies the Sequence diagram for preference dialog handlingsequenceDiagram
participant User as User
participant App as Application
participant DS as DataSettings
User->>DS: Open Preferences
DS->>App: getPreferenceDialog()
alt Dialog already exists
App-->>DS: Return existing dialog
DS->>DS: Present existing dialog
else No existing dialog
App-->>DS: Return null
DS->>DS: Create new dialog
DS->>App: setPreferenceDialog(dialog)
DS->>DS: Show dialog
Note over DS: User interacts with dialog
DS->>App: setPreferenceDialog(null)
end
Class diagram showing Application and DataSettings changesclassDiagram
class Application {
-GtkWidget* preference_dialog_
+GtkWidget* getPreferenceDialog()
+void setPreferenceDialog(GtkWidget* dialog)
}
class DataSettings {
+void ResetDataEntry(Application* app, GtkWidget* parent)
}
DataSettings ..> Application: uses
State diagram for preference dialog lifecyclestateDiagram-v2
[*] --> Closed: Initial State
Closed --> Open: Open Dialog
Open --> Open: Try to Open Again
note right of Open: Present existing dialog
Open --> Closed: Close Dialog
Closed --> [*]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @lidaobing - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a more descriptive commit message that explains the fix (preventing multiple preference dialogs) rather than just referencing the issue number.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -98,6 +104,7 @@ void DataSettings::ResetDataEntry(Application* app, GtkWidget* parent) { | |||
break; | |||
} | |||
} | |||
app->setPreferenceDialog(NULL); |
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.
suggestion: Consider connecting to the dialog's 'destroy' signal to handle cases where the dialog is closed externally
This would ensure the preference_dialog_ pointer is properly nulled even if the dialog is destroyed by the window manager or through other means.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #645 +/- ##
==========================================
- Coverage 52.51% 52.47% -0.05%
==========================================
Files 64 64
Lines 8550 8557 +7
==========================================
Hits 4490 4490
- Misses 4060 4067 +7 ☔ View full report in Codecov by Sentry. |
/close #644
Summary by Sourcery
Bug Fixes: