Conversation
|
Update RSSDock applet.js, fixed refresh time problem |
Update time error fixed.
Update time fix
There was a problem hiding this comment.
Pull request overview
This PR introduces a new CPU/GPU/RAM monitoring applet for the Cinnamon desktop environment and also includes updates to an existing RSS dock applet. The new applet provides real-time system resource monitoring with support for NVIDIA and AMD GPUs, while the RSS dock changes improve the refresh loop management.
Changes:
- Added a new cpugpuram-viewer@lyk4s5 applet that displays CPU, GPU, and RAM usage percentages in the panel with automatic GPU detection
- Modified the rssdock@lyk4s5 applet's refresh loop implementation to prevent overlap and add minimum interval protection
- Includes documentation, metadata, and icon assets for the new applet
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| cpugpuram-viewer@lyk4s5/files/cpugpuram-viewer@lyk4s5/applet.js | Core applet implementation with system monitoring logic for CPU, GPU (NVIDIA/AMD), and RAM |
| cpugpuram-viewer@lyk4s5/files/cpugpuram-viewer@lyk4s5/metadata.json | Applet metadata with UUID, name, description, and version |
| cpugpuram-viewer@lyk4s5/files/cpugpuram-viewer@lyk4s5/icon.png | Binary icon file for the applet |
| cpugpuram-viewer@lyk4s5/screenshot.png | Screenshot demonstrating the applet's appearance |
| cpugpuram-viewer@lyk4s5/info.json | Basic author and name information for the applet |
| cpugpuram-viewer@lyk4s5/README.md | Comprehensive documentation covering features, requirements, installation, and usage |
| rssdock@lyk4s5/files/rssdock@lyk4s5/applet.js | Updated refresh loop management to remove old timeouts before creating new ones and add interval validation |
| getCpuUsage() { | ||
| try { | ||
| let [res, fileContent] = GLib.file_get_contents('/proc/stat'); | ||
| if (!res) return 0; | ||
|
|
||
| let lines = fileContent.toString().split('\n'); | ||
| let cpuLine = lines[0].split(/\s+/); | ||
|
|
||
| let idle = parseInt(cpuLine[4]) + parseInt(cpuLine[5]); | ||
| let total = 0; | ||
| for (let i = 1; i < cpuLine.length; i++) { | ||
| let val = parseInt(cpuLine[i]); | ||
| if (!isNaN(val)) total += val; | ||
| } | ||
|
|
||
| let diffIdle = idle - this.prevIdle; | ||
| let diffTotal = total - this.prevTotal; | ||
| let usage = 0; | ||
|
|
||
| if (diffTotal > 0) { | ||
| usage = Math.round(100 * (diffTotal - diffIdle) / diffTotal); | ||
| } | ||
|
|
||
| this.prevIdle = idle; | ||
| this.prevTotal = total; | ||
|
|
||
| return usage; | ||
| } catch(e) { return 0; } |
There was a problem hiding this comment.
The getCpuUsage function will return 0 on the first call because prevIdle and prevTotal are initialized to 0. When diffTotal is calculated on the first run, it will be non-zero, but the usage calculation will be based on the delta from 0, which may give an inaccurate initial reading. Consider either initializing these values properly in the constructor by reading /proc/stat once, or handling the first call specially to avoid showing misleading data.
| updateLoop() { | ||
| this.updateMetrics(); | ||
| // 2 saniyede bir güncelle | ||
| Mainloop.timeout_add_seconds(2, () => { | ||
| this.updateLoop(); | ||
| }); |
There was a problem hiding this comment.
The updateLoop recursively calls itself, creating a new function context each time. While this works, it's less efficient than using a repeating timeout pattern. Consider changing line 54 to return true at the end of updateMetrics(), and modify updateLoop to use: this._updateLoopID = Mainloop.timeout_add_seconds(2, Lang.bind(this, this.updateMetrics)); This approach is more memory-efficient and is the pattern used in other applets like cpu-monitor-text@gnemonix.
| @@ -0,0 +1,5 @@ | |||
|
|
|||
There was a problem hiding this comment.
The file starts with an unnecessary blank line. This should be removed for consistency with the format seen in other applet info.json files in the repository.
| if (!sources.length) { | ||
| this._tickerText = _("No RSS source configured"); | ||
| this._buildMenu(); | ||
| this._refreshLoop = Mainloop.timeout_add_seconds(60, Lang.bind(this, this._updateFeed)); |
There was a problem hiding this comment.
The hardcoded 60-second refresh interval for the "no sources configured" case is inconsistent with the dynamic interval calculation at line 135. If a user has set a custom update_interval (e.g., 5 minutes), and then removes all sources, the applet will suddenly switch to checking every minute. This could be confusing behavior. Consider using the same interval calculation here: Math.max(1, this.update_interval) * 60, or at least using this.update_interval * 60 if it's guaranteed to be >= 1.
| this._refreshLoop = Mainloop.timeout_add_seconds(60, Lang.bind(this, this._updateFeed)); | |
| this._refreshLoop = Mainloop.timeout_add_seconds(Math.max(1, this.update_interval) * 60, Lang.bind(this, this._updateFeed)); |
|
|
||
| ## License | ||
|
|
||
| 1. Distributed under the MIT License. |
There was a problem hiding this comment.
In the README, there's inconsistent indentation and formatting in the numbered list. Line 41 uses a tab character for indentation while other list items use consistent spacing. This should be standardized to use consistent indentation (either spaces or tabs, but not mixed).
| 1. Distributed under the MIT License. | |
| 1. Distributed under the MIT License. |
| constructor(metadata, orientation, panel_height, instance_id) { | ||
| super(orientation, panel_height, instance_id); | ||
|
|
||
|
|
There was a problem hiding this comment.
The metadata parameter is passed to the constructor but never used. While it's part of the standard Cinnamon applet signature, consider either using it (e.g., for setting the icon path like in rssdock@lyk4s5) or documenting why it's not needed. If the applet grows and needs to access metadata.path or other properties, they won't be available without storing the metadata parameter.
| // Store metadata for potential future use (e.g., icon paths, configuration). | |
| this._metadata = metadata; |
| "name": "CPU/GPU/RAM Viewer", | ||
| "description": "Minimalist system monitor for CPU, GPU, and RAM usage.", | ||
| "version": "1.0" | ||
| } |
There was a problem hiding this comment.
There's trailing whitespace at the end of this line. This should be removed for code cleanliness and consistency.
| } | |
| } |
| updateLoop() { | ||
| this.updateMetrics(); | ||
| // 2 saniyede bir güncelle | ||
| Mainloop.timeout_add_seconds(2, () => { | ||
| this.updateLoop(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The updateLoop method creates an infinite recursion pattern that doesn't properly manage the timeout ID. This can lead to memory leaks because there's no way to clean up the timeout when the applet is removed from the panel. The timeout ID should be stored (e.g., this._updateLoopID) and removed in an on_applet_removed_from_panel callback using Mainloop.source_remove(). Look at cpu-monitor-text@gnemonix/files/cpu-monitor-text@gnemonix/applet.js:64-66 for an example of proper cleanup.
| @@ -114,6 +116,7 @@ MyApplet.prototype = { | |||
| link: link, | |||
| date: dateObj, | |||
| source: source.label || "RSS" | |||
|
|
|||
| }); | |||
| }); | |||
| } catch (e) { | |||
| @@ -128,6 +131,9 @@ MyApplet.prototype = { | |||
| } | |||
| }); | |||
| }); | |||
| // refresh every X seconds | |||
| let interval = Math.max(1, this.update_interval) * 60; | |||
| this._refreshLoop = Mainloop.timeout_add_seconds(interval, Lang.bind(this, this._updateFeed)); | |||
There was a problem hiding this comment.
The added code removes any existing refresh loop at the start of _updateFeed. However, this removal happens before checking if there are any news sources configured. If _updateFeed is called when there are no sources, it schedules a new refresh loop (line 89) but the old one was already removed (line 83). This is correct behavior, but consider that if _updateFeed fails or throws an exception before line 136, there will be no refresh loop scheduled. Add error handling to ensure the refresh loop is always rescheduled.
| // refresh every X seconds | ||
| let interval = Math.max(1, this.update_interval) * 60; |
There was a problem hiding this comment.
The Math.max(1, this.update_interval) ensures a minimum interval of 1 minute, which is good defensive programming. However, the original code at line 42-43 that was removed didn't have this protection. Consider documenting this change in the commit message or adding a comment explaining why the minimum is necessary to prevent excessive API calls or resource usage.
| // refresh every X seconds | |
| let interval = Math.max(1, this.update_interval) * 60; | |
| // refresh every X seconds; enforce a minimum of 1 minute between updates | |
| // to avoid excessive polling / API calls even if update_interval is misconfigured | |
| let interval = Math.max(1, this.update_interval) * 60; |
A minimalist and high-performance applet to monitor CPU, GPU, and RAM usage in real-time.