Decouple the app indicator from the Application class#711
Merged
Conversation
Reviewer's GuideRefactors IptuxAppIndicator to decouple it from Application by passing a GActionGroup and exposing a signal for main-window activation, while moving unread-count wiring and activation logic into Application and updating the dummy implementation accordingly. Sequence diagram for main window activation via app indicator scrollsequenceDiagram
actor User
participant IptuxAppIndicatorPrivate
participant IptuxAppIndicator
participant Application
participant GActionGroup
User->>IptuxAppIndicatorPrivate: scroll_event
IptuxAppIndicatorPrivate->>IptuxAppIndicator: onScrollEvent triggers sigActivateMainWindow.emit()
IptuxAppIndicator->>Application: sigActivateMainWindow callback
Application->>GActionGroup: g_action_group_activate_action open_main_window
Sequence diagram for startup wiring of app indicator and unread countsequenceDiagram
participant Application
participant GActionGroup
participant UiCoreThread
participant IptuxAppIndicator
Application->>GActionGroup: obtain from self.app
Application->>IptuxAppIndicator: constructor(action_group)
Application->>IptuxAppIndicator: connect sigActivateMainWindow handler
Application->>UiCoreThread: connect sigUnreadMsgCountUpdated to IptuxAppIndicator.SetUnreadCount
Updated class diagram for IptuxAppIndicator decoupled from ApplicationclassDiagram
class Application {
+onStartup(self)
-bool enable_app_indicator_
-shared_ptr~IptuxAppIndicator~ app_indicator
-GApplication* app
-UiCoreThread* cthrd
}
class IptuxAppIndicator {
+IptuxAppIndicator(GActionGroup* action_group)
+void SetUnreadCount(int count)
+sigc::signal~void~ sigActivateMainWindow
-shared_ptr~IptuxAppIndicatorPrivate~ priv
}
class IptuxAppIndicatorPrivate {
+IptuxAppIndicatorPrivate(IptuxAppIndicator* owner)
+~IptuxAppIndicatorPrivate()
+static void onScrollEvent(IptuxAppIndicatorPrivate* self)
-IptuxAppIndicator* owner
-AppIndicator* indicator
-GtkBuilder* menuBuilder
}
class IptuxAppIndicatorDummy {
+IptuxAppIndicator(GActionGroup* action_group)
+void SetUnreadCount(int count)
}
class UiCoreThread {
+sigUnreadMsgCountUpdated
}
Application *-- IptuxAppIndicator : owns
Application --> GActionGroup : uses
Application --> UiCoreThread : uses
IptuxAppIndicator *-- IptuxAppIndicatorPrivate : owns
IptuxAppIndicator --> GActionGroup : uses
UiCoreThread --> IptuxAppIndicator : updates_unread_count
IptuxAppIndicatorDummy ..|> IptuxAppIndicator : dummy_implementation
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
- Coverage 52.01% 51.99% -0.03%
==========================================
Files 64 64
Lines 8599 8601 +2
==========================================
- Hits 4473 4472 -1
- Misses 4126 4129 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 left some high level feedback:
- In
Application::onStartup, consider avoiding capturingselfby reference in the lambda connected tosigActivateMainWindowand instead capture only the neededGActionGroup*(orself.app) by value to make the signal connection more robust against future changes inApplication’s lifetime management. - The
IptuxAppIndicatorPrivatestores a rawIptuxAppIndicator* owner; if the ownership model ever changes, this could become a dangling pointer, so it may be safer to clarify or enforce the lifetime relationship (e.g., with comments or by using a safer pattern like having the outer class explicitly disconnect signals before destruction).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Application::onStartup`, consider avoiding capturing `self` by reference in the lambda connected to `sigActivateMainWindow` and instead capture only the needed `GActionGroup*` (or `self.app`) by value to make the signal connection more robust against future changes in `Application`’s lifetime management.
- The `IptuxAppIndicatorPrivate` stores a raw `IptuxAppIndicator* owner`; if the ownership model ever changes, this could become a dangling pointer, so it may be safer to clarify or enforce the lifetime relationship (e.g., with comments or by using a safer pattern like having the outer class explicitly disconnect signals before destruction).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the IptuxAppIndicator class to remove its direct dependency on the Application class, improving modularity and separation of concerns. The app indicator now accepts a generic GActionGroup* instead of an Application* pointer and communicates back to the application through a signal rather than direct method calls.
Changes:
- Modified
IptuxAppIndicatorto accept aGActionGroup*in its constructor instead of anApplication*pointer - Added a
sigActivateMainWindowsignal toIptuxAppIndicatorfor communicating activation events back to the application - Updated
Application::onStartupto wire up the app indicator to the application's action group and connect the unread message count signal externally
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/iptux/Application.cpp | Updated to instantiate app indicator with GActionGroup, connect activation signal to action, and wire up unread count updates |
| src/iptux/AppIndicator.h | Changed constructor to accept GActionGroup* instead of Application*, added sigActivateMainWindow signal, removed Application dependency |
| src/iptux/AppIndicator.cpp | Modified constructor and event handler to use owner pointer and emit signal instead of directly calling application methods |
| src/iptux/AppIndicatorDummy.cpp | Updated dummy implementation to match new constructor signature and added SetUnreadCount stub |
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.
Summary by Sourcery
Decouple the app indicator from the Application class by routing actions and signals through a generic GActionGroup and a new activation signal.
Enhancements: