support show/hide status icon #713
Conversation
Reviewer's GuideAdds configurable status icon behavior (none/normal/blinking) across the UI, core configuration, and app indicator implementations, including persistence and runtime updates when settings change. Sequence diagram for applying status icon mode on startup and config changesequenceDiagram
actor User
participant DataSettings
participant ProgramData
participant Application
participant IptuxAppIndicator as AppIndicator
rect rgb(230,230,250)
Application->>ProgramData: ReadProgData()
Note right of ProgramData: status_icon_mode_ loaded
Application->>IptuxAppIndicator: IptuxAppIndicator(action_group)
Application->>IptuxAppIndicator: SetMode(StatusIconMode(ProgramData.statusIconMode()))
IptuxAppIndicator->>IptuxAppIndicator: priv.mode = mode
alt mode == STATUS_ICON_MODE_NONE
IptuxAppIndicator->>IptuxAppIndicator: hide or deactivate status icon
else mode != STATUS_ICON_MODE_NONE
IptuxAppIndicator->>IptuxAppIndicator: SetUnreadCount(priv.unreadCount)
end
end
rect rgb(220,255,220)
User->>DataSettings: Open system settings
DataSettings->>ProgramData: SetSystemValue() uses statusIconMode()
User->>DataSettings: Change status icon mode
User->>DataSettings: Confirm settings
DataSettings->>ProgramData: setStatusIconMode(new_value)
DataSettings->>ProgramData: ObtainSystemValue(false)
ProgramData->>ProgramData: WriteProgData() persists status_icon_mode_
Application->>Application: onConfigChanged()
Application->>ProgramData: statusIconMode()
Application->>IptuxAppIndicator: SetMode(StatusIconMode(ProgramData.statusIconMode()))
IptuxAppIndicator->>IptuxAppIndicator: update visibility and attention based on mode
end
Class diagram for status icon mode integrationclassDiagram
class StatusIconMode {
<<enumeration>>
STATUS_ICON_MODE_NONE
STATUS_ICON_MODE_NORMAL
STATUS_ICON_MODE_BLINKING
}
class ProgramData {
+bool proof_shared
+bool hide_taskbar_when_main_window_iconified_
+bool need_restart_
+uint8_t status_icon_mode_ : 2
+void WriteProgData()
+void ReadProgData()
+bool isHideTaskbarWhenMainWindowIconified() const
+int statusIconMode() const
+void setStatusIconMode(int value)
}
class DataSettings {
+GtkWidget* CreateSystem()
+void SetSystemValue()
+string ObtainSystemValue(bool dryrun)
-GtkWidget* status_icon_mode_combo_widget
}
class Application {
+shared_ptr~IptuxAppIndicator~ app_indicator
+shared_ptr~ProgramData~ data
+bool enable_app_indicator_
+static void onStartup(Application self)
+void onConfigChanged()
}
class IptuxAppIndicator {
+IptuxAppIndicator(GActionGroup* action_group)
+void SetUnreadCount(int count)
+void SetMode(StatusIconMode mode)
+sigc::signal~void~ sigActivateMainWindow
-IptuxAppIndicatorPrivate* priv
}
class IptuxAppIndicatorPrivate {
+IptuxAppIndicator* owner
+void* indicator
+GtkBuilder* menuBuilder
+StatusIconMode mode
+int unreadCount
+static void onScrollEvent(IptuxAppIndicatorPrivate* self)
}
StatusIconMode <.. ProgramData : uses
StatusIconMode <.. IptuxAppIndicator : uses
ProgramData o-- StatusIconMode : status_icon_mode_
Application --> ProgramData : reads_statusIconMode
Application --> IptuxAppIndicator : configures_mode
DataSettings --> ProgramData : get_set_statusIconMode
IptuxAppIndicator *-- IptuxAppIndicatorPrivate : composition
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 - I've found 2 issues, and left some high level feedback:
- When reading the selected status icon mode (
std::stoi(gtk_combo_box_get_active_id(...))), handle the case wheregtk_combo_box_get_active_idreturnsnullptror a non-integer string to avoid potential exceptions/crashes and fall back to a sensible default. - Consider changing
ProgramData::statusIconMode()andsetStatusIconMode()to use theStatusIconModeenum instead ofint, and keeping the internal storage typed accordingly, to make invalid values harder to introduce and better reflect intent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When reading the selected status icon mode (`std::stoi(gtk_combo_box_get_active_id(...))`), handle the case where `gtk_combo_box_get_active_id` returns `nullptr` or a non-integer string to avoid potential exceptions/crashes and fall back to a sensible default.
- Consider changing `ProgramData::statusIconMode()` and `setStatusIconMode()` to use the `StatusIconMode` enum instead of `int`, and keeping the internal storage typed accordingly, to make invalid values harder to introduce and better reflect intent.
## Individual Comments
### Comment 1
<location> `src/iptux/DataSettings.cpp:1018-1019` </location>
<code_context>
+#if HAVE_STATUS_ICON
+ widget = GTK_WIDGET(
+ g_datalist_get_data(&widset, "status-icon-mode-combo-widget"));
+ progdt->setStatusIconMode(
+ std::stoi(gtk_combo_box_get_active_id(GTK_COMBO_BOX(widget))));
#endif
return oss.str();
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against null or non-numeric IDs from `gtk_combo_box_get_active_id` before calling `std::stoi`.
`gtk_combo_box_get_active_id` may return `NULL` or a non‑numeric string, causing `std::stoi` to crash or throw. Please check for `nullptr` and validate the ID before converting (e.g., digit check + `std::from_chars` or `std::stoi` in a small try/catch), and fall back to a safe default mode if parsing fails.
</issue_to_address>
### Comment 2
<location> `src/api/iptux-core/ProgramData.h:114` </location>
<code_context>
uint8_t proof_shared : 1;
uint8_t hide_taskbar_when_main_window_iconified_ : 1;
uint8_t need_restart_ : 1;
+ uint8_t status_icon_mode_ : 2;
private:
</code_context>
<issue_to_address>
**suggestion:** Storing `StatusIconMode` in a 2‑bit field is fragile if more modes are added or if config values are out of range.
With only 2 bits, any value >3 (future enum values or bad config data) will be silently truncated when stored. To avoid mangling unexpected values, either clearly document that `StatusIconMode` is limited to 0–3, or store it as a full `uint8_t` and clamp on read/write (e.g., in `ReadProgData` / `setStatusIconMode`).
Suggested implementation:
```c
uint8_t proof_shared : 1;
uint8_t hide_taskbar_when_main_window_iconified_ : 1;
uint8_t need_restart_ : 1;
// store StatusIconMode as a full byte; value is validated/clamped in setter / config loading
uint8_t status_icon_mode_;
```
To fully implement the behavior described in your review comment, the following updates are also needed in the corresponding `.cpp` file(s):
1. In the implementation of `ProgramData::setStatusIconMode(int value)`:
- Clamp or validate the incoming value before assigning to `status_icon_mode_`, e.g.:
- if `StatusIconMode` is an enum with values 0–3, something like:
```cpp
value = std::clamp(value, 0, 3);
status_icon_mode_ = static_cast<uint8_t>(value);
```
- or explicitly handle invalid values by falling back to a default mode.
2. In the implementation of `ProgramData::statusIconMode() const`:
- Optionally ensure the returned value is in the valid range (in case older persisted config had invalid data), e.g.:
```cpp
int mode = static_cast<int>(status_icon_mode_);
// clamp or map invalid values to a default
if (mode < 0 || mode > 3) {
mode = /* default valid mode */;
}
return mode;
```
3. In `ReadProgData` (or whichever function loads the config):
- When reading the status icon mode from configuration:
- Either call `setStatusIconMode(loaded_value)` so the clamping logic is centralized, or
- Clamp/validate `loaded_value` before assigning to `status_icon_mode_`.
These changes ensure that we do not silently truncate values >3 when storing them and that any out-of-range or future enum values are handled in a controlled way.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #713 +/- ##
=======================================
Coverage ? 52.11%
=======================================
Files ? 64
Lines ? 8641
Branches ? 0
=======================================
Hits ? 4503
Misses ? 4138
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new user-configurable status icon mode setting to iptux, allowing users to choose between no status icon, normal status icon, or blinking status icon (though blinking is not yet implemented). The feature includes UI preferences, configuration persistence, and platform-specific implementations for Linux and macOS app indicators.
Changes:
- Introduces StatusIconMode enum with three modes: NONE, NORMAL, and BLINKING
- Adds UI preferences in DataSettings for selecting status icon mode with proper localization
- Implements configuration persistence through ProgramData with storage in IptuxConfig
- Updates app indicator implementations on Linux and macOS to respect the selected mode, including hiding the icon when NONE is selected
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/api/iptux-core/StatusIconMode.h | Defines the StatusIconMode enum with three values |
| src/api/iptux-core/ProgramData.h | Adds statusIconMode getter/setter methods and 2-bit bitfield storage |
| src/iptux-core/ProgramData.cpp | Implements configuration read/write for status icon mode with default value |
| src/iptux/DataSettings.cpp | Adds GTK+ combo box UI widget for selecting status icon mode in preferences |
| src/iptux/Application.cpp | Initializes app indicator mode on startup and updates it when configuration changes |
| src/iptux/AppIndicator.h | Adds SetMode method to the IptuxAppIndicator interface |
| src/iptux/AppIndicator.cpp | Implements status icon mode handling for Linux using ayatana-appindicator |
| src/iptux/AppIndicatorMac.mm | Implements status icon mode handling for macOS using NSStatusItem visibility |
| src/iptux/AppIndicatorDummy.cpp | Adds dummy SetMode implementation for platforms without status icon support |
| src/config.h.in | Defines HAVE_STATUS_ICON macro based on platform capabilities |
| NEWS | Documents the new feature in version 0.10.0 release notes |
| if (self.enable_app_indicator_) { | ||
| self.app_indicator = | ||
| make_shared<IptuxAppIndicator>(G_ACTION_GROUP(self.app)); | ||
| self.app_indicator->SetMode(StatusIconMode(self.data->statusIconMode())); |
There was a problem hiding this comment.
Using a C-style cast for StatusIconMode conversion. Consider using static_cast for better type safety and to make the conversion explicit. This would help catch potential type mismatches at compile time.
| self.app_indicator->SetMode(StatusIconMode(self.data->statusIconMode())); | |
| self.app_indicator->SetMode( | |
| static_cast<StatusIconMode>(self.data->statusIconMode())); |
| add_accelerator(app, "win.send_message", "<Primary>Return"); | ||
| } | ||
| if (app_indicator) { | ||
| app_indicator->SetMode(StatusIconMode(data->statusIconMode())); |
There was a problem hiding this comment.
Using a C-style cast for StatusIconMode conversion. Consider using static_cast for better type safety and to make the conversion explicit.
| app_indicator->SetMode(StatusIconMode(data->statusIconMode())); | |
| app_indicator->SetMode( | |
| static_cast<StatusIconMode>(data->statusIconMode())); |
| progdt->setStatusIconMode( | ||
| std::stoi(gtk_combo_box_get_active_id(GTK_COMBO_BOX(widget)))); |
There was a problem hiding this comment.
The value retrieved from the combo box is parsed using std::stoi without validation. If the combo box somehow contains an invalid or out-of-range value (e.g., not 0, 1, or 2), this could result in an invalid StatusIconMode being stored. Consider validating the parsed integer is within the valid range (0-2) before calling setStatusIconMode, or add a bounds check in the setStatusIconMode method itself.
| return status_icon_mode_; | ||
| } | ||
|
|
||
| void ProgramData::setStatusIconMode(int value) { |
There was a problem hiding this comment.
The setStatusIconMode method accepts an int parameter and directly assigns it to the 2-bit bitfield status_icon_mode_ without validation. Since the bitfield can only hold values 0-3, but only 0-2 are valid StatusIconMode values, values outside this range could lead to unexpected behavior. Consider either: 1) changing the parameter type to StatusIconMode to enforce type safety, or 2) adding validation to clamp or reject invalid values.
| void ProgramData::setStatusIconMode(int value) { | |
| void ProgramData::setStatusIconMode(int value) { | |
| if (value < 0) { | |
| value = 0; | |
| } else if (value > 2) { | |
| value = 2; | |
| } |
| int statusIconMode() const; | ||
| void setStatusIconMode(int value); |
There was a problem hiding this comment.
The API declares statusIconMode() as returning int and setStatusIconMode() as accepting int, rather than using the StatusIconMode enum type. For better type safety and API clarity, consider changing these to use StatusIconMode directly.
| int statusIconMode() const; | |
| void setStatusIconMode(int value); | |
| StatusIconMode statusIconMode() const; | |
| void setStatusIconMode(StatusIconMode value); |
| } kStatusIconModes[] = { | ||
| {STATUS_ICON_MODE_NONE, N_("No status icon")}, | ||
| {STATUS_ICON_MODE_NORMAL, N_("Normal status icon")}, | ||
| {STATUS_ICON_MODE_BLINKING, N_("Blinking status icon")}, |
There was a problem hiding this comment.
The STATUS_ICON_MODE_BLINKING option is presented to users in the UI, but there is no implementation that actually handles blinking behavior. Both the Linux (AppIndicator.cpp) and macOS (AppIndicatorMac.mm) implementations only check for STATUS_ICON_MODE_NONE and treat all other modes (including BLINKING) the same as NORMAL. Either implement the blinking functionality or remove this option from the UI until it's supported.
| {STATUS_ICON_MODE_BLINKING, N_("Blinking status icon")}, |
| bool IsUsingBlacklist() const; | ||
| bool IsFilterFileShareRequest() const; | ||
| bool isHideTaskbarWhenMainWindowIconified() const; | ||
| int statusIconMode() const; |
There was a problem hiding this comment.
Naming convention inconsistency: the getter uses camelCase (statusIconMode) while the setter uses camelCase (setStatusIconMode), but the similar method isHideTaskbarWhenMainWindowIconified uses "is" prefix for the getter. While "is" is typically used for boolean getters, for consistency with other non-boolean getters in this class (like port(), which doesn't use "get"), consider using getStatusIconMode() or keeping statusIconMode() but ensuring consistency throughout.
| int statusIconMode() const; | |
| int statusIconMode() const; | |
| int getStatusIconMode() const { return statusIconMode(); } |
| int ProgramData::statusIconMode() const { | ||
| return status_icon_mode_; | ||
| } | ||
|
|
||
| void ProgramData::setStatusIconMode(int value) { | ||
| status_icon_mode_ = value; |
There was a problem hiding this comment.
The statusIconMode getter returns int instead of StatusIconMode. For better type safety and API consistency, consider returning StatusIconMode directly. This would make the API clearer and prevent misuse. The setStatusIconMode method could similarly be updated to accept StatusIconMode instead of int.
| int ProgramData::statusIconMode() const { | |
| return status_icon_mode_; | |
| } | |
| void ProgramData::setStatusIconMode(int value) { | |
| status_icon_mode_ = value; | |
| StatusIconMode ProgramData::statusIconMode() const { | |
| return static_cast<StatusIconMode>(status_icon_mode_); | |
| } | |
| void ProgramData::setStatusIconMode(StatusIconMode value) { | |
| status_icon_mode_ = static_cast<decltype(status_icon_mode_)>(value); |
| proof_shared = config->GetBool("proof_shared"); | ||
| hide_taskbar_when_main_window_iconified_ = | ||
| config->GetBool("hide_taskbar_when_main_window_iconified"); | ||
| status_icon_mode_ = config->GetInt("status_icon_mode", STATUS_ICON_MODE_NORMAL); |
There was a problem hiding this comment.
The new status icon mode feature lacks test coverage. Consider adding tests to verify: 1) the status icon mode is correctly saved and loaded from configuration, 2) the default value is STATUS_ICON_MODE_NORMAL when not set, and 3) invalid values are handled appropriately. The existing ProgramDataTest.cpp file demonstrates the testing pattern for configuration persistence.
| proof_shared = config->GetBool("proof_shared"); | ||
| hide_taskbar_when_main_window_iconified_ = | ||
| config->GetBool("hide_taskbar_when_main_window_iconified"); | ||
| status_icon_mode_ = config->GetInt("status_icon_mode", STATUS_ICON_MODE_NORMAL); |
There was a problem hiding this comment.
When reading the status_icon_mode_ from configuration, there is no validation to ensure the value is within the valid range (0-2). If the configuration file contains an invalid value (e.g., 3 or negative), it will be stored directly into the 2-bit bitfield, potentially causing unexpected behavior. Consider adding validation similar to how send_message_retry_in_us is validated on lines 153-155, clamping the value to the valid range or resetting to the default if invalid.
| status_icon_mode_ = config->GetInt("status_icon_mode", STATUS_ICON_MODE_NORMAL); | |
| int status_icon_mode = | |
| config->GetInt("status_icon_mode", STATUS_ICON_MODE_NORMAL); | |
| if (status_icon_mode < 0 || status_icon_mode > 2) { | |
| status_icon_mode = STATUS_ICON_MODE_NORMAL; | |
| } | |
| status_icon_mode_ = status_icon_mode; |
Summary by Sourcery
Add configurable status icon display modes and wire them through application settings, persistence, and app indicator implementations.
New Features:
Enhancements: