After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 328101 - awesome UI for graphs
awesome UI for graphs
Status: RESOLVED FIXED
Product: system-monitor
Classification: Core
Component: resources
unspecified
Other All
: Normal enhancement
: ---
Assigned To: Karl Lattimer
: 505439 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-01-21 23:31 UTC by Karl Lattimer
Modified: 2011-11-11 10:03 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Mockup of a better layout (37.64 KB, image/png)
2006-01-23 13:33 UTC, Karl Lattimer
  Details
Mockup of a new graph UI (122.29 KB, image/png)
2006-10-26 10:20 UTC, Karl Lattimer
  Details
redesign of the resources tab layout (18.63 KB, patch)
2007-12-06 22:23 UTC, Christophe Dehais
none Details | Review
Before and after - 600 lines height (96.23 KB, image/png)
2007-12-06 22:27 UTC, Christophe Dehais
  Details
Before and after - minimum height (47.68 KB, image/png)
2007-12-06 22:29 UTC, Christophe Dehais
  Details
Visual refresh for graphs (10.01 KB, patch)
2007-12-08 21:21 UTC, Christophe Dehais
none Details | Review
The new graphs in action (80.12 KB, image/png)
2007-12-08 21:22 UTC, Christophe Dehais
  Details
Variations with some tango colors (90.00 KB, image/png)
2007-12-08 21:25 UTC, Christophe Dehais
  Details
One more thing. (93.31 KB, image/png)
2007-12-08 21:29 UTC, Christophe Dehais
  Details
indention with tabs (sadly) instead of spaces. (25.60 KB, patch)
2007-12-08 22:14 UTC, Christophe Dehais
none Details | Review
indention with tabs (sadly) instead of spaces - take2 (9.03 KB, patch)
2007-12-08 22:16 UTC, Christophe Dehais
none Details | Review
screen shot of new graphs UI (53.61 KB, image/png)
2007-12-09 12:22 UTC, Karl Lattimer
  Details
Patch to use graphs similar to those in attachment 75432 (10.29 KB, patch)
2007-12-09 12:28 UTC, Karl Lattimer
committed Details | Review
Proposed renderer for color buttons (5.86 KB, application/x-gzip)
2007-12-11 12:59 UTC, Karl Lattimer
  Details
New mockup, far cleaner and usable for cairo measurements (45.85 KB, image/png)
2007-12-11 20:07 UTC, Karl Lattimer
  Details
changed GSMColorPicker to inherit from GtkDrawingArea (1.55 KB, patch)
2007-12-12 16:43 UTC, Jared Moore
none Details | Review
Working GSMColorPicker renderer, missing network stuff (6.52 KB, application/x-gzip)
2007-12-13 06:54 UTC, Karl Lattimer
  Details
A patch with the new files actually added! (46.70 KB, patch)
2007-12-25 09:25 UTC, Karl Lattimer
committed Details | Review
Screenshot (39.14 KB, image/png)
2007-12-26 13:16 UTC, Toby Dacre
  Details
screen shot of new pie (8.28 KB, image/png)
2007-12-27 12:16 UTC, Karl Lattimer
  Details
background color (13.71 KB, image/png)
2007-12-27 21:41 UTC, Benoît Dejean
  Details
graph min-size (515 bytes, patch)
2007-12-28 01:18 UTC, Toby Dacre
committed Details | Review
screenshot showning new bugs (47.07 KB, image/png)
2008-01-02 20:52 UTC, Toby Dacre
  Details
screenshot showing network graph axis label bugs (61.40 KB, image/png)
2008-01-02 22:43 UTC, Jared Moore
  Details
screenshot showing mixed units (63.71 KB, image/png)
2008-01-03 10:32 UTC, Jared Moore
  Details
patch to stop background redraws (696 bytes, patch)
2008-01-03 14:53 UTC, Toby Dacre
none Details | Review
patch to correct time axis values (2.39 KB, patch)
2008-01-03 15:46 UTC, Toby Dacre
none Details | Review
network rescale update patch (466 bytes, patch)
2008-01-04 13:32 UTC, Toby Dacre
committed Details | Review
patch to stop network scale showing repeated values (961 bytes, patch)
2008-01-04 13:50 UTC, Toby Dacre
rejected Details | Review
fix to smooth jumpy graph (724 bytes, patch)
2008-01-17 19:51 UTC, Toby Dacre
reviewed Details | Review

Description Karl Lattimer 2006-01-21 23:31:19 UTC
Simply looking at system monitor it seems that its not perfect as far as the UI
is concerned. 

When squashing the window down the graphs shown on the "Resources" tab get
squashed too much. 

A simple fix for this would be to move the colour picker buttons to the
left/right of the graph therefore giving a minimum size to the graphs and
allowing the text stats to occupy less space.

Other information:
Comment 1 Benoît Dejean 2006-01-23 11:01:19 UTC
any mockup ? What about the colorpicker labels ?
Comment 2 Karl Lattimer 2006-01-23 13:33:01 UTC
Created attachment 57935 [details]
Mockup of a better layout

Basically all I've done here is compress the user interface putting more emphasis on the graphs, which will no longer become 1px tall because the colour pickers will limit them to a minimum size.
Comment 3 Benoît Dejean 2006-01-23 14:52:51 UTC
what if i have 4 or 8 cpus ?
Comment 4 Karl Lattimer 2006-01-23 15:45:06 UTC
dunno? How about tabling them, putting the current usage for each cpu to the left hand side of the colour as with the memory/network. 

With a table of a max of 2/3/4 rows, and however many columns is appropriate it should work.
Comment 5 Karl Lattimer 2006-10-11 07:52:14 UTC
It occurred to me that you don't need to know which CPU is which colour so having named colour pickers is fairly irrelevant, saving space on the text may allow some better UI wizardry, tooltips could be added for convienience sake.

Another idea was that instead of using the standard colour picker why not simply have a mini colour picker, or a custom widget which fires off the gnome colour picker? 

There are lots of ways the UI could be improved, even having the graphs as a separate tab to the numerical statistics or using some other reveal maybe a graph tooltip which shows the numerical info.
Comment 6 Karl Lattimer 2006-10-26 10:20:40 UTC
Created attachment 75432 [details]
Mockup of a new graph UI
Comment 7 Benoît Dejean 2007-06-27 10:32:52 UTC
WoW, i can't believe i've never seen your UI. I'm very sorry. It's awesome !
Any idea about how to arrange the color picker when they are >= 8 CPUS ?
Comment 8 Karl Lattimer 2007-06-27 10:41:19 UTC
For the colour pickers for >= 8cpu's they should really be stacked up in 2+rows, so for each set of 4 chips/cores one row.

One thing crossed my mind regarding the UI I attached, and that is the graphs are based on the graphs from gnome power manager, I think there should be a concerted effort to create a graphing widget with cairo, rather than re-inventing the wheel for each app. 

I'm already working on one gtk-cairo widget which is pure bling but am hoping to build it into a library I think it may be an idea to include in that library pie charts and graph widgets too, and maybe a boat load of other (useful visual) stuff. Kinda like libsexy with cairo. Could be called Cleopatra :)
Comment 9 Christophe Dehais 2007-12-06 22:22:25 UTC
Funny I completely missed this bug before attempting a redesign. And a gnome-love one !

So I had the same concerns as the initial poster, and a similar idea for solving the problem (namely put the 'legends' to the (right) side).
The proposal has the combined advantages of reducing the minimum size of the application (from 452 to 392 lines on my system) and making things more readable even at this minimum size.

CPUs are stacked in a table of 4 lines and fill the table from top to bottom and left to right so 9 CPUs would look like:
[ ] CPU1:  1%  [ ] CPU5: 7%   [ ] CPU9: 12% 
[ ] CPU2: 10%  [ ] CPU6: 5% 
[ ] CPU3: 19%  [ ] CPU7: 9% 
[ ] CPU4: 37%  [ ] CPU8: 0% 

Hopefully it's a first step toward better usability. The previously attached mockup is really nice, IMHO the default GTK color pickers are just too big and clunky.

Follows patch and screenshots.
Comment 10 Christophe Dehais 2007-12-06 22:23:55 UTC
Created attachment 100479 [details] [review]
redesign of the resources tab layout
Comment 11 Christophe Dehais 2007-12-06 22:27:04 UTC
Created attachment 100480 [details]
Before and after - 600 lines height

Shows the new layout in "normal" use. Graphs have more vertical space and are less stretched.
Comment 12 Christophe Dehais 2007-12-06 22:29:33 UTC
Created attachment 100481 [details]
Before and after - minimum height

Shows what happens on e.g. low resolution devices. New layout is still readable.
Comment 13 Benoît Dejean 2007-12-06 23:55:37 UTC
Why not, but the patch contains too much unrelated/cosmetic changes which obfuscates the change. Like:

-	gtk_box_pack_start (GTK_BOX (cpu_graph_box),
-			    load_graph_get_widget(cpu_graph),
-			    TRUE,
-			    TRUE,
-			    0);
+	gtk_box_pack_start (GTK_BOX (cpu_graph_box), load_graph_get_widget (cpu_graph),
+			    TRUE, TRUE, 0);

