-
Notifications
You must be signed in to change notification settings - Fork 212
serverstatus@maksmokriev: fix BoxBgColor setting, fix multiple mainloop callbacks#1740
serverstatus@maksmokriev: fix BoxBgColor setting, fix multiple mainloop callbacks#1740KopfdesDaemons wants to merge 2 commits intolinuxmint:masterfrom
Conversation
@maksmokriev closes #1736
The wrong callback is being used for the BoxBgColor setting. onSettingsBgChanged should be onSettingsChanged. onSettingsBgChanged only updates the container, not the box itself.
There is also a critical bug with the main loop. Every time a setting is changed, another callback is added to the main loop. Changing settings multiple times can overload the entire system.
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.
Pull request overview
This pull request fixes a critical bug in the serverstatus desklet where settings changes would create multiple mainloop callbacks, overloading the system. It also corrects the BoxBgColor and FontSize setting callbacks to properly update the UI elements. The changes improve stability and fix a user-reported issue where the element background color setting was being ignored.
Changes:
- Fixed BoxBgColor and FontSize settings to use the correct callback handler
- Added logic to prevent duplicate mainloop callbacks when settings change
- Bumped version from 1.2.0 to 1.2.1
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| metadata.json | Version bump to 1.2.1 to reflect the bug fixes |
| desklet.js | Fixed BoxBgColor/FontSize callbacks and added mainloop deduplication logic |
| Mainloop.timeout_add_seconds(5, () => { | ||
| this.updateStatus(); | ||
| return true; | ||
| if (this.mainloop) { | ||
| Mainloop.source_remove(this.mainloop); | ||
| } | ||
| //After the first call, we start a periodic update | ||
| this.mainloop = Mainloop.timeout_add_seconds(this.TimeRepeat, () => { | ||
| this.updateStatus(); | ||
| return true; | ||
| }); | ||
| return false; //Return false so that this timer only runs once | ||
| }); |
Copilot
AI
Feb 19, 2026
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.
The initial 5-second timeout is not tracked, which means if settings are changed during this initial 5-second delay, a new timeout will be created but the old one will continue to execute. This could result in updateStatus() being called and creating a new mainloop even though the settings have changed. Consider storing the initial timeout ID and removing it in the condition check, similar to how the mainloop is handled.
| //After the first call, we start a periodic update | ||
| this.mainloop = Mainloop.timeout_add_seconds(this.TimeRepeat, () => { | ||
|
|
||
| if (!this.mainloop || this.oldTimeRepeat != this.TimeRepeat) { |
Copilot
AI
Feb 19, 2026
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.
The variable oldTimeRepeat is never initialized in the constructor, which means the first time this condition is evaluated, it will compare undefined != this.TimeRepeat, which will always be true. While this works for the first initialization, it would be clearer to explicitly initialize this.oldTimeRepeat = null in the _init method to make the intent explicit.