Created attachment 75062 [details] [review]
Patch to add to the ringschart the "sector highlight" feature 

The patch highligths the selected sector by increasing the intensity of the sector's color.

Created attachment 75068 [details] [review]
Patch to add to the ringschart the "sector highlight" feature 

Same patch but now created with "cvs diff -up" from the gnome-utils directory.

I hope you like it!! :)

Very nice! I really do like this.
Please wait for Paolo's code review before committing.

looks good to me except for the closing brace having wrong indentation and a trailing whitespace[1]


Please commit (or tell Alejandro to commit, he has cvs write permissions)



[1] I am such a pain in the ass, am I not? ;-)

>>[1] I am such a pain in the ass, am I not? ;-)

No... it's just a matter of habit.. :-)

It is nice, I have a couple of suggestions we can decide about the solution and the implementation:
   * The center circle is not hightlighted, maybe we can modify the interface of the draw_circle to add the fill_color, and move the fill color management to the external if clause (just after the "if (real_draw)"? What do you think? Maybe we can even have a boolean variable, highlighted, that we pass as a parameter to the get_color method and avoid the if condition.
   * The color of the highlight is some way similar to the dimmed color, could we modify the get_color to get a more intense color instead of adding white? If highlighted we add more red to the red and so on. What do you think? Could you check if this makes no sense at all :-)
   * Shouldn't we use the ChangeLog in the baobab directory?

I hope this helps, thanks for the patch Miguel :-). 

off-topic - Paolo don't stop the pain in the ass habit :-)

I agree with Alejandro's suggestions, but if it means too much pain, we can stay as it is now in the patch.
Yes, we should use ChangeLog in baobab's dorectory.

Created attachment 75394 [details] [review]
New proposal of the sector highlightning patch

I had to change more code than I initially though, but I think this is the right patch.

I followed Alex's instructions in order to highlight the center circle. Now the "draw_circle" and "get_color" functions receive a "highlighted" flag in order to modify the color and make the sector highlighted.

In order to highlight the inner circle, I had to modify the "traverse_tree" function, and remove the condition to draw the center circle when the tree_model has only one node (the initial state, when only the total disk usage is shown). Now, the center circle is always drawn in the "ringschart_draw" function, and the "traverse_tree" one doesn't do it anymore. Once the ringschart is completely drawn, if the pointer is selecting the center circle (we are going to draw a tip), it is redrawn with the highlighted color.
 
In the initial state (when disk usage is shown), the center circle is not highlighted when selected (it doesn't have a tip either).

I modified the way the color highlightning is done. Now the color components are divided by the higher one. This way, the higher component is set to 1 (the max), resulting in an enhacement of that component.

Finally, I used the baobab Changelog this time ;)

Looks very good for me. Paolo?

I'd like Alejandro to review the patch, he knows the code better. As far as I am concerned I am fine with it.

Created attachment 75548 [details] [review]
Proposed patch

I think it is ok :-), I've just modified the Changelog to reflect the committer, the assignment of the default central color and the last but not the least, the email of Miguel in the header because it was wrong :-).

If it is ok with you guys, Miguel, Paolo and Fabio, I can commit it.

Nice work.

Now that I look at it, can you change fill_color() to something like:

void get_color (GdkColor *color, foo, bar);

and use it like:

...
GdkColor color;
...
get_color (&color, foo, bar);
...

so that color is allocated on the stack and doesn't need to be allocated and freed every time.


Other than that it looks good to me, feel free to commit.


In the usual "pbor-is-a-pain" departement, can you remove spaces around -> ?
foo->bar, not foo -> bar :)

Created attachment 75625 [details] [review]
New proposed patch, color functions API modified

Added the recomendations from Paolo about the API of the functions, Miguel could you review and test the patch, I'll commit it as soon as you send me the confirmation if there isn't any more objection by someone. BTW, I like the format of the ChangeLog with information of all the modifications of each function it is easy to follow the changes later, IMO would be nice if you can add it, I mean, this way:

        * src/baobab-treeview.c:
        (create_directory_treeview): Selection changed signal connected to
        a new callback that synchronizes the status of the treeview with
        the ringschart.
        * src/baobab.c:
        (set_busy): Added the control of the ringschart widget when the
        scanning process starts and finishes, we can't draw the ringschart
        when the treemodel is being scanned.
        (initialize_ringschart): Added this function, it creates and
        initializes the ringschart.
        (main): Called the initialize_ringschart function.
        * src/baobab.h: Added the new attribute to the application
        structure.

Created attachment 75868 [details] [review]
New version of the patch

Alex, the patch works fine for me. 

I added the Changelog information as you asked (not sure about whose name shold go there: mine one? yours one? both?), so you can commit it when you wish.

BTW I removed the whitespaces around "->" as eagle-eye-paolo asked ;)

Cheers!!

Created attachment 75878 [details] [review]
New version of the patch (little error fixed)

Ooooops, last patch had a little error in the Changelog file. This one is the good one ;)

Cheers!

IMO it is ok, I'll commit it as soon as Paolo and Fabio give their approval,
just a couple of small format changes. There is a tab inside one indentation
that I can change:

+get_color (Color *color,
+           gdouble angle,
+          gint circle_number,
+          gboolean highlighted)

And two spaces instead of four in this case, I would also change them:
+        if ((priv->draw_center) && (tooltip->depth == -1))
+            draw_circle (cr, priv->center_x, priv->center_y, priv->min_radius,
TRUE);

I'll change also the name of the committer in the ChangeLog and add a comment
about the author of the patch.

Anything else?

go for it!

alex committed. Thanks!

Thanks Alex!