Comment 14 Christophe Dehais 2007-12-07 10:29:17 UTC
Sorry about that. I hope this can help:
- I replaced the "spacer" label used for left indentation of graphs by a GtkAdjustment. 
- I removed the "size-request" callback (size_request() function) from the cpu and network labels and registered it to the tables containing the pickers and labels. so that their width is maximized and stays put. I commented that in the code.
- The original size_group applied to these labels is now used for the tables both horizontally (reason explained in a comment) and vertically to have graphs of the same height.
- The rest is just boring UI layout stuff (vboxes become hboxes, etc...).
Comment 15 Thomas D Ahle 2007-12-07 15:35:39 UTC
I think the GTK colorpickers are fine. That they don't fit in here is merely an indication that there is really no reason anybody should ever want to change those colors.
At least it should be a theme thing then.

For the labels next to graph thing, won't it look awkward and be a waste of space, for all those people with only one core?
For them small window it looks nice though.
Comment 16 Christophe Dehais 2007-12-08 21:12:20 UTC
Today I refreshed how the graphs are drawn. It's not a grand design, but I think it shows what can be done when pushing cairo a little.
Result of this work follows.
Comment 17 Christophe Dehais 2007-12-08 21:21:18 UTC
Created attachment 100605 [details] [review]
Visual refresh for graphs

This patch makes the graph a little bit sexier (I hope).
I didn't changed the plotting itself (excpept lower line width a bit) and mainly tweaked the framing, labels and background. They are now cached in a cairo surface that is pasted on the widget on each expose, and only redrawn when widget size changes (previously all framing and labels were draw on each expose, which was suboptimal).
On the visible side of things, the font for the labels is now "sans" instead of "monospace", which I think is more pleasing.
Comment 18 Christophe Dehais 2007-12-08 21:22:37 UTC
Created attachment 100606 [details]
The new graphs in action
Comment 19 Christophe Dehais 2007-12-08 21:25:42 UTC
Created attachment 100607 [details]
Variations with some tango colors

The widget adapts quiet well to different background colors.
To become fully "themable", I guess the LoadGraph object would need to become a real GtkWidget.
Comment 20 Christophe Dehais 2007-12-08 21:29:45 UTC
Created attachment 100608 [details]
One more thing.

