Conversation
Updated conditionals for readability and bumped a version to 6.8 in setup.py.
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors window lifecycle handling by replacing widget hiding with destruction in update-related flows, simplifies boolean conditionals, slightly cleans up helper logic, and bumps the application version and a small setup utility loop. Sequence diagram for updated window lifecycle during update startsequenceDiagram
actor User
participant FrontendWindow
participant GtkWindow as GtkWindowInstance
participant Data
participant UpdateProcess
User ->> FrontendWindow: click_start_update(widget)
FrontendWindow ->> Data: set update_started = True
FrontendWindow ->> UpdateProcess: InstallUpdate()
Note over UpdateProcess: Update process runs
FrontendWindow ->> GtkWindow: destroy()
Note over GtkWindow: Window is destroyed instead of hidden
Sequence diagram for updated delete event behaviorsequenceDiagram
actor User
participant FrontendWindow
participant GtkWindow as GtkWindowInstance
participant Data
participant UpdateCore
User ->> FrontendWindow: delete_event(widget)
FrontendWindow ->> Data: check close_session
alt close_session is True
FrontendWindow ->> UpdateCore: updating()
alt updating is True
FrontendWindow ->> UpdateCore: unlock_update_station()
end
FrontendWindow ->> GtkWindow: Gtk.main_quit()
else close_session is False
FrontendWindow ->> GtkWindow: destroy()
FrontendWindow ->> Data: check update_started
alt update_started is False
FrontendWindow ->> Data: set stop_pkg_refreshing = False
FrontendWindow ->> UpdateCore: updating()
alt updating is True
FrontendWindow ->> UpdateCore: unlock_update_station()
end
end
end
Sequence diagram for updated notification click behaviorsequenceDiagram
actor User
participant Notification
participant Data
User ->> Notification: on_clicked(widget)
Notification ->> Data: read major_upgrade_state
alt major_upgrade
Notification ->> Data: set major_upgrade = False
Notification ->> Data: set do_not_upgrade = False
else not major_upgrade
Notification ->> Data: set major_upgrade = False
Notification ->> Data: set do_not_upgrade = True
end
Notification ->> Notification: destroy()
Note over Notification: Notification is destroyed instead of hidden
Class diagram for updated frontend and notification lifecycle methodsclassDiagram
class Data {
<<static>> bool close_session
<<static>> bool update_started
<<static>> bool stop_pkg_refreshing
<<static>> bool major_upgrade
<<static>> bool do_not_upgrade
}
class FrontendWindow {
GtkWindow window
delete_event(widget: GtkWidget) void
start_update(widget: GtkWidget) void
if_backup(widget: GtkWidget) void
read_output(progress: object) void
stop_tread(start_window: object) void
}
class Notification {
on_clicked(widget: GtkWidget) void
}
class UpdateCore {
updating() bool
unlock_update_station() void
InstallUpdate() void
}
FrontendWindow --> Data : uses
FrontendWindow --> UpdateCore : uses
Notification --> Data : uses
Notification --> FrontendWindow : lifecycle_related
FrontendWindow ..> GtkWindow : destroys
Notification ..> Notification : destroys_self
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- After replacing
hide()withdestroy()onself.window,self.win, and the notification widget, double-check that no later code assumes these widgets still exist or reuses them; if they are accessed again you may want to add guards or recreate them instead of destroying. - In
delete_event, both branches callunlock_update_station()whenupdating()is true; consider centralizing this into a single, post-branch call to avoid duplicated logic and keep the cleanup behavior consistent. - In
data_file_list,root.replace(source_base, install_base)can behave unexpectedly ifsource_baseappears multiple times in the path; usingos.path.join(install_base, os.path.relpath(root, source_base))would make the path transformation more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- After replacing `hide()` with `destroy()` on `self.window`, `self.win`, and the notification widget, double-check that no later code assumes these widgets still exist or reuses them; if they are accessed again you may want to add guards or recreate them instead of destroying.
- In `delete_event`, both branches call `unlock_update_station()` when `updating()` is true; consider centralizing this into a single, post-branch call to avoid duplicated logic and keep the cleanup behavior consistent.
- In `data_file_list`, `root.replace(source_base, install_base)` can behave unexpectedly if `source_base` appears multiple times in the path; using `os.path.join(install_base, os.path.relpath(root, source_base))` would make the path transformation more robust.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
Updated conditionals for readability and bumped a version to 6.8 in setup.py.
Summary by Sourcery
Replace window hiding with destruction and update supporting code and metadata accordingly.
Bug Fixes:
Enhancements:
Build: