GNOME Bugzilla – Bug 77108
multiload applet should auto-select appropriate colors tab
Last modified: 2005-08-22 16:16:08 UTC
multiload-applet-2 has a "Properties..." menu item which brings up a settings dialog. This dialog includes four tabs, each of which offers color settings for one of the applet's displays. When the dialog appears, the first tab (Processor) is always displayed, regardless of where the mouse pointer was when the menu was brought up. It would be better for this dialog to be more context sensitive. For example, if my mouse was floating over the Memory display, then the dialog should appear with the Memory color tab initially visible. And it clearly doesn't make much sense to have the Processor tab selected if the Processor display isn't even active.
enhancement. Won't happen for GNOME2
I think this would be nice for 2.8 tho!
This should be an easy fix.
the tooltip code really needs love. Currently, the tooltip is update every <interval> even if it's not displayed (calls multiload_applet_tooltip_update). This because on the first "enter_notify_event", multiload_enter_cb() sets .tooltip_update to TRUE. Then on every load_graph_update(), as .tooltip_update is TRUE, multiload_applet_tooltip_update() is called.
Created attachment 39577 [details] [review] Opens clicked graph's tab in properties Patch puts a clicked_on flag to graph widgets, so when they are right clicked that flag should be set. It opens the page in notebook widget, knowing that pages are in same order w/ graph widgets. It might be better to make a order map, but currently it's working unless order is satisfied. Patch is against gnome 2.8
There some whitespace / indentation issue in your patch. + for (i = 0; i < nlist; i++) { + graph = g_list_nth_data(object_list, i); hey, it's a list, not an array :) Please fix this.
Well whitespace issue is alright, but I think it's best to get graph with g_list_nth_data, because g_list_nth is returning another glist, which is not useful. What do you think about it? I'll fix the indentation issue, no biggie.
Created attachment 39587 [details] [review] Fixed indentaion/whitespace and new line problems
int cpu_initialized; + gboolean clicked_on; gboolean visible; g->show_frame = TRUE; + g->clicked_on = FALSE; g_signal_connect (G_OBJECT(g->disp), "destroy", G_CALLBACK (load_graph_destroy), g); + g_signal_connect (G_OBJECT(g->disp), "button_press_event", + G_CALLBACK (load_graph_bpress) , g); gtk_notebook_append_page(GTK_NOTEBOOK(notebook), page, page_label); - return page; still some garbage about the list : i mean, iterate it manually, not using o(N) g_list_nth*
Well they are all used. Why do you think they are garbage? I'll iterate the list if it's better.
Ah you meant indentation, sorry, I'll fix all at once, thanks for noticing.
Created attachment 39592 [details] [review] Hopefully last fix How's that? I iterated the list.
Comment on attachment 39592 [details] [review] Hopefully last fix This is looking ok. Most of these points are stylistic. > g_signal_connect (G_OBJECT(g->disp), "destroy", > G_CALLBACK (load_graph_destroy), g); >+ g_signal_connect (G_OBJECT(g->disp), "button_press_event", >+ G_CALLBACK (load_graph_bpress) , g); >+ Please keep indentation consistant with other signals. >+ >+ if (event->button == 3) { >+ while ((list_iterator = g_list_next(list_iterator)) != NULL) { >+ graph = list_iterator->data; >+ graph -> clicked_on = FALSE; Stylistically in GTK, you usually see for (l = list; l; l = l->next) for iterating through linked lists. Also we use K&R brace style (even though that might not be obvious due to the stylistic mess of the existing codebase). All new code should be in K&R style. So: if (event->button == 3) { ... ... } Code that is executed as part of the same block should be indented together. > gtk_window_set_screen (GTK_WINDOW (dialog), >- gtk_widget_get_screen (GTK_WIDGET (ma->applet))); >+ gtk_widget_get_screen (GTK_WIDGET (ma->applet))); Why has the indentation of this changed? The gtk_widget_get_screen would appear to be part of the gtk_window_set_screen above, hence forth it should be indented somewhat (style varies). It definitely should be at the same indent level as other things however. Thanks.
Created attachment 39593 [details] [review] Lastly proposed patch Second indentation is my mistake, i though it's indented wrongly, i though it's another call, will change it to back. For iteration, I also like to use for for iterating over linked lists, but for this one, line was really long, and while did job very neat and shorter. Since as a performance both is same, (while is even a lil faster I think, for this one). I'll use K&R style for now on.
[15:56:54][pts/9][~/cvs.gnome.org/gnome-applets/multiload][#6][&0] benoit@ibook >>> curl 'http://bugzilla.gnome.org/attachment.cgi?id=39593&action=view' | patch -p0 % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 5432 0 5432 0 0 4784 0 --:--:-- 0:00:01 --:--:-- 1027k patching file global.h Hunk #1 succeeded at 46 (offset -5 lines). patch: **** malformed patch at line 27: Index: load-graph.c WTF ? + GList *list_iterator = g_list_first(object_list); + graph -> clicked_on = FALSE; still some whitespace issues :/ and you haven't fixed the GList usage. /cvs/gnome/gnome-applets/multiload/main.c : i don't see any diff there, why is it in the patch ? Your patch is OK but it's VERY important to keep the code clean.
K&R style is if (something) { ... ... } don't put braces on a new line...
Created attachment 39602 [details] [review] More stylistic fixes I fixed some whitespace issues, also reverted K&R style to previous state. I'm sorry about all those problems. Hope that's finally clean enough to be committed.
Created attachment 39611 [details] [review] one more patch it's basically the same patch but i got rid of that object_list shit. The new callback doesn't need to be public. davyd > tell if it's OK to have this simple cleanup for gnome-2-10, else i'll split the patch. The less static crap we have ... Btw, multiload-applet now ~works, but there are a LOT of cleanups to do. Baris > you should try Meld, it has a great CVS mode :) This patch should be applied to HEAD and gnome-2-10.
Ok then, whatever style forces us to put braces on new lines. GNU style? Basically, whatever style they were using in Nautilus last time I looked.
Created attachment 39620 [details] [review] patch with original behaviour
Created attachment 39621 [details] [review] /me stupid more spam Sorry, i forgot to removed 1 include :/
ping ?
Created attachment 49447 [details] [review] updated patch
Created attachment 51095 [details] [review] p4tch All of the above patches do strange things to work-around the fact that multiload is dumb about not including a backref to the main structure from the structures for the individual load graphs. Instead of messing around with things like keeping track of timevals, I've decided that this is worth fixing properly. Doing so has removed about 500 lines of code (there were 6 copies of a bunch of stuff that have been consolidated into one place). I've replaced the 'applet' member in each load graph with a backref to the main multiloadapplet structure (the 'g->applet' is now accessible as 'g->multiload->applet'). This allows the direct tracking of what graph has been most recently clicked on (as is required to implement the functionality requested by this bug). When clicked on, the graph follows the backref and sets the 'last_clicked' field on the main struct. If another graph is clicked on later then it sets last_clicked, replacing the previous value. I feel that this is better than each graph keeping track of the time at which it was last clicked and the applet doing a comparison on the timevals to see who wins. Other than the new functionality, the patched code is functionally equivilent to the old code with the added advantage that working with it in the future will be less hacky.
Committed with maintainer approval.