Here is another idea: put an icon corresponding to the kind of data that is plotted. This is an inkscape mockup illustrating this, with an icon trying to represent CPU resources and one for the network.
What do you think ?
Comment 21 Benoît Dejean 2007-12-08 21:51:49 UTC
I appreciate your work but i'm not happy with your patches:
- about the layout change: why not (i'm not really sure that this improves anything). Your patch contains too much noise. Instead of writing C i'd rather convert everything to GtkBuilder
- about the graph background: patch contains too much unrelated changes, indentation is inconsistent.
Comment 22 Christophe Dehais 2007-12-08 22:14:30 UTC
Created attachment 100611 [details] [review]
indention with tabs (sadly) instead of spaces.
Comment 23 Christophe Dehais 2007-12-08 22:16:25 UTC
Created attachment 100612 [details] [review]
indention with tabs (sadly) instead of spaces - take2

correct file
Comment 24 Karl Lattimer 2007-12-08 22:17:22 UTC
After looking at the screen shots I must say the design isn't GNOME, it looks more like KDE. 

You should take a look back at my mockup of the UI (mockup of a new graph UI). I spent a long time designing it, and would hope that the usability and visual style improvements over ride your desire to "bling" up the system monitor. 
Comment 25 Christophe Dehais 2007-12-08 22:30:21 UTC
(In reply to comment #21)
> - about the layout change: why not (i'm not really sure that this improves
> anything).
If you can prove the screenshots at small size wrong, more power to you.

> Your patch contains too much noise.
I see we're in the mood of quoting Steve Jobs tonight...

> Instead of writing C i'd rather
> convert everything to GtkBuilder
I completely agree. This would vastly improve code clarity.

> - about the graph background: patch contains too much unrelated changes,
> indentation is inconsistent. 
My mistake, I used spaces instead of tabs to intend my code. It should be corrected now. For the unrelated changes, you must be kidding me. Please look at the code or ask someone else to review it if you're not a developer.
Comment 26 Benoît Dejean 2007-12-08 22:47:13 UTC
Indentation still inconsistent.
Sorry to repeat but the background patch still touches unrelated things. I don't get why you have to rewrite everything: creating the background and then setting it. If you want to improve other things, please split your patch.

Karl > your mockup is still the UI of my dream. I'm not really into bling backgrounds because that doesn't really improve readability and a11y. Your mockup is great because the UI is simple and very readable.
Comment 27 Christophe Dehais 2007-12-08 22:49:57 UTC
(In reply to comment #24)
> After looking at the screen shots I must say the design isn't GNOME, it looks
> more like KDE. 

I thought that too. Maybe I pushed the bling too much. 

> You should take a look back at my mockup of the UI (mockup of a new graph UI).
> I spent a long time designing it, and would hope that the usability and visual
> style improvements over ride your desire to "bling" up the system monitor.  

Well as I said I just did the minimum thing that is at hand right now. I think putting the graph labels to the right addresses the general clarity of the UI.
Your mockup looks great but will take some time to implement: I see at least 2 custom widgets, if I count the cpu color pickers as regular GTK ones.

Moreover, I question two of your side notes on the mockup:
"Quick look at memory usage": isn't the graph already meant to be a quick look view ? (so the pie graphs are just redundant -besides, we have the numerical values that can be glimpsed at quickly)
"Less text same information": well, the total amount of data sent and received need to be accompanied with an explanation (namely the word "Total:"). That's what 's there now.

Comment 28 Christophe Dehais 2007-12-08 23:15:37 UTC
(In reply to comment #26)
> Indentation still inconsistent.

Locate those inconsistencies please. I find it funny to ear that considering the overall poor code quality of the g-s-m code (at least for the 2 files i touched load-graph.cpp and interface.cpp)

> Sorry to repeat but the background patch still touches unrelated things.

Name them please.

> I
> don't get why you have to rewrite everything: creating the background and then
> setting it. If you want to improve other things, please split your patch.

I improvedVVVV changed one thing: the graph "widget", and I obviously can't split my patch because it's an atomic change.
The previous system did not have a background, it was just clearing to black, so that "part" could be integrated in the expose event (thought also integrating the framing and rendering of labels was suboptimal - did I repeat myself?).
Drawing a more complex background takes more time, so it makes sense to cache it when it's not changing. I just did that. And sorry but yes, I like to design my code, so I may, from time to time, have to put some code in what we programmers call a function.


> Karl > your mockup is still the UI of my dream. I'm not really into bling
> backgrounds because that doesn't really improve readability and a11y. Your
> mockup is great because the UI is simple and very readable.
> 

I can't wait for the patches !
Comment 29 Karl Lattimer 2007-12-08 23:46:14 UTC
> "Quick look at memory usage": isn't the graph already meant to be a quick look
> view ? (so the pie graphs are just redundant -besides, we have the numerical
> values that can be glimpsed at quickly)

Quick look at CURRENT memory usage. the pie charts give a better general readout, especially when accompanied with the relevant text. The idea here is not to have a moving target for the eyes to track.

> "Less text same information": well, the total amount of data sent and received
> need to be accompanied with an explanation (namely the word "Total:"). That's
> what 's there now.

My point here was two fold, Received is meaningless, as is sent, sending and receiving are present tense (correct grammar), also using Total is ambiguous, Total of what? Sending, sent it makes no sense! So here too the grammar is incorrect, and therefore there is no harm in removing it. 

My advice here is instead of spending your time and ours in reviewing your changes you look at the existing work that has been done and implement it. It wouldn't be _THAT_ difficult to use the graph design that I laid down, at least it fits with GNOME. 

Following that, the widgets for the other elements can be done over time. The most important point about this bug is getting the graphs to have a minimum height and be more readable, green on black is NOT readable, the only reason it is used is because all oscilloscopes used to be green on black, its traditional not readable. 

You've now submitted 4 patches all of which are garbage as far as I can see, they implement something different than is in the bug comments. You seem to have taken this in your own direction without a thought to who's toes you're stepping on and the amount of work you're creating for yourself.
Comment 30 Benoît Dejean 2007-12-09 00:51:01 UTC
(In reply to comment #28)
> (In reply to comment #26)
> > Indentation still inconsistent.
> 
> Locate those inconsistencies please. I find it funny to ear that considering
> the overall poor code quality of the g-s-m code (at least for the 2 files i
> touched load-graph.cpp and interface.cpp)
>

two immediate examples:

 	cairo_surface_t *buffer;
-
+        cairo_surface_t *background;

and

+#define FRAME_WIDTH     6
+#define FONT_SIZE   11
 
-#define FRAME_WIDTH 4

or all the thing about num_bars, etc


> > Sorry to repeat but the background patch still touches unrelated things.
> 
> Name them please.
> 
> > I
> > don't get why you have to rewrite everything: creating the background and then
> > setting it. If you want to improve other things, please split your patch.
> 
> I improvedVVVV changed one thing: the graph "widget", and I obviously can't
> split my patch because it's an atomic change.
> The previous system did not have a background, it was just clearing to black,
> so that "part" could be integrated in the expose event (thought also
> integrating the framing and rendering of labels was suboptimal - did I repeat
> myself?).
> Drawing a more complex background takes more time, so it makes sense to cache
> it when it's not changing. I just did that. And sorry but yes, I like to design
> my code, so I may, from time to time, have to put some code in what we
> programmers call a function.
>

That's enough for me.
Comment 31 Thomas D Ahle 2007-12-09 09:51:27 UTC
@Christophe Dehais
> I see at least 2 custom widgets, if I count the cpu color pickers as regular
> GTK ones.
To me the color squares doesn't seam like pickers. And I honestly don't understand why they should be so.
If people want to change the look of their desktop they go to preferences->themes. They shouldn't have to personally decide any color in any app. It only makes more clutter.

@Karl Lattimer
> You've now submitted 4 patches all of which are garbage as far as I can see,
> they implement something different than is in the bug comments. You seem to
> have taken this in your own direction without a thought to who's toes you're
> stepping on and the amount of work you're creating for yourself.
That's how it is. He writes it, he decides. ;)
And it seams the graph has been made much more flexible, so it shouldn't be too hard to style more gnomy.
Comment 32 Karl Lattimer 2007-12-09 12:22:36 UTC
Created attachment 100636 [details]
screen shot of new graphs UI
Comment 33 Karl Lattimer 2007-12-09 12:28:39 UTC
Created attachment 100637 [details] [review]
Patch to use graphs similar to those in attachment 75432 [details]

Summary of changes
 * moved declaration of caption and y to the top of the function they were defined inside of the for loop, which I think is bad.
 * Changed the graph to draw using black and white and theme colours
 * Created a simple dash for all graph lines
 * Changed the font and font size and rejiggered the calculation of num_bars
 * Set the text alignment to right 
 * Added 7 vertical lines between the graph edges
 * set the minimum height to 80px, more space can be regained later on by re-widgeting the colour pickers. 
 * Removed references to the old background/foreground colour definitions and re-aligned the colour array
 * Added myself to the authors (at the request of Benoit)
Comment 34 Benoît Dejean 2007-12-09 13:35:25 UTC
Go on Karl, you now have super-cow powers.
Please just change these declaration things we talked about.
Comment 35 Christophe Dehais 2007-12-09 14:49:54 UTC
(In reply to comment #29)
> Quick look at CURRENT memory usage. the pie charts give a better general
> readout, especially when accompanied with the relevant text. The idea here is
> not to have a moving target for the eyes to track.

First I consider my code as a "live mockup", not done with inkscape or gimp but with cairo and gtk. Some things are more easily caught with such an approach, and e.g. here you would see immediately on _real_ memory graph that the dynamics is quite slow, which mean that the current memory usage is NOT a moving target. Most of the time those graphs are FLAT.
 
> My point here was two fold, Received is meaningless, as is sent, sending and
> receiving are present tense (correct grammar), also using Total is ambiguous,
> Total of what? Sending, sent it makes no sense! So here too the grammar is
> incorrect, and therefore there is no harm in removing it. 

Well I could also ask, when presented with "200Mb" alone, "200Mb of what ?" so basically you can't just remove things and expect users to guess the missing part. The network labels as they are now are not that verbose.


> My advice here is instead of spending your time and ours in reviewing your
> changes you look at the existing work that has been done and implement it. It
> wouldn't be _THAT_ difficult to use the graph design that I laid down, at least
> it fits with GNOME. 

I will spend my time at my own discretion, if you allow me. If analysing the disctibution of the '+' and '-' of the first column of a patch file is called reviewing, well sure some time has been wasted.

> Following that, the widgets for the other elements can be done over time. The
> most important point about this bug is getting the graphs to have a minimum
> height and be more readable, green on black is NOT readable, the only reason it
> is used is because all oscilloscopes used to be green on black, its traditional
> not readable. 

Maybe the title of the bug should be changed to reflect that then.

> You've now submitted 4 patches all of which are garbage as far as I can see,
> they implement something different than is in the bug comments.

Garbage ? Come on, I'm sure you don't want to go this road.

> You seem to
> have taken this in your own direction without a thought to who's toes you're
> stepping on and the amount of work you're creating for yourself.

I suggest you (re)read the initial comment carefully and tell me that my first patch doesn't exactly address what's suggested.
Yes, I started working before looking if any bug were filed, which is obviously an error, but I did all this for fun and I don't expect anything from this (I only launch this g-s-m tab myself once in a while so that's not really a problem for me).

Comment 36 Christophe Dehais 2007-12-09 15:26:25 UTC
(In reply to comment #34)
> Go on Karl, you now have super-cow powers.
> Please just change these declaration things we talked about.
> 

So when a 20-points bugzilla reporter talks to a 6-points bugzilla reporter, the 6-points reporter just need to shut up. I would be pleased to see the IRC excerpt where you both laughed at me.

To conclude my participation here, I'll say that I'm pleased to see that my nut-breaker attitude allowed the production of some actual code for this nearly 2-years old bug. The result of Karl's patch is really good, and IMHO it's the right balance between candy-ness and clarity.

Some comments about the patch:
 * coding style: is this foo (bar) or foo(bar) ?
 * the offset for 100% and 0% is not to +/-fontsize/2 but half the height extent of the strings "100%" and "0%". See e.g. my patch.
 * you don't need %3d anymore with a "sans" font, %d is sufficient.
 * you really need to think about deferring all static drawings (labels and graph frame) to the configure event and just paste and draw the plot in the expose event.
 * the calculation of num_bars is simply ugly (and the switch structure is formatted par dessus la jambe). Please consider the solution I proposed in my patch.
 * I found that setting line_width to 1.5 instead of 2 made the plots little clearer. Please consider that.
 * please don't forget the initial idea of putting legends to the right: I think it renders setting any minimum size useless (the overall size is then constrained by the other tabs)

By the way the above points is what I call a review.


gnome-love everyone.
Comment 37 Karl Lattimer 2007-12-09 15:40:48 UTC
> First I consider my code as a "live mockup", not done with inkscape or gimp but
> with cairo and gtk. Some things are more easily caught with such an approach,
> and e.g. here you would see immediately on _real_ memory graph that the
> dynamics is quite slow, which mean that the current memory usage is NOT a
> moving target. Most of the time those graphs are FLAT.

any horizontal movement within the focal plane of the eye causes fatigue. So for instance if you start an app and have a large jump in the line this causes the eye to partially follow that blip while still attempting to identify the current value. 

> I will spend my time at my own discretion, if you allow me. If analysing the
> disctibution of the '+' and '-' of the first column of a patch file is called
> reviewing, well sure some time has been wasted.

And the time wasted reading your emails, reading through your massive patches and trying to understand exactly why you went the direction you did.

> Maybe the title of the bug should be changed to reflect that then.

The original title of the bug was system monitor graphs get squashed too much, in fact when I posted the bug originally it was because the graphs got squashed to 0px in height.

The bug was renamed to reflect the new direction of the bug after I posted my carefully thought out mockup.

> Garbage ? Come on, I'm sure you don't want to go this road.

Garbage! The sheer number of lines in your patch, ignorance of coding guidelines, ignorance of prior work, hell for leather attitude and accusations to Benoit of being incapable of reviewing a patch "Please look at the code or ask someone else to review it if you're not a developer." and "And sorry but yes, I like to design my code, so I may, from time to time, have to put some code in what we programmers call a function."

As far as I'm concerned your reactions have been nothing but arrogant and ignorant. Benoit has done a brilliant job maintaining gsm over the years you should have realised that before making such an insulting comment.

> I suggest you (re)read the initial comment carefully and tell me that my first
> patch doesn't exactly address what's suggested.

I suggest you; 
a) stop suggesting I read what I've written
b) read the follow up comments rather than thinking the initial posting is what the solution should be, the comments are there to discuss the bug and derive a solution many people will be happy with
c) read http://live.gnome.org/CodeOfConduct pay close attention to the first point 
d) learn how to write small clean patches 
e) learn how to take constructive criticism without firing back abuse

btw: what road would that be that I don't want to go down?
Comment 38 Christophe Dehais 2007-12-09 15:56:31 UTC
(In reply to comment #37)
> c) read http://live.gnome.org/CodeOfConduct pay close attention to the first
> point 

Ok will you and Benoît please accept my apologies. I've been arrogant (I knew I was right while I was writing my comments, so that's bad). And I understood that I wrote my patches the wrong way but I was excited to do this, so I throw myself at the code and then I was too lazy to rewrite the incremental way.

This is not an excuse for my attitude but I felt like having been victim of the third point of the above mentioned Code of Conduct.

Comment 39 Martin Ejdestig 2007-12-10 20:04:30 UTC
This looks nice. I've been thinking about making the graphs look more like the ones in GNOME Power Manager (or should I say, open a bug about it).

Anyways, I had a quick look at the patch and couldn't see any change related to the default graph colors to look more like what's in attachment 100636 [details]. The current default colors won't look good against a white background. They are also horrible in other ways (and I'm in part responsible for that :/ ... I would feel much better if you fixed it :). Using a theme color as background complicates things even more.
Comment 40 Louis-Francis Ratté-Boulianne 2007-12-10 20:35:26 UTC
I would suggest that the arrows in the mockup from Comment #6 should be downward for Receiving (like in Download) and upward for Sending (like in Upload).

