Merge lp:~niclasl/granite/fix-padding into lp:~elementary-pantheon/granite/granite

Proposed by Niclas Lockner on 2013-11-17
Status: Merged
Approved by: Julián Unrrein on 2013-12-06
Approved revision: 650
Merged at revision: 654
Proposed branch: lp:~niclasl/granite/fix-padding
Merge into: lp:~elementary-pantheon/granite/granite
Diff against target: 30 lines (+5/-3)
1 file modified
lib/Widgets/DynamicNotebook.vala (+5/-3)
To merge this branch: bzr merge lp:~niclasl/granite/fix-padding
Reviewer Review Type Date Requested Status
Daniel Fore 2013-11-19 Approve on 2013-12-04
Julián Unrrein (community) Approve on 2013-12-03
elementary UX 2013-11-26 Pending
elementary Pantheon team 2013-11-17 Pending
Review via email: mp+195525@code.launchpad.net

Commit message

Add padding around the new tab button

Description of the change

Add padding around the new tab button

To post a comment you must log in.
Niclas Lockner (niclasl) wrote :

With this padding, the button looks a bit weird, but according to #1166389, this bug is worked around in the theme, so it may be due to that.
When I switch to HighContrast, the button looks better.

Daniel Fore (danrabbit) wrote :

Build fails with lots of output >.<

http://snippi.com/s/wl73xr3

But yes, it's worked around in the theme so it's gonna look funny unless you remove the workaround.

Niclas Lockner (niclasl) wrote :

Hm I don't know why the build fails for you but it doesn't look like it's caused by a compile error

Julián Unrrein (junrrein) wrote :

I tested this branch, but I couldn't notice any difference in the distance between the New Tab button and the Previous Tab button while using the High Contrast theme (3.4).

Screenshots:
- In trunk: http://imagebin.org/280253
- In this branch: http://imagebin.org/280254

To test this branch, I first run the following line:

LD_LIBRARY_PATH=/home/julian/elementary-projects/granite/fix-1181657/build/lib ./demo/granite-demo

After not noticing any difference, I make install'd the branch, which made no difference.

Could you share how you tested this and show screenshots?

review: Needs Fixing
Niclas Lockner (niclasl) wrote :

I simply opened pantheon-terminal and created some tabs so that the arrows appear. I get the same results with Files. (This is with Ubuntu 13.10 + the daily PPA)

With granite trunk:
http://imagebin.org/280264
http://imagebin.org/280263

With this branch (make && sudo make install):
http://imagebin.org/280261
http://imagebin.org/280262

Julián Unrrein (junrrein) wrote :

I somehow mis-tested your branch. I tested it again and it works just as expected.

Code style review following soon.

Julián Unrrein (junrrein) wrote :

Change the name of the Gtk.Box to something more descriptive, like "add_button_box".

review: Needs Fixing
lp:~niclasl/granite/fix-padding updated on 2013-12-03
650. By Niclas Lockner on 2013-12-03

Renamed variables

Julián Unrrein (junrrein) wrote :

I'm approving, but we still need a review from Dan.

Thanks for your work!

review: Approve
Daniel Fore (danrabbit) wrote :

Looks good!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/Widgets/DynamicNotebook.vala'
2--- lib/Widgets/DynamicNotebook.vala 2013-10-23 21:20:04 +0000
3+++ lib/Widgets/DynamicNotebook.vala 2013-12-03 01:03:35 +0000
4@@ -560,6 +560,7 @@
5 private Gtk.Button add_button;
6 private Gtk.Button restore_button; // should be a Gtk.MenuButton when we have Gtk+ 3.6
7
8+ private static const int ADD_BUTTON_PADDING = 5; // Padding around the new tab button
9 private static const string CLOSE_BUTTON_STYLE = """
10 * {
11 -GtkButton-default-border : 0;
12@@ -626,14 +627,15 @@
13 restore_tab_m.sensitive = false;
14 });
15
16+ Gtk.Box add_button_box = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 0);
17 add_button = new Gtk.Button ();
18 add_button.add (new Gtk.Image.from_icon_name ("list-add-symbolic", Gtk.IconSize.MENU));
19- add_button.margin_left = 6;
20 add_button.relief = Gtk.ReliefStyle.NONE;
21 add_button.tooltip_text = _("New Tab");
22- this.notebook.set_action_widget (add_button, Gtk.PackType.START);
23- add_button.show_all ();
24 add_button.get_style_context ().add_provider (button_fix, Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION);
25+ add_button_box.pack_start (add_button, false, false, ADD_BUTTON_PADDING);
26+ add_button_box.show_all ();
27+ this.notebook.set_action_widget (add_button_box, Gtk.PackType.START);
28
29 restore_button = new Gtk.Button ();
30 restore_button.add (new Gtk.Image.from_icon_name ("user-trash-symbolic", Gtk.IconSize.MENU));

Subscribers

People subscribed via source and target branches