GNOME Bugzilla – Bug 634080
system monitor applet
Last modified: 2011-07-21 16:03:14 UTC
I cant find it in recent mockup( possible places for system monitor applet: 1 left side of message tray 2 some where in overview 3 in alt_f2 screen Which one is better? should it be an extension?
Actually, if a system monitor is ever needed, it should go to the left of the status area (top right), along with other system indicators. Bottom right is for notifications, the overview is for window management and alt-f2 should do rapid window switching and nothing else. This said, I'm not sure if it is worth making it core (there was a discussion about depending on libgtop for something else) or it is better left as an extension.
I really miss the system monitor applet. As a developper I often use bleeding edge software and being able to quickly see when one process start eating all my CPU and/or memory is a must have to me.
I don't have any plans to add such a thing. I think it would be better for the system to be robust against this type of failure. Ideally the system would be able to gather a backtrace of an application malfunctioning in this way just like if it had crashed. For more generally usage the system monitor application should do.
Created attachment 179229 [details] [review] extension: systemMonitor (depend on Bug 640468, Bug 630915)
Review of attachment 179229 [details] [review]: Generally good, but still needs some fixes. ::: configure.ac @@ +33,3 @@ ADDITIONAL_PACKAGES="gnome-desktop-3.0 >= 2.91.6" ;; + alternate-tab|example|windowsNavigator|systemMonitor) It should not be added here, because it hard depends on libgtop, whose pkg-config file must be appended to $ADDITIONAL_PACKAGES. ::: extensions/systemMonitor/Makefile.am~ @@ +1,3 @@ +EXTENSION_ID = windowsNavigator + +include ../../extension.mk Kill this file, please. ::: extensions/systemMonitor/extension.js @@ +27,3 @@ + + _initValues: function() { + this.values = []; If this is a pure virtual, it doesn't need code. @@ +127,3 @@ + box.add((new CpuIndicator()).actor); + box.add((new MemoryIndicator()).actor); + Main.messageTray.actor.add_actor(box); This is going to break hard if messageTray code changes. Instead you should use a custom MessageTray.Source (returning your custom actor in createIcon). I assume that you want at some point implement a real notification, showing additional data, instead of two dumb icons (having something that looks like a notification but does not respond to clicks is awkward) - else it is probably better to place the indicator in the system status area (use the newly added indicator factories to avoid getting a menu you don't need) @@ +128,3 @@ + box.add((new MemoryIndicator()).actor); + Main.messageTray.actor.add_actor(box); +} catch(e) {log(e);} You don't need try-catch, the extensionSystem already ensures that exceptions from main() don't propagate. ::: extensions/systemMonitor/stylesheet.css @@ +3,3 @@ +} + +.indicator-area { Stylesheets are not scoped to the extension, so it is better to prefix classes with something unique, to avoid conflicts.
*** Bug 644314 has been marked as a duplicate of this bug. ***
Created attachment 183258 [details] [review] extension: systemMonitor >Instead you should use a custom MessageTray.Source (returning your custom actor in createIcon). I want place it on left side. > having something that looks like a notification but does not respond to clicks is awkward now It start gnome-system-monitor on click > better to place the indicator in the system status area Not sure about this. (still depend on Bug 640468)
(In reply to comment #7) > Created an attachment (id=183258) [details] [review] > extension: systemMonitor > > >Instead you should use a custom MessageTray.Source (returning your custom actor in createIcon). > I want place it on left side. You mean on the left corner? Isn't it difficult to discover? I for one only look to the right when I open the tray. > > better to place the indicator in the system status area > Not sure about this. In the status area it would be more visible, you wouldn't need to open the tray to check (which may be problematic if the CPU is 100%). On the other hand, in the message tray, it would be better if you want to show extended details on click. But since you just start gnome-system-monitor, and everything not using Notification should be killed by the message tray, just stuff it in the status area. > (still depend on Bug 640468)
Created attachment 185161 [details] [review] add ability to write part of extension in C
Created attachment 185162 [details] [review] extension: systemMonitor
(In reply to comment #9) > Created an attachment (id=185161) [details] [review] > add ability to write part of extension in C So this obsoletes the need for bug 640468 to be fixed?
(In reply to comment #11) > So this obsoletes the need for bug 640468 to be fixed? yes.
Maxim, I just tried building your latest patches on Fedora 15 alpha. I applied both your patches to git master. I then did this: $ ./configure --enable-extensions=systemMonitor ... $ make -j4 ... configure.ac:72: required file `config/config.guess' not found configure.ac:72: `automake --add-missing' can install `config.guess' configure.ac:72: required file `config/config.sub' not found configure.ac:72: `automake --add-missing' can install `config.sub' $ automake --add-missing configure.ac:72: installing `config/config.guess' configure.ac:72: installing `config/config.sub' configure.ac:72: required file `config/ltmain.sh' not found extensions/common/Makefile.am:36: typelibdir multiply defined in condition TRUE ... extensions/common/Makefile.am:9: ... `typelibdir' previously defined here extensions/common/Makefile.am:37: typelib_DATA multiply defined in condition TRUE ... extensions/common/Makefile.am:10: ... `typelib_DATA' previously defined here $ And now I'm stuck there. If I run make again, I see the same errors about multiple definitions in Makefile.am.
(In reply to comment #13) > And now I'm stuck there. If I run make again, I see the same errors about > multiple definitions in Makefile.am. 'make distclean' (or git clean -f) should fix this problem.
Still stuck: $ git clean -f ... $ ./autogen.sh --enable-extensions=systemMonitor ... $ make Making all in extensions make[1]: Entering directory `/home/adam/src/gnome-shell-extensions/extensions' Making all in systemMonitor make[2]: Entering directory `/home/adam/src/gnome-shell-extensions/extensions/systemMonitor' make[2]: *** No rule to make target `Makefile.am', needed by `Makefile.in'. Stop. make[2]: Leaving directory `/home/adam/src/gnome-shell-extensions/extensions/systemMonitor' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/adam/src/gnome-shell-extensions/extensions' make: *** [all-recursive] Error 1 $
I realized that the 'git clean' in my previous comment was incomplete - I didn't specify -x, so it left ignored files in place. Here's what happens if I use 'git clean -f -x' to restore a pristine working tree: $ git clean -f -x ... $ ./autogen.sh --enable-extensions=systemMonitor ... extensions/common/Makefile.am:36: typelibdir multiply defined in condition TRUE ... extensions/common/Makefile.am:9: ... `typelibdir' previously defined here extensions/common/Makefile.am:37: typelib_DATA multiply defined in condition TRUE ... extensions/common/Makefile.am:10: ... `typelib_DATA' previously defined here extensions/common/Makefile.am: installing `config/depcomp' configure.ac:75: required file `extensions/systemMonitor/Makefile.in' not found $
Review of attachment 185162 [details] [review]: forgot attach extensions/systemMonitor/Makefile.am. (will do it later today)
Review of attachment 185161 [details] [review]: Please avoid doing this at all costs if possible. Rewrite gtop if needed (it would help anyway), but really, C code in the extensions is a big problem. ::: configure.ac @@ +66,3 @@ + +# Collect more than 20 libraries for a prize! +PKG_CHECK_MODULES(GNOME_SHELL_EXTENSIONS, gio-2.0 >= $GIO_MIN_VERSION) You check for gio here, then the GIR depends on Clutter and ClutterX11, and shell-extensions-global.h includes <glib-object.h>. ??? ::: extensions/Makefile.am @@ +1,3 @@ DIST_SUBDIRS = example alternate-tab xrandr-indicator windowsNavigator auto-move-windows dock user-theme alternative-status-menu gajim +SUBDIRS = $(ENABLED_EXTENSIONS) common Since DIST_SUBDIRS is set explicitly, you need to add "common" there as well. ::: extensions/common/Makefile.am @@ +1,1 @@ +lib_LTLIBRARIES = libgnome-shell-extensions.la You know that $(prefix) will be ~/.local in most situations, and that ~/.local/lib is not a library directory by default? Do you expect people to set $LD_LIBRARY_PATH in ~/.profile? @@ +9,3 @@ +typelibdir = $(pkglibdir) +typelib_DATA = $(INTROSPECTION_GIRS:.gir=.typelib) + Duplicate variable here. ::: extensions/common/shell-extensions-global.c @@ +16,3 @@ + */ + +#include <glib-object.h> Maybe include you're own header? (GCC complains if you define a function without a prototype)
(In reply to comment #18) > C code in the extensions is a big problem. This ability will decrease complexity of development new extension. (e.g. I start working on Bug 646137. There need to fix vala gir generator... ) It have only purpose to wrap functions.
(In reply to comment #19) > (In reply to comment #18) > > C code in the extensions is a big problem. > This ability will decrease complexity of development new extension. > (e.g. I start working on Bug 646137. There need to fix vala gir generator... ) > It have only purpose to wrap functions. A bug in vala is a bug in vala. Same for gjs, or gobject-introspection. (or libgtop, for this bug). If we need C code to access our libraries, then we have failed, completely.
Created attachment 185393 [details] [review] add ability to write part of extension in C > A bug in vala is a bug in vala. Same for gjs, or gobject-introspection. Bugs should be fixed, _but_ they should not block extension development. > (or libgtop, for this bug). gjs and gobject-introspection (patch for libgtop was landed) > If we need C code to access our libraries Extension should have ability use ANY library :) > Do you expect people to set $LD_LIBRARY_PATH in ~/.profile? I expect that, it should be configured with --libdir=/usr/lib64 ( or better, install extension without prefix )
Created attachment 185394 [details] [review] add ability to write part of extension in C
Created attachment 185395 [details] [review] extension: systemMonitor
I tried out this latest patch. I had to build and install libgtop, since the Fedora 15 libgtop package doesn't include a type library. I installed libgtop in /usr/local, so I had to export GI_TYPELIB_PATH=/usr/local/lib/girepository-1.0 in order for gjs to be able to find the libgtop type library. Once I did this it worked. I think this is a promising start. The current patch displays two meters (one CPU, one memory?) in the lower left corner of the activities overview. Like Giovanni, I'd much prefer for a CPU indicator to appear as a system indicator in the upper right corner of the display. That way I can see it at all the time. I'd rather not have separate indicators for CPU and memory in that area, though; that would be too visually noisy. I really only care about CPU; I can use another tool (e.g. system-monitor) to look at memory usage in the rare cases where I'm worried about that. It seems there's resistance to including C code in gnome-shell-extensions (though I myself am sort of neutral about that). Given that, it seems the right path forward here is presumably either (a) to fix bug 640468, which would allow gjs to access the existing libgtop API via introspection, or (b) to change the libgtop API to export a method which gjs can call more easily.
Created attachment 187321 [details] [review] systemMonitor: new extension fix bug 640468 was fixed. now system monitor don't depend on ability to write part of extension in C. But I think second patch still usefull. > I'd much prefer for a CPU indicator to appear as a system indicator in the upper right corner of the display. it should configurable. > I'd rather not have separate indicators for CPU and memory in that area same...
Review of attachment 187321 [details] [review]: Almost ready to merge... ::: configure.ac @@ +42,3 @@ + systemMonitor) + ENABLED_EXTENSIONS="$ENABLED_EXTENSIONS $e" + ADDITIONAL_PACKAGES="libgtop-2.0 >= 2.28.3" One can specify extensions in any order, so you need to append, not replace $ADDITIONAL_PACKAGES. ::: extensions/systemMonitor/extension.js @@ +21,3 @@ + this.actor.connect('repaint', Lang.bind(this, this._draw)); + this.actor.connect('button-press-event', function() { + Util.spawn(["gnome-system-monitor"]); Can you use ShellAppSystem here? It seems to have better support for focus stealing prevention (don't ask me why) @@ +69,3 @@ + _initValues: function() { + this._prev = new GTop.glibtop_cpu; + GTop.glibtop_get_cpu(this._prev); Shouldn't this be (out caller-allocates)? (same for memory) Also, why the function is called glibtop_get_cpu? Shouldn't it be GTop.get_cpu()? @@ +125,3 @@ +}; + +function main() { Don't you need to init libgtop here?
Created attachment 187531 [details] [review] systemMonitor: new extension > One can specify extensions in any order, so you need to append, not replace > $ADDITIONAL_PACKAGES. done > Can you use ShellAppSystem here? done >Shouldn't this be (out caller-allocates)? (same for memory) It can be. But it will greatly increase memory usage. (1/0.25 * 2 * 60sec * 2080byte on x64) > Also, why the function is called glibtop_get_cpu? Shouldn't it be GTop.get_cpu()? right... I think, it should not block this patch. > Don't you need to init libgtop here? No
(In reply to comment #27) > Created an attachment (id=187531) [details] [review] > systemMonitor: new extension > > >Shouldn't this be (out caller-allocates)? (same for memory) > It can be. But it will greatly increase memory usage. (1/0.25 * 2 * 60sec * > 2080byte on x64) Wait: this means that there is a leak somewhere in gjs. We should track it down, not workaround it. Memory consuption should not increase by using (out caller-allocates): while it does allocate an extra temporary structure, it is immediately freed at the end of the call.
Review of attachment 187531 [details] [review]: Fine to commit!
Comment on attachment 187531 [details] [review] systemMonitor: new extension > Wait: this means that there is a leak somewhere in gjs. No. memory will free on gc. But will increase frequency of gc (and increase gc trigger in gjs_maybe_gc).