Also, I think you should keep "Total:" or something similar because it's hard to figure out what means 200 Mb and 40 Mb alone.
Comment 41 Karl Lattimer 2007-12-11 12:55:54 UTC
Comment #39 : Martin, I agree completely, I haven't gotten around to doing this, or choosing the colors or how I may calculate the colors. I think the defaults for the CPU need to be figured out somehow currently this is a slight shift in brightness, I want to use a change in hue instead, by manipulating the hue by a specific amount relative to how many cpus there are there is a significant contrast between cpus, I'll then use a constant saturation and lightness so there is a consistent feel to the colors.

Comment #40: Agreed, vertical arrows are far more descriptive, I am considering using an SVG icon for this also, and setting the base color in cairo. While still allowing the user to select a color. (See later attached zip file)
Comment 42 Karl Lattimer 2007-12-11 12:59:11 UTC
Created attachment 100761 [details]
Proposed renderer for color buttons

This is a custom widget template I'm trying to get working (example/makefile included). This will allow me to get all of the various color buttons working through a single widget, however I'm getting a spurious segfault, if anyone could offer any insight I'd be most grateful.

This widget will be responsible for all of the cpu/pie/network color buttons, its derrived from GtkColorButton but has a variety of required variations and changes. 

The current segfault seems to be coming from gsm_color_button_set_color
Comment 43 Martin Ejdestig 2007-12-11 14:39:19 UTC
Gtk-ERROR **: file /build/buildd/gtk+2.0-2.12.0/gtk/gtkwidget.c: line 7964 (gtk_widget_real_realize): assertion failed: (GTK_WIDGET_NO_WINDOW (widget))
Comment 44 Martin Ejdestig 2007-12-11 14:45:40 UTC
(Sorry about pressing save changes too soon.)

I had a quick look at gsm_color_button_set_color() but couldn't come up with anything helpful to say so I thought I'd try running it in gdb. I hit the assertion above before getting that far though...
Comment 45 Martin Ejdestig 2007-12-11 14:59:42 UTC
GTK_WIDGET_NO_WINDOW(): Evaluates to TRUE if the widget doesn't have an own GdkWindow.
Comment 46 Karl Lattimer 2007-12-11 20:07:35 UTC
Created attachment 100779 [details]
New mockup, far cleaner and usable for cairo measurements

This is what I'm going for, all I really need to do is render an svg on top of the colour of the download buttons, i draw the colour fill underneath with cairo.

The sizing is 32px tall for the icons and the pie charts. The whole window is quite a bit smaller than currently.

**karl toddles off to mess with GSMColorPicker
Comment 47 Karl Lattimer 2007-12-12 12:59:20 UTC
 I just wanted to make a short list of things I need to do to get this fixed, and co-ordinate my work with bugzilla, I'm going to be working from my laptop from friday so I need a central location :)

 * Move the notebook identification loop into load_graph_new so its only done once per graph, rather than every redraw (this is a waste of cpu time)
 * Look into making the graphs render 1 segment behind in order to render them early and put them on screen more often -- caching the cairo surface for the graph lines & smooth scrolling.
 * Identify the correct theme colours to use in the graph lines/background, the colours should be dark + light, matching the background/foreground of a treeview widget.
 * Figure out how to calculate the default colours and set appropriate defaults
 * Complete GSMColorPicker to conform with the diagrams in attachment 100779 [details]
Comment 48 Thomas D Ahle 2007-12-12 14:40:47 UTC
When you write Kbps do you mean kilobit or kilobyte?
Kb naturally matches the standard provider-description better, but nearly all other applications use KB..
Comment 49 Karl Lattimer 2007-12-12 15:24:16 UTC
I think in this instance as it is a system monitor and not a download client the user would prefer to see how many bps are being downloaded, so they can comparatively see their current download speed against what they are expected to achieve from their ISP. 
Comment 50 Jared Moore 2007-12-12 15:49:59 UTC
Hmm, I saw this bug around 6 months ago and I just decided I might work on it, but it seems I've been beaten by only a few days! grr :P I guess I'll have to work on something else instead.

Anyway, if I could just at my $0.02, the UI in comment 46 is really great, but it's really not obvious in the middle graph that the purple line is user memory and the yellow line is swap. Perhaps a text label could be added next to the pie charts. :)

*ducks back into his hole*
Comment 51 Karl Lattimer 2007-12-12 15:56:34 UTC
Jared if you'd like to help please take a look at GSMColorPicker, this is crash happy and I'm getting frustrated, I need another set of eyes to get me past the issue.

WRT text beside icons for the pie graphs, sure that makes sense :)
Comment 52 Jared Moore 2007-12-12 16:39:50 UTC
I think I may have found the problem. Seems like instead of inheriting from GtkWidget, you should inherit from GtkDrawingArea. Have a look here -> http://lists.ximian.com/pipermail/gtk-sharp-list/2006-March/006997.html

Comment 53 Jared Moore 2007-12-12 16:43:00 UTC
Created attachment 100837 [details] [review]
changed GSMColorPicker to inherit from GtkDrawingArea

Tested, seems to work :)
Comment 54 Karl Lattimer 2007-12-12 16:57:50 UTC
Tested too, brilliant!

however things are not as I had planned, I shall look into this more methinks... but now at least I'm not too far from a working widget :)
Comment 55 Karl Lattimer 2007-12-13 06:54:23 UTC
Created attachment 100869 [details]
Working GSMColorPicker renderer, missing network stuff

Needs work;
 * Improve pie highlight
 * Create a trimmed SVG missing the background colour for network button
 * Note coordinates for arrow points in SVG source
 * Write the network colour button based on the above two points
 * Add a gsm_color_button_pulse_network public method, makes the network indicator pulse when there is network activity above a certain threshold (5% of current, and above 5KB/s for example)
Comment 56 Karl Lattimer 2007-12-13 08:40:55 UTC
Created SVG files and worked out the coordinates for the arrow. Writing the network colour button won't be difficult :) Cairo practically writes itself!

For the pie highlight some work is required to calculate the radians per pixel wide at both the max radius and the min radius. Min radius being the avoidance/repositioning of the internal centre line. (Graph paper to the rescue!)

Next up is adding the pulse method, this will require a timer to switch it on/off. If it hasn't fired in side of the time period it should then take 5 frames at 20ms appart to fade out giving another 100ms to be activated, activation will be a incremental rise from the current brightness adjustment. This shouldn't take too much CPU and is not a feature I intend to use before gnome 2.22 so if necessary it can wait.

After I'm satisfied with this widget it should be trivial to add it into gnome-system-monitor just add the includes and the files and adjust the makefile :)

Comment 57 André Klapper 2007-12-13 12:59:25 UTC
karl, gnome-target is meant for bugs severe enough to block a gnome release. i don't think that is the case here.
please ask the maintainer to add a target milestone instead, if required. thanks.
Comment 58 Karl Lattimer 2007-12-13 13:05:23 UTC
oops sorry
Comment 59 Toby Dacre 2007-12-24 19:57:14 UTC
This has been an interesting bug report! I had not looked at it since comment #13 and things have picked up.

Karl, Your UI has moved on loads from the first draft, so much cleaner.  The white backgrounds on the graph is both simple and genius.  And congratulations on becoming a developer on system-monitor.

Benoît, you are a patient man ;)

my observations;

* The csv trunk is broken and won't compile. missing pixmaps (upload.svg, download.svg) and also the GSMColorPicker stuff related to these changes.  I'm sure this is just temporary.

* There is an inconsistency with the keys/colours on the graphs in this new design.  It would be nice IMHO if the colour was a block (as for CPU) as when there is a 0% or 100% the colour is not visible in the pie chart and not apparently shown in the upload/download.  The pie charts are a nice touch but unclear which colour is used/unused. I favour the colour block being just an indicator of just the colour of the line in the graph.  Extra information such as which CPU, Memory Type, network etc could remain in text.

* Future proofing: we need to think of multi core/processor servers with many network cards etc.  At work I have 16 Core servers with 2 NICs (nothing special).  This will increase over time.  The solutions need scalability.

Anyway, I can't wait for the svn to be fixed as I want to see how the graphs look live.  I did a patch about 6 months ago and have been playing with vertical/time lines in the graphs but was never happy with the results, plus how to deal with update time changes etc.  I'd like to see what Karl has done and see how it compares.

