GNOME Bugzilla – Bug 677713
pimp the sidebar
Last modified: 2016-03-31 14:03:11 UTC
It doesn't look like the mockups
Created attachment 215959 [details] [review] Add names to more actors for easier debugging
Created attachment 215960 [details] [review] Fix up indentation
Created attachment 215961 [details] [review] Add shadow to sidebar We remove the background from the sidebar Gtk+ widgets and put in an actor background with a shadow.
Created attachment 215962 [details] [review] Make properties sidebar look like mockup
Created attachment 215963 [details] [review] Make MiniGraphs look like mockup Use the right colors and line widht. Stroke after fill.
*** Bug 674665 has been marked as a duplicate of this bug. ***
Created attachment 215964 [details] [review] Fix bold in properties sidebar It seems the cell renderer doesn't pick up the css font weight. Also, with the bold, drop the font size to compensate.
Review of attachment 215959 [details] [review]: ACK
Review of attachment 215960 [details] [review]: ACK
Review of attachment 215961 [details] [review]: Aside from the nitpicks, I also get loads of warnings on this one: http://fpaste.org/oDXL/ ::: src/sidebar.vala @@ -127,0 +129,12 @@ + bin_actor = new Clutter.Actor (); + var bin = new Clutter.BinLayout (Clutter.BinAlignment.FILL, + Clutter.BinAlignment.FILL); ... 9 more ... * indentation is off by 1char here i think (or splinter is messing it up) * p -> pattern @@ +145,3 @@ + cr.rectangle (0, 0, w, h); + cr.fill (); + return true; newline before return @@ -127,0 +129,38 @@ + bin_actor = new Clutter.Actor (); + var bin = new Clutter.BinLayout (Clutter.BinAlignment.FILL, + Clutter.BinAlignment.FILL); ... 35 more ... No need to break a call on multiple lines if it fits 120chars.
Review of attachment 215961 [details] [review]: Weird that I only see a shadow on the right side if i revert this patch (only) in this series. ::: src/sidebar.vala @@ -127,0 +129,21 @@ + bin_actor = new Clutter.Actor (); + var bin = new Clutter.BinLayout (Clutter.BinAlignment.FILL, + Clutter.BinAlignment.FILL); ... 18 more ... Do we need to explicitly invalidate if we move the canvas.set_size() call here after we have connected to draw signal?
Review of attachment 215961 [details] [review]: To avoid the warnings you need: http://git.gnome.org/browse/clutter/commit/?id=c0b3e2e83aca99541ef5eeb15549a482ecf4f7d7 ::: src/sidebar.vala @@ -127,0 +129,21 @@ + bin_actor = new Clutter.Actor (); + var bin = new Clutter.BinLayout (Clutter.BinAlignment.FILL, + Clutter.BinAlignment.FILL); ... 18 more ... No, so that seems cleaner. @@ +150,3 @@ + shadow.set_content (canvas); + shadow.set_content_gravity (Clutter.ContentGravity.RESIZE_FILL); + shadow.set_content_scaling_filters (Clutter.ScalingFilter.NEAREST, Clutter.ScalingFilter.NEAREST); We can now use the new content repeat API instead of scaling here.
(In reply to comment #12) > Review of attachment 215961 [details] [review]: > > To avoid the warnings you need: > > http://git.gnome.org/browse/clutter/commit/?id=c0b3e2e83aca99541ef5eeb15549a482ecf4f7d7 Yeah, updating to latest clutter git master helps. Could you please bump our clutter dep in the same commit?
Review of attachment 215962 [details] [review]: Looks good, not just as in the code but the results as well. :)
There has been no clutter release with that patch, so we can't depend on it. Its just a warning though, so it can be ignored for now.
(In reply to comment #15) > There has been no clutter release with that patch, so we can't depend on it. The version is bumped in clutter git master so we add depenendcy to that. Since clutter follows gnome release cycle(?), we can and IMO should do that. We do it with other deps all the time.. > Its just a warning though, so it can be ignored for now. I don't think thats true cause I wasn't able to see the sidebar shadow before I updated my clutter install.
Ok, then we should bump the version definitely.
Attachment 215961 [details] pushed as cd718ff - Add shadow to sidebar Attachment 215962 [details] pushed as 8820fc5 - Make properties sidebar look like mockup
Still two patches to review..
Review of attachment 215963 [details] [review]: I'm really not the expert of cairo so was hoping someone else reviews this but seems right and works.
Review of attachment 215964 [details] [review]: ACK
Attachment 215963 [details] pushed as 2471e6a - Make MiniGraphs look like mockup Attachment 215964 [details] pushed as 49b5e9e - Fix bold in properties sidebar