Keep up the good work
Comment 60 Karl Lattimer 2007-12-25 09:25:58 UTC
Created attachment 101570 [details] [review]
A patch with the new files actually added!

Toby, thanks for the bug report. I seem to have forgotten to add the files to svn! Unfortunately I haven't got my dsa key with me as I'm now in prague.

Attached is the patch to include the correct files, Benoit can you please check this in to svn ASAP. 

Merry xmas everybody :)
Comment 61 Benoît Dejean 2007-12-25 19:51:00 UTC
*** Bug 505439 has been marked as a duplicate of this bug. ***
Comment 62 Benoît Dejean 2007-12-25 20:40:38 UTC
I've fixed SVN.
Comment 63 Toby Dacre 2007-12-26 12:37:51 UTC
Cool, Got it working now :)

I've got a few small bug fixes, however I can't see how to attach the patches so I'll have to just paste them into the comments box.  Sorry about this.

I'll split them up to make it easier to follow
Comment 64 Toby Dacre 2007-12-26 12:45:55 UTC
OK this is confusing me I tried to add an attachment and it added it to a different bug.  Oh well I'll try again

first bug

The vertical lines are not evenly spaced due to typo


PATCH

diff -u ./svn2/trunk/src/load-graph.cpp ./sys-mon/trunk/src/load-graph.cpp
--- ./svn2/trunk/src/load-graph.cpp     2007-12-26 12:12:32.000000000 +0000
+++ ./sys-mon/trunk/src/load-graph.cpp  2007-12-26 11:59:38.000000000 +0000
@@ -173,7 +173,7 @@
        cairo_set_dash (cr, dash, 2, 1.5);
 
        for (unsigned i = 0; i < 7; i++) {
-               double x = (i + 1) * (g->draw_width - rmargin + indent) / 8;
+               double x = (i + 1) * (g->draw_width - rmargin - indent) / 8;
                cairo_move_to (cr, (ceil(x) + 0.5) + rmargin + indent, 0.5);
                cairo_line_to (cr, (ceil(x) + 0.5) + rmargin + indent, real_draw_height + 0.5);
        }
Comment 65 Toby Dacre 2007-12-26 12:48:55 UTC
Next, the CPU load graph lags behind the others as no load is recorded the first time get_load() is called.

There is also the g->cpu.initialized variable that is never used.


PATCH

diff -u ./svn2/trunk/src/load-graph.cpp ./sys-mon/trunk/src/load-graph.cpp
--- ./svn2/trunk/src/load-graph.cpp     2007-12-26 12:12:32.000000000 +0000
+++ ./sys-mon/trunk/src/load-graph.cpp  2007-12-26 12:15:54.000000000 +0000
@@ -68,7 +68,6 @@
 
        /* union { */
                struct {
-                       gboolean initialized;
                        guint now; /* 0 -> current, 1 -> last
                                    now ^ 1 each time */
                        /* times[now], times[now ^ 1] is last */
@@ -296,10 +295,6 @@
                }
        }
 
-       if (G_UNLIKELY(!g->cpu.initialized)) {
-               /* No data yet */
-               g->cpu.initialized = TRUE;
-       } else {
                for (i = 0; i < g->n; i++) {
                        float load;
                        float total, used;
@@ -316,7 +311,6 @@
                        gtk_label_set_text(GTK_LABEL(g->labels.cpu[i]), text);
                        g_free(text);
                }
-       }
 
        g->cpu.now ^= 1;
 
Comment 66 Toby Dacre 2007-12-26 12:53:04 UTC
Finally,

This patch includes the 2 above and also some indentation fixes

the before after shot is at

http://bugzilla.gnome.org/attachment.cgi?id=101617&action=view

here is the full patch sorry they are all inline

Toby



diff -u ./svn2/trunk/src/load-graph.cpp ./sys-mon/trunk/src/load-graph.cpp
--- ./svn2/trunk/src/load-graph.cpp     2007-12-26 12:12:32.000000000 +0000
+++ ./sys-mon/trunk/src/load-graph.cpp  2007-12-26 12:20:15.000000000 +0000
@@ -68,7 +68,6 @@
 
        /* union { */
                struct {
-                       gboolean initialized;
                        guint now; /* 0 -> current, 1 -> last
                                    now ^ 1 each time */
                        /* times[now], times[now ^ 1] is last */
@@ -173,7 +172,7 @@
        cairo_set_dash (cr, dash, 2, 1.5);
 
        for (unsigned i = 0; i < 7; i++) {
-               double x = (i + 1) * (g->draw_width - rmargin + indent) / 8;
+               double x = (i + 1) * (g->draw_width - rmargin - indent) / 8;
                cairo_move_to (cr, (ceil(x) + 0.5) + rmargin + indent, 0.5);
                cairo_line_to (cr, (ceil(x) + 0.5) + rmargin + indent, real_draw_height + 0.5);
        }
@@ -296,26 +295,21 @@
                }
        }
 
-       if (G_UNLIKELY(!g->cpu.initialized)) {
-               /* No data yet */
-               g->cpu.initialized = TRUE;
-       } else {
-               for (i = 0; i < g->n; i++) {
-                       float load;
-                       float total, used;
-                       gchar *text;
-
-                       total = NOW[i][CPU_TOTAL] - LAST[i][CPU_TOTAL];
-                       used  = NOW[i][CPU_USED]  - LAST[i][CPU_USED];
-
-                       load = used / MAX(total, 1.0f);
-                       g->data[0][i] = load;
-
-                       /* Update label */
-                       text = g_strdup_printf("%.1f%%", load * 100.0f);
-                       gtk_label_set_text(GTK_LABEL(g->labels.cpu[i]), text);
-                       g_free(text);
-               }
+       for (i = 0; i < g->n; i++) {
+               float load;
+               float total, used;
+               gchar *text;
+
+               total = NOW[i][CPU_TOTAL] - LAST[i][CPU_TOTAL];
+               used  = NOW[i][CPU_USED]  - LAST[i][CPU_USED];
+
+               load = used / MAX(total, 1.0f);
+               g->data[0][i] = load;
+
+               /* Update label */
+               text = g_strdup_printf("%.1f%%", load * 100.0f);
+               gtk_label_set_text(GTK_LABEL(g->labels.cpu[i]), text);
+               g_free(text);
        }
 
        g->cpu.now ^= 1;
Comment 67 Toby Dacre 2007-12-26 13:16:00 UTC
Created attachment 101619 [details]
Screenshot

OK I'm just stupid ;)

Back to comment #59 I do think that the display is more informative and cleaner with just solid colour pickers.  Here is a screen shot.  It also takes up less vertical space which is good.  Let me know your thoughts
Comment 68 Benoît Dejean 2007-12-26 14:31:18 UTC
The indentation fix are not needed.

Of course, the CPU is starting 1-shot later because 2 samples are required to get a value.

First patch looks OK.
Comment 69 Toby Dacre 2007-12-26 15:31:15 UTC
OK, I should look at the code more :)

As an alternative how about delaying the other graphs by 1-shot so they all line up.  I can do a patch if needed.  Should this move to a new bug?
Comment 70 Karl Lattimer 2007-12-27 10:02:20 UTC
Hi Toby, 

thanks for the patches. 

I'd rather not remove the pie charts yet... I have a cunning plan I came up with today, I'll hack a patch and attach it for you to test later on, this will solve the 0% swap issue and neaten things up somewhat.

You see the pies and their size for this is two fold:

 * The pie is supercool for display and allows a quick look at the status
 * The pie creates adequete vertical spacing, without it looking chunky to split the text on to two lines, and have both memory and swap side by side.

Similar reasoning applies to the network buttons too.

Your patches otherwise look great and will be excited to test them, what we really need is the enter/leave/motion events working, so what can we do about this? I believe we need to set some kind of gdk motion mask up or something... I'm going to try and fix that today, but any patches are welcome :)
Comment 71 Benoît Dejean 2007-12-27 10:11:44 UTC
(In reply to comment #69)

> As an alternative how about delaying the other graphs by 1-shot so they all
> line up.  I can do a patch if needed.  Should this move to a new bug?

If you think it's really important for the graph to be aligned, it would be very easy to set the graph start to be the average load since boot.

Comment 72 Karl Lattimer 2007-12-27 10:21:13 UTC
(In reply to comment #71)
> (In reply to comment #69)
> 
> > As an alternative how about delaying the other graphs by 1-shot so they all
> > line up.  I can do a patch if needed.  Should this move to a new bug?
> 
> If you think it's really important for the graph to be aligned, it would be
> very easy to set the graph start to be the average load since boot.
> 

This is actually going to be pretty important anyway for a caching method I'm working on which goes something like this...

For each sample of data we take, we shift by one a ring buffer and render a cairo path and a surface, we have a surface stored from the expose event with the graph background on it as Christophe Dehais suggested and here's the clever bit :)

because all the graphs are 1-shot behind, we put out 10 frames per second where the graphs are moving 1px at a time, the surfaces can be easily clipped and composited together much faster than doing the rendering and as we only need to render it once per second or so the heavy lifting is quite a bit lighter because we've pushed the graph backgrounds to expose, then we're mashing it together again at a smoother frame rate, this should drop the cpu usage for graphs and improve the bling factor by force 10 :)
Comment 73 Benoît Dejean 2007-12-27 12:11:57 UTC
OK i do it tonight.
Comment 74 Karl Lattimer 2007-12-27 12:16:53 UTC
Created attachment 101667 [details]
screen shot of new pie

Here's my idea for the pie, by doing this we can skip duplication of the percentage. Maybe we could even add more text including some other memory statistics.
Comment 75 Karl Lattimer 2007-12-27 13:09:54 UTC
I've just added highlight when the mouse enters the widget, and I've also added a tooltip which will need translating.

I'll upload this patch once I've cleaned it up a bit.

Benoit are you happy with the screenshot in attachment 101667 [details] for the pies? I think I've achieved a good parity between clarity, functionality and design here.
Comment 76 Benoît Dejean 2007-12-27 13:29:51 UTC
I don't know, it's a kind of progress bar but the text is much more smaller. If the text is really needed, then the pie is not really useful anymore.
Comment 77 Karl Lattimer 2007-12-27 13:37:36 UTC
I see your point, I arrived at this by adding a central disc to ensure that some of the colour was always visible. The text may just be a step too far, I think I'll keep the inner disc, make it smaller and remove the text.
Comment 78 Martin Ejdestig 2007-12-27 14:52:45 UTC
(In reply to comment #76)
> I don't know, it's a kind of progress bar but the text is much more smaller. If
> the text is really needed, then the pie is not really useful anymore.

Hummm... this might have an impact on a11y...
Comment 79 Teppo Turtiainen 2007-12-27 16:09:14 UTC
(In reply to comment #74)
> Here's my idea for the pie, by doing this we can skip duplication of the
> percentage. Maybe we could even add more text including some other memory
> statistics.

Why does the pie run counterclockwise? Is that customary for pie charts? Also, I think it would look nicer if all the color buttons had the same height and the graphs were thus evenly spaced.
Comment 80 Benoît Dejean 2007-12-27 19:07:18 UTC
I've commited:
- the - indent fix.
- no init delay for the cpu graphs

Am i the only to feel that the graph background is not exactly the same color than the other gtk widgets ?
Comment 81 Karl Lattimer 2007-12-27 19:37:52 UTC
The graph background is determined from the bg style property of the GtkNotebook, can you screen shot it? I tested with all the themes I had installed and it worked fine for me...

Comment 82 Benoît Dejean 2007-12-27 21:41:04 UTC
Created attachment 101697 [details]
background color
Comment 83 Karl Lattimer 2007-12-27 21:57:44 UTC
i just checked this out, and it definitely has a slight variance of colour. However this isn't a problem with gnome-system-monitor the determination of the background colour from the GtkNotebook if you look at the color buttons the variance is still visible around those and they do no clear the background, they simply draw straight on top of it. So this leads me to believe its an issue of colour matching between cairo and X somewhere.

Maybe an upstream bug or at least a post to the relevant mailing list should be made.
Comment 84 Toby Dacre 2007-12-28 01:13:25 UTC
Looking good!

I still think that the pie chart/colour block thing needs resolving.  As you know I favour just the colour blocks.  I think they make for a more unified look and help on the vertical space problem.  Karl, I realise that you want to make the info be spread over 2 lines and this may be a solution, but I think it would still end up being a distraction to the information. The pie charts are nice but is sys-monitor the place for such bling?  I'm not sure.  Now the smooth scrolling graph that you mentioned in comment #72, especially if less CPU work, is the sort of 'bling' I would want.

I've been thinking about the vertical space issue and apart from the CPU info I think it will be hard to put any of the other info on less than one whole line. I think there may be an accessibility issue with it too for people with large font sizes.  I'm thinking maybe some more radical move like move the network graph to it's own tab.  That way we have more space to play with network information especially as more than one NIC is common now.  I think this will also help with computers with silly numbers of CPU/cores.




Comment 85 Toby Dacre 2007-12-28 01:18:40 UTC
Created attachment 101714 [details] [review]
graph min-size

On the vertical size I'd like to see GRAPH_MIN_HEIGHT set back to 40 if people want squashed up graphs let them :)

The other bit  
+       case 0
is just to stop all the horizontal bars getting displayed if you have very small graphs ie set GRAPH_MIN_HEIGHT to 10 like I did this is not a current bug.
Comment 86 Benoît Dejean 2007-12-28 08:09:52 UTC
(In reply to comment #84)
> Looking good!
> 
> I still think that the pie chart/colour block thing needs resolving.  As you
> know I favour just the colour blocks.  I think they make for a more unified
> look and help on the vertical space problem.  Karl, I realise that you want to
> make the info be spread over 2 lines and this may be a solution, but I think it
> would still end up being a distraction to the information. The pie charts are
> nice but is sys-monitor the place for such bling?  I'm not sure. 

Question is "is it readable ?". I think the current one is better and that putting text in the pie chart adds inconsistency with the other color buttons / labels. Unless we move everything to pies.

> Now the
> smooth scrolling graph that you mentioned in comment #72, especially if less
> CPU work, is the sort of 'bling' I would want.
> 
> I've been thinking about the vertical space issue and apart from the CPU info I
> think it will be hard to put any of the other info on less than one whole line.
> I think there may be an accessibility issue with it too for people with large
> font sizes.  I'm thinking maybe some more radical move like move the network
> graph to it's own tab.  That way we have more space to play with network
> information especially as more than one NIC is common now.  I think this will
> also help with computers with silly numbers of CPU/cores.
> 

Moving something to a tab doesn't move it to another CPU :)
On my 1GHZ laptop, the graph with 1s update takes ~25% of CPU.
Comment 87 Toby Dacre 2007-12-28 14:32:00 UTC
(In reply to comment #86)
> Question is "is it readable ?". I think the current one is better and that
> putting text in the pie chart adds inconsistency with the other color buttons /
> labels. Unless we move everything to pies.

Consistency is good.  I still prefer colour squares, but they should be the same for cpu/memory/network whether pie/square/whatever.

> 
> Moving something to a tab doesn't move it to another CPU :)
> 

I know this. Moving the network stuff to it's own tab would give more room to display info/graphs. Maybe add IP/MAC address etc at some point.

Comment 88 Toby Dacre 2008-01-02 20:52:12 UTC
Created attachment 102014 [details]
screenshot showning new bugs

Hi,

I've just compiled rev 2235.  Some observations (plus see screenshot)

1) the graphs are now using much more CPU to be displayed.  Approx twice what is was at the default settings. over 50%.

2) There is a bug which I don't think I've seen before (see screenshot). The graph line goes below 0.  I can reliably recreate this by switching g-s-m to the system tab.  Close g-s-m.  Open g-s-m.  Wait ~10 seconds.  switch to resources tab. 

3) The time scale is incorrect. When the resources update is changed to >= 2.00 or <= 0.75 the time axis give spurious numbers (again see screenshot) these can be negative or more than one identical value in several places.

4) following on from this the scale is not accurate.  at 1.5 sec update rate a 10 second period on the graph lasts ~19 seconds.

5) The text on the graph scales is fixed size. I think that may just be acceptable for 100% etc but when the information is actually needed (and changes) then accessibility is an issue here.


I hope this helps.
Comment 89 Jared Moore 2008-01-02 22:43:14 UTC
Created attachment 102019 [details]
screenshot showing network graph axis label bugs

Also, regarding the network graph, there has been some work done at bug 418181. Unfortunately there have been so many changes to HEAD that the patch probably doesn't apply cleanly now. 

Anyway, there are some problems with the current revision:

1) All labels are 1 byte/s when no data has been logged

2) The bottom label is non-zero, and furthermore it should just be "0", not "0 bytes/s"

3) The traditional units for network speeds is bits/second with power of 10, not Bytes/second with powers of 2

4) In some cases there are mixed units in the labels (e.g. some KiB and some B, see screenshot)
Comment 90 Benoît Dejean 2008-01-02 23:07:36 UTC
(In reply to comment #85)
> Created an attachment (id=101714) [edit]
> graph min-size
> 
> On the vertical size I'd like to see GRAPH_MIN_HEIGHT set back to 40 if people
> want squashed up graphs let them :)
> 
> The other bit  
> +       case 0
> is just to stop all the horizontal bars getting displayed if you have very
> small graphs ie set GRAPH_MIN_HEIGHT to 10 like I did this is not a current
> bug.
> 

Integrated.
Comment 91 Benoît Dejean 2008-01-02 23:20:30 UTC
(In reply to comment #89)
> Created an attachment (id=102019) [edit]
> screenshot showing network graph axis label bugs
> 
> Also, regarding the network graph, there has been some work done at bug 418181.
> Unfortunately there have been so many changes to HEAD that the patch probably
> doesn't apply cleanly now. 
> 
> Anyway, there are some problems with the current revision:
> 
> 1) All labels are 1 byte/s when no data has been logged

Yup, that would be soon solved when i write and commit the network scale patch.

> 2) The bottom label is non-zero, and furthermore it should just be "0", not "0
> bytes/s"

I'm not sure about this.
I've just fixed the bottom label.

> 3) The traditional units for network speeds is bits/second with power of 10,
> not Bytes/second with powers of 2

Let's give user the choice.

> 4) In some cases there are mixed units in the labels (e.g. some KiB and some B,
> see screenshot)
> 

Has been fixed.
Comment 92 Benoît Dejean 2008-01-02 23:21:53 UTC
More todo: fix network label alignment.
Comment 93 Thomas D Ahle 2008-01-03 01:59:17 UTC
> > 3) The traditional units for network speeds is bits/second with power of 10,
> > not Bytes/second with powers of 2
Eh, as far as I'm aware only from SDL vendors. All desktop applications measure download speed etc. in KiB/s

> Let's give user the choice.
You mean like a preferences option? That sounds like a major run a way from responsibility to me.


> > 4) In some cases there are mixed units in the labels (e.g. some KiB and some
> > B, see screenshot)
> Has been fixed.
Sad, I thought that was a really nice bit. It made it look much more dynamic and smart, rather than hardcoded with unfitting units.
Comment 94 Jared Moore 2008-01-03 02:19:56 UTC
(In reply to comment #91)
> > 2) The bottom label is non-zero, and furthermore it should just be "0", not "0
> > bytes/s"
>
> I'm not sure about this.
> I've just fixed the bottom label.

Well, that's just my preference anyway. It's not particularly important. :)

(In reply to comment #93)
> > > 3) The traditional units for network speeds is bits/second with power of 10,
> > > not Bytes/second with powers of 2
> Eh, as far as I'm aware only from SDL vendors. All desktop applications measure
> download speed etc. in KiB/s
> 

Now that I investigate a bit, you're right. Looks like b/s is only used from the hardware side, not in desktop apps.

> > Let's give user the choice.
> You mean like a preferences option? That sounds like a major run a way from
> responsibility to me.
> 

I agree, adding a preference is unnecessary.

> 
> > > 4) In some cases there are mixed units in the labels (e.g. some KiB and some
> > > B, see screenshot)
> > Has been fixed.

Are you sure? I'm still seeing the same problem.

> Sad, I thought that was a really nice bit. It made it look much more dynamic
> and smart, rather than hardcoded with unfitting units.
> 

I strongly disagree. Mixed units in graphs is _always_ wrong, it makes the scale much harder to read.

Comment 95 Benoît Dejean 2008-01-03 09:01:46 UTC
(In reply to comment #94)
> > > > 3) The traditional units for network speeds is bits/second with power of 10,
> > > > not Bytes/second with powers of 2
> > Eh, as far as I'm aware only from SDL vendors. All desktop applications measure
> > download speed etc. in KiB/s
> > 
> 
> Now that I investigate a bit, you're right. Looks like b/s is only used from
> the hardware side, not in desktop apps.
> 
> > > Let's give user the choice.
> > You mean like a preferences option? That sounds like a major run a way from
> > responsibility to me.
> > 
> 
> I agree, adding a preference is unnecessary.

it's always hard to chose: on one hand system-monitor is a GNOME desktop application and on the other hand more and more users want more technical information. Having the possibility to switch from KiB to kb may be a nice option so you can compare with your 20Mb DSL or 54Mb WiFi.
The netspeed applet (not part of GNOME desktop) has this option.
But if now you think it doesn't worth it, that's OK.


> > 
> > > > 4) In some cases there are mixed units in the labels (e.g. some KiB and some
> > > > B, see screenshot)
> > > Has been fixed.
> 
> Are you sure? I'm still seeing the same problem.

Yes i think so, checkout svn.
Comment 96 Jared Moore 2008-01-03 10:32:46 UTC
Created attachment 102037 [details]
screenshot showing mixed units



(In reply to comment #95)
> 
> Yes i think so, checkout svn.
> 

Hmm... attached screenshot is from svn revision 2241. Still showing the problem :S
Comment 97 Benoît Dejean 2008-01-03 10:46:43 UTC
Sorry, but i can't see.
Comment 98 Jared Moore 2008-01-03 10:53:34 UTC
Sorry, I didn't explain myself very well. What I mean is that on the vertical axis of the network graph, some of the values are in KiB/s and some in bytes/s. They should be all KiB/s or all bytes/s. Otherwise is much harder to read the scale, since it goes 

0, 912, 1.8, 2.7, 3.6, 4.5 

rather than

0, 0.9, 1.8, 2.7, 3.6, 4.5 

:)
Comment 99 Karl Lattimer 2008-01-03 11:06:21 UTC
> 1) the graphs are now using much more CPU to be displayed.  Approx twice what
> is was at the default settings. over 50%.

This isn't ideal, I agree, what I want to do is add an option for frames per sample, and have 1 as the default in GConf. 

> 2) There is a bug which I don't think I've seen before (see screenshot). The
> graph line goes below 0.  I can reliably recreate this by switching g-s-m to
> the system tab.  Close g-s-m.  Open g-s-m.  Wait ~10 seconds.  switch to
> resources tab. 

I've noticed this bug too in other circumstances, I'll be working on this tonight to try and figure it out, I think its something to do with the configure event.

> 3) The time scale is incorrect. When the resources update is changed to >= 2.00
> or <= 0.75 the time axis give spurious numbers (again see screenshot) these can
> be negative or more than one identical value in several places.

This is strange and I'll look into it, 

> 4) following on from this the scale is not accurate.  at 1.5 sec update rate a
> 10 second period on the graph lasts ~19 seconds.

Ouch, looks like a performance hit from cairo, I think the smooth scrolling will improve as cairo performance in graphics drivers generally goes up. So it'll be a good idea to leave the code necessary to have it in there but defaulting to 1 frame per sample will take us back to where we were, and hopefully the accuracy of the graphs will work OK from then.

> 5) The text on the graph scales is fixed size. I think that may just be
> acceptable for 100% etc but when the information is actually needed (and
> changes) then accessibility is an issue here.

Should we scale the text relative to the graph size?

Comment 100 Karl Lattimer 2008-01-03 11:13:30 UTC
I'd like to add that we should revert the changes I've made and instead use the patch from bug #418181 to solve the network graph issue. 

What I want to do next is completely remove the expermental 'subframe' separation in from the load graph update and load graph draw methods, this will make it easier to apply the above patch. 

The idea behind this was to stop the jitter, however I noticed curiously the jitter doesn't happen at all when using >30 fps on my iMac so it appears that its mostly something related to the CPU scaling delay or something similar rather than the actual computations that go on during the sampling.

I also agree that having a preference option for showing in KB/s KiB/s and kbps would be an advantage, allow the distributors to choose their default and we can use whatever we agree is more pertinent for our default.

Hopefully we can get this bug and bug #418181 fixed by release, at a minimum I also want bug #361374 fixed before this release as all of the necessary ground work is already done (gconf/treeview stuff) it just needs to be tied up to the preferences UI.
Comment 101 Toby Dacre 2008-01-03 14:53:44 UTC
Created attachment 102050 [details] [review]
patch to stop background redraws

Karl,

It seems you are redrawing the graph background on each 'tick'.

This patch will cause redrawing of the background only when needed (when graph speed or size changes)
Comment 102 Toby Dacre 2008-01-03 15:06:30 UTC
(In reply to comment #99)
> > 1) the graphs are now using much more CPU to be displayed.  Approx twice what
> > is was at the default settings. over 50%.
> 
> This isn't ideal, I agree, what I want to do is add an option for frames per
> sample, and have 1 as the default in GConf. 
> 

I'm thinking can we save some work by using the graph_buffer.  If we shift it left and then just add the last point. This would save lots of redrawing the old points (that we've already drawn).

A second thing we could do is to have 'smooth scrolling' as an option so people with low spec machines are less affected.

> > 2) There is a bug which I don't think I've seen before (see screenshot). The
> > graph line goes below 0.  I can reliably recreate this by switching g-s-m to
> > the system tab.  Close g-s-m.  Open g-s-m.  Wait ~10 seconds.  switch to
> > resources tab. 
> 
> I've noticed this bug too in other circumstances, I'll be working on this
> tonight to try and figure it out, I think its something to do with the
> configure event.
> 

It is annoying ;)  I haven't been able to track it down yet.

> > 3) The time scale is incorrect. When the resources update is changed to >= 2.00
> > or <= 0.75 the time axis give spurious numbers (again see screenshot) these can
> > be negative or more than one identical value in several places.
> 
> This is strange and I'll look into it, 
> 
> > 4) following on from this the scale is not accurate.  at 1.5 sec update rate a
> > 10 second period on the graph lasts ~19 seconds.
> 
> Ouch, looks like a performance hit from cairo, I think the smooth scrolling
> will improve as cairo performance in graphics drivers generally goes up. So
> it'll be a good idea to leave the code necessary to have it in there but
> defaulting to 1 frame per sample will take us back to where we were, and
> hopefully the accuracy of the graphs will work OK from then.
> 

I think this is related to point 3 I think if 3 is fixed this will be too

> > 5) The text on the graph scales is fixed size. I think that may just be
> > acceptable for 100% etc but when the information is actually needed (and
> > changes) then accessibility is an issue here.
> 
> Should we scale the text relative to the graph size?
> 

I think it really wants to be scaled relative to the desktop font size.  I like it nice and small but I have a friend with eyesight problems and also I've done lots of accessibility stuff on the web, so it's something I really care about.


PS I think my last patch may need a little garbage collecting :)
Comment 103 Toby Dacre 2008-01-03 15:46:29 UTC
Created attachment 102053 [details] [review]
patch to correct time axis values

This patch corrects the display of the time axis on the graphs.

changes

* The loop now generates and adds all the values.

* Rounding error causing (funny values) fixed ( moving the /1000).

* removed set value '50' now generated from used values to allow changes to graph update speed.

* removed the 'buffer' from NUM_POINTS.

There is still a timing issue (comment #88 pont 4) but it is now closer than it was ;)
Comment 104 Martin Ejdestig 2008-01-03 16:32:29 UTC
(In reply to comment #102)
> A second thing we could do is to have 'smooth scrolling' as an option so people
> with low spec machines are less affected.

No. First you should:

1) Try to make it fast enough on low spec machines. (Really, I don't see a reason for this to be slow on ANY machine capable of running GNOME.)

2) If you can't make it fast enough (though I SERIOUSLY doubt that), try to do the low spec mechine detection automatically.

3) If you can't do automatic detection automatically, there should be a global "low spec machine" option for the whole desktop.

But come on. There really should be no reason for this to go slow. What needs optimizing is another question.
Comment 105 Toby Dacre 2008-01-04 13:32:34 UTC
Created attachment 102112 [details] [review]
network rescale update patch

Oops,

forgot we need to do a redraw of the network graph background when the scale changes too.

This fixes it
Comment 106 Toby Dacre 2008-01-04 13:50:48 UTC
Created attachment 102116 [details] [review]
patch to stop network scale showing repeated values

When g-s-m starts and there is no network traffic it shows 1 bytes/sec on several points on the y-axis.

This is a patch to correct this.  A different approach would be to have a minimum value for g->net.max, which may be a nicer fix thinking about it.  Anyway here is the option that makes the graphs only show 'sane' values. As I say it may be better to ensure it only gets 'sane' values in the first place.

ie change 

net_scale(...

	// make sure max is not 0 to avoid / 0
	new_max = std::max(new_max, 1U);
Comment 107 Benoît Dejean 2008-01-04 17:58:06 UTC
I'm actually going to revert that patch.
(Anyway rate < g->net.max is always true).
I think this is more related to #418181. I'm going to revert and commit a smart round up. I'd appreciate you to test.
Comment 108 Toby Dacre 2008-01-04 18:22:27 UTC
(In reply to comment #107)


> I'd appreciate you to test.

This seems to now be working :)

The numbers are also now giving nice values.

Comment 109 Toby Dacre 2008-01-11 16:37:23 UTC
I've done a patch to reduce the CPU load when drawing the graphs by ~90%

it is at http://bugzilla.gnome.org/show_bug.cgi?id=507797#c11 as that bug is specifically about the graph CPU usage.
Comment 110 Yang Hong 2008-01-16 02:54:12 UTC
I just build system-monitor from SVN HEAD.

Both mem/cpu/net graph is jumping forward when update interval is arrived, it should be soomthly.
Comment 111 Toby Dacre 2008-01-17 19:51:13 UTC
Created attachment 103087 [details] [review]
fix to smooth jumpy graph

from Comment #110

This is a quick fix
as before the (int) cast is important for performance on my pc
Comment 112 Benoît Dejean 2008-01-17 21:46:28 UTC
I see no improvment. Cairo is too slow, so we use a low framerate, that's why the graph gets jumpy.
Comment 113 Toby Dacre 2008-01-17 23:55:37 UTC
g->frames_per_unit used to equate to number of pixels per frame (before being capped at 5) so could be used in calculations as such.  It is different from the old FRAMES constant.

This is for svn HEAD.  At the moment it moves the graph 1px per update for 4 updates and then moves however many pixels are left in the frame on the next update.  This is the 'jumpy described'.  Build from svn and you will see.  

The patch just spreads the updates evenly across the number of pixels in the frame.



Did you get any further with why ciaro is so different on different systems?  I've tried searching but got nowhere and don't have a radeon card to see if that's a part of it. 
Comment 114 Benoît Dejean 2008-01-18 08:20:33 UTC
It's still jumpy, it's not smooth.

Anyway, turning delx into an int would also the same as your patch. No?
Comment 115 Karl Lattimer 2008-01-18 10:02:41 UTC
Toby can you write a patch ASAP to return the frame counting to using a constant FPS, so I get 60 seconds rather than 58 on the horizontal scale again?

Your patch had some sneaky draw backs and this is one of them... We need a static FPS (reasonable for release), but have the flexibility to test the smoothing effect over time. There will be significant enhancement possible to this but we need to make sure it doesn't chow down on everyones chips until we get over some of the performance issues.

It also means we can more adequetely profile gsm while its running and do comparative analysis on the amount of time spent in each library and each function call.

Comment 116 Benoît Dejean 2008-01-18 11:31:04 UTC
The problem is that profiling doesn't really help since bad things happen in the X server.
Comment 117 Toby Dacre 2008-01-18 12:16:43 UTC
(In reply to comment #115)
> Toby can you write a patch ASAP to return the frame counting to using a
> constant FPS

I can.  The last patch I wrote comment #111 allows for constant FPS (if it is capped as it now is, or variable if the cap is removed).  I'm sorry if my explanation (comment #113) was not clear. 

in rev2279  we are using (g->graph_buffer_offset - g->render_counter) for the y co-ordinate where g->render_counter is 0,1,2,3,4.  Since each frame is ~9px wide at g-s-m minimum width.  This causes the jump as whenever g->render_counter is reset the graph buffer is 'moved' by ~9px  but when we do the overlay we never move it by more than 5px in total.
The patch makes the graph be shifted evenly across all the pixels of the frame not just the first 5px.


g->render_counter * (g->graph_delx/g->frames_per_unit)  is the formula we want.
but when g->graph_delx = g->frames_per_unit as it did when we had variable number of frames then (g->graph_delx/g->frames_per_unit) = 1 so we did not need it there.

>, so I get 60 seconds rather than 58 on the horizontal scale again?
> 
#define NUM_POINTS 60    => #define NUM_POINTS 62

I'm not sure why this got changed it was not in my patch.
The graph 'buffer' of 2 frames is needed.  We could rewrite things so that NUM_POINTS becomes 'number of points we see' instead of 'number of points including buffer' but I'm not sure maybe a '// remember to add 2 for the buffer' comment is needed.

> Your patch had some sneaky draw backs and this is one of them... We need a
> static FPS (reasonable for release)

I agree that it needs to be not using unreasonable amounts of CPU.  It is a shame we can't find the cause of this to determine if it is a problem on a given machine and cap the frame count accordingly.  As I've said for me it is beautifully smooth, even on 0.25sec updates.  But it needs to work for all users so capping may be that solution.


>, but have the flexibility to test the
> smoothing effect over time. There will be significant enhancement possible to
> this but we need to make sure it doesn't chow down on everyones chips until we
> get over some of the performance issues.
> 

Has anyone been able to narrow down the cairo issues to a cairo release that makes a difference or whether it is more a hardware issue? Could we autodetect or is the problem still a mystery?  Has anyone tried to find out from the cairo people why this is a problem.

In conclusion I don't think There is any other patch that is needed.  I can put 
#define NUM_POINTS 60    => #define NUM_POINTS 62 in a patch if you want.  Let me know how you wish me to progress this.  
Comment 118 Martin Ejdestig 2008-01-18 13:09:09 UTC
(In reply to comment #116)
> The problem is that profiling doesn't really help

Not true. :)

> since bad things happen in the X server.

You just have to do a system wide profile using e.g. Sysprof or Oprofile.
Comment 119 Benoît Dejean 2008-01-18 14:24:43 UTC
(In reply to comment #118)
> (In reply to comment #116)
> > The problem is that profiling doesn't really help
> 
> Not true. :)
> 
> > since bad things happen in the X server.
> 
> You just have to do a system wide profile using e.g. Sysprof or Oprofile.
> 

Well, i mean that not everyone has X debug packages and can understand X internals.
Comment 120 Toby Dacre 2008-01-18 21:03:33 UTC
I've been on #cairo on IRC

the most useful info I got is

"The performance of RENDER on X is determined most of all by whether the drawables end up in video memory or normal RAM"

Using XaaNoOffscreenPixmaps in xorg.conf can make a difference but this is not an option for us as it is beyond our control.

Is there a way we can determine what graphics driver is being used?  If so then maybe we can whitelist drivers when they have good enough cairo performance and cap the others.