GNOME Bugzilla – Bug 701966
Implementation the new 3.10 design
Last modified: 2013-07-24 10:21:52 UTC
See https://live.gnome.org/Design/Whiteboards/CloseButton. Another moment I will add patches.
Created attachment 246455 [details] [review] Port to GtkHeaderBar
Created attachment 246456 [details] [review] Make headerbar a titlebar
Created attachment 246457 [details] [review] Use GtkButton subclasses instead of GdHeaderButton ones
Created attachment 246458 [details] [review] Remove libgd submodule
Created attachment 246459 [details] [review] Bump required GTK+
Screenshot maybe?
Created attachment 246493 [details] DevHelp new 3.10 design (screenshot)
Created attachment 246494 [details] DevHelp new 3.10 design (Screenshot, Maximize)
Review of attachment 246458 [details] [review]: Would be good to say in the commit log why it is being removed. Also, autogen.sh complaints about the missing libgd submodule: [aleksander@ares devhelp]$ ./autogen.sh /usr/bin/gnome-autogen.sh No submodule mapping found in .gitmodules for path 'libgd' Probably needs to be removed from the git config as well.
Review of attachment 246457 [details] [review]: Please make sure that no empty lines only with whitespaces are added; got 4 reported when applying this patch.
Review of attachment 246456 [details] [review]: ::: src/dh-window.c @@ +693,3 @@ + /* Make headerbar a titlebar */ + gtk_window_set_titlebar(GTK_WINDOW(window), priv->header_bar); Whitespace needed before the open parenthesis
Comment on attachment 246455 [details] [review] Port to GtkHeaderBar I'd merge the GTK+ bump commit into this one, as GtkHeaderBar already needs the new GTK+ version. Also, explicitly say in the commit message that GTK+ is being bumped and to which version.
*** Bug 700004 has been marked as a duplicate of this bug. ***
Created attachment 246504 [details] [review] Use GtkButton subclasses instead of GdHeaderButton ones (no empty lines only with whitespaces are added)
Created attachment 246505 [details] [review] Make headerbar a titlebar (whitespace needed before the open parenthesis)
That's good enough?
(In reply to comment #16) > That's good enough? Please also merge the GtkHeaderBar and gtk+ bump in the same commit; and see what happens with the libgd warning during autogen.
Created attachment 246533 [details] [review] Port to GtkHeaderBar and Bump required GTK+
Review of attachment 246505 [details] [review]: This patch now doesn't apply on top of the new "Port to GtkHeaderBar and Bump required GTK+" one :/
Review of attachment 246505 [details] [review]: Actually, also make sure that you add a whitespace before each open parenthesis in this patch.
Sorry for all this review loop, but changing patches independently instead of updating them all together has these things :) Could you please fix these last things and upload all the patches again? Also, please make sure you mark as "obsolete" patches that are being updated with new ones.
Created attachment 246536 [details] [review] Port to GtkHeaderBar and bump required GTK+
Created attachment 246537 [details] [review] Make headerbar a titlebar
Created attachment 246538 [details] [review] Use GtkButton subclasses instead of GdHeaderButton ones
Created attachment 246539 [details] [review] Remove libgd submodule
I uploaded the revised amendments.
(In reply to comment #26) > I uploaded the revised amendments. Please mark as obsolete the patches that no longer should be considered. Go to the Attachments section below, click on "Details" for each of those patches, and then mark the "obsolete" checkbox there. Thanks!
Review of attachment 246538 [details] [review]: Still multiple whitespaces needs to get added in this patch... :/ ::: src/dh-window.c @@ +675,3 @@ + } + + back_button = gtk_button_new(); Whitespace needed before parenthesis... @@ +676,3 @@ + + back_button = gtk_button_new(); + back_image = gtk_image_new_from_icon_name(prev_icon, GTK_ICON_SIZE_MENU); Whitespace needed before parenthesis... @@ +677,3 @@ + back_button = gtk_button_new(); + back_image = gtk_image_new_from_icon_name(prev_icon, GTK_ICON_SIZE_MENU); + gtk_button_set_image(GTK_BUTTON(back_button), back_image); Whitespace needed before parenthesis... Really lots of these still to get fixed in this same patch...
Review of attachment 246539 [details] [review]: There's still some LIBGD_INIT(.. in configure.ac
Created attachment 246540 [details] [review] Use GtkButton subclasses instead of GdHeaderButton ones (whitespace needed before the open parenthesis)
Created attachment 246544 [details] [review] Remove libgd submodule
I asked on #gnome-design and the stuff in Design/Whiteboards is not yet appropriate for implementation; I'd keep the close button changes out of the bug report for the moment.
(In reply to comment #32) > I asked on #gnome-design and the stuff in Design/Whiteboards is not yet > appropriate for implementation; I'd keep the close button changes out of the > bug report for the moment. Are you sure? It has been implemented in several applications. See: Baobab: https://git.gnome.org/browse/baobab/commit/?id=fe327ebe89c7e86c12766be71c6d376695d2dc2b gitg: https://git.gnome.org/browse/gitg/commit/?id=87650faead3cc1d7ff2370472b2e5f38a4568996 GNOME Weather: https://git.gnome.org/browse/gnome-weather/commit/?id=2bb7bee1c10df02b453f4eab4b181d28e4152230 GNOME Clocks: https://git.gnome.org/browse/gnome-clocks/commit/?id=8662e90d2ae3c1b8ea030111b967ff3d8deca74c
You do not push the changes?
I'd like to have designer input first. Adding the ui-review keyword. You could also try pinging in #gnome-design.
I think you can implement this design. It has been applied enough applications (gitg, gnome-clocks, gnome-weather, baobab) so it will be designed for implementation.
(In reply to comment #30) > Created an attachment (id=246540) [details] [review] > Use GtkButton subclasses instead of GdHeaderButton ones (whitespace needed > before the open parenthesis) I think once you port buttons form libgd in the same patch you should add context classes like it is done in baobab gnome-clocks etc. otherwise buttons will not look the same way is was in libgd, take a look for example here https://git.gnome.org/browse/gnome-clocks/tree/src/window.vala#n82 close_button.get_style_context ().add_class ("image-button"); moreover for a separator align is missing: separator.valign = Gtk.Align.FILL; I would suggest that there are screenshots for comparison before and after the change of porting from libgd, there should be no visible change.
Created attachment 248088 [details] [review] Port to GtkHeaderBar and bump required GTK+
Created attachment 248089 [details] [review] Use GtkButton subclasses instead of GdHeaderButton ones and fix arrow icons in RTL locales
Created attachment 248090 [details] [review] Remove libgd submodule
Created attachment 248091 [details] [review] Make headerbar a title and add close button You can wait with this patch. The quilt codes, you can push.
Created attachment 248092 [details] Before the changes
Created attachment 248093 [details] After the changes (without close button)
Created attachment 248094 [details] After the changes (with close button)
Created attachment 248100 [details] [review] Fix a tooltip of forward_button Error of copy/paste.
I can push the patches ? (without the patch 'Make headerbar a title and add close button')
Talked to aday in IRC and he said that we should probably wait to add the close button stuff, as that is not clearly defined yet. So I'll merge all the patches except for the one implementing the close button change.
I can push the patches. I got access to git.
I pushed the patches (without the patch 'Make headerbar a title and add close button').
(In reply to comment #49) > I pushed the patches (without the patch 'Make headerbar a title and add close > button'). :( I was going to fixup several whitespace issues that I found and fix the format of the commit logs before doing that.
(In reply to comment #50) > (In reply to comment #49) > > I pushed the patches (without the patch 'Make headerbar a title and add close > > button'). > > :( I was going to fixup several whitespace issues that I found and fix the > format of the commit logs before doing that. In general, you shouldn't push the patches yourself, unless someone reviewed the patch and added the "accepted-commit-now" tag in the patch; please don't do that again :) I also wanted to re-test the patches myself before pushing them, maybe I should have said that explicitly, sorry for not saying it. Anyway, for the patches that you pushed, now please mark them as 'committed' in this bugreport.
(In reply to comment #45) > Created an attachment (id=248100) [details] [review] > Fix a tooltip of forward_button > > Error of copy/paste. You didn't push this one, did you? Or did you fixed-it-up with some other commit?
(In reply to comment #51) > (In reply to comment #50) > > (In reply to comment #49) > > > I pushed the patches (without the patch 'Make headerbar a title and add close > > > button'). > > > > :( I was going to fixup several whitespace issues that I found and fix the > > format of the commit logs before doing that. > > In general, you shouldn't push the patches yourself, unless someone reviewed > the patch and added the "accepted-commit-now" tag in the patch; please don't do > that again :) I also wanted to re-test the patches myself before pushing them, > maybe I should have said that explicitly, sorry for not saying it. Just a few hours ago I received a access permissions to git. The wiki does not say when you can push changes. > Anyway, for the patches that you pushed, now please mark them as 'committed' in > this bugreport. How to do it? (In reply to comment #52) > (In reply to comment #45) > > Created an attachment (id=248100) [details] [review] [details] [review] > > Fix a tooltip of forward_button > > > > Error of copy/paste. > > You didn't push this one, did you? Or did you fixed-it-up with some other > commit? I fixed-it-up with some other commit (with the 'Use GtkButton subclasses instead of GdHeaderButton ones' patch).
> > > > I pushed the patches (without the patch 'Make headerbar a title and add close > > > > button'). > > > > > > :( I was going to fixup several whitespace issues that I found and fix the > > > format of the commit logs before doing that. > > > > In general, you shouldn't push the patches yourself, unless someone reviewed > > the patch and added the "accepted-commit-now" tag in the patch; please don't do > > that again :) I also wanted to re-test the patches myself before pushing them, > > maybe I should have said that explicitly, sorry for not saying it. > > Just a few hours ago I received a access permissions to git. > The wiki does not say when you can push changes. > In general, you shouldn't push changes to a project in which you are not a maintainer, *unless* someone explicitly asks you to do so. This explicit permission is usually through the 'accepted-commit-now' tag in bugzilla per-patch, or may be less official, like a "go on" in IRC. Having commit rights don't give you the right to push whatever you want, you still should go through the maintainers filter. But don't worry, you'll get used to it :) Also, note, that even if *I* was the one pushing *your* patch, the authorship in git will still be yours. So if someone tells you he's going to push your patches, just forget about it, you shouldn't go on and push them yourself, you don't need to worry. Specially because maintainers will usually re-review long patches, re-compile and re-test before really pushing them, unless they are simple enough to let the patch creator push them (flagging them with accepted-commit-now). > > Anyway, for the patches that you pushed, now please mark them as 'committed' in > > this bugreport. > > How to do it? > Go to the list of patches below, and click in "Details" for each of those already pushed. Then, you'll be able to select "committed" in the combobox for the "status". Each patch has a status, and there are different status values. Take a look at them and get used to them; as you'll see them used widely through gnome bugzilla.
(In reply to comment #54) > > > > > I pushed the patches (without the patch 'Make headerbar a title and add close > > > > > button'). > > > > > > > > :( I was going to fixup several whitespace issues that I found and fix the > > > > format of the commit logs before doing that. > > > > > > In general, you shouldn't push the patches yourself, unless someone reviewed > > > the patch and added the "accepted-commit-now" tag in the patch; please don't do > > > that again :) I also wanted to re-test the patches myself before pushing them, > > > maybe I should have said that explicitly, sorry for not saying it. > > > > Just a few hours ago I received a access permissions to git. > > The wiki does not say when you can push changes. > > > > In general, you shouldn't push changes to a project in which you are not a > maintainer, *unless* someone explicitly asks you to do so. This explicit > permission is usually through the 'accepted-commit-now' tag in bugzilla > per-patch, or may be less official, like a "go on" in IRC. Having commit rights > don't give you the right to push whatever you want, you still should go through > the maintainers filter. But don't worry, you'll get used to it :) > > Also, note, that even if *I* was the one pushing *your* patch, the authorship > in git will still be yours. So if someone tells you he's going to push your > patches, just forget about it, you shouldn't go on and push them yourself, you > don't need to worry. Specially because maintainers will usually re-review long > patches, re-compile and re-test before really pushing them, unless they are > simple enough to let the patch creator push them (flagging them with > accepted-commit-now). Thanks for the explanation. I searched such an explanation in the wiki, and not found. Without knowing the rules, I lost git access pretty quick. > > > Anyway, for the patches that you pushed, now please mark them as 'committed' in > > > this bugreport. > > > > How to do it? > > > > Go to the list of patches below, and click in "Details" for each of those > already pushed. Then, you'll be able to select "committed" in the combobox for > the "status". > > Each patch has a status, and there are different status values. Take a look at > them and get used to them; as you'll see them used widely through gnome > bugzilla. I do not see combobox. Should permissions for it?
> > > > > Anyway, for the patches that you pushed, now please mark them as 'committed' in > > > > this bugreport. > > > > > > How to do it? > > > > > > > Go to the list of patches below, and click in "Details" for each of those > > already pushed. Then, you'll be able to select "committed" in the combobox for > > the "status". > > > > Each patch has a status, and there are different status values. Take a look at > > them and get used to them; as you'll see them used widely through gnome > > bugzilla. > > I do not see combobox. > Should permissions for it? Ahh... that may have been it. You're listed just as reporter, not as developer. In general, if you have git access, you should also be marked as developer in bugzilla. You should ask for that to the one who gave you git access. I'll mark them myself as commited.
Nacho has been working also in the close button thing, plus some other things, see bug 704752. Will close this bug as duplicate of that one, as those patches will get merged before the UI freeze. *** This bug has been marked as a duplicate of bug 704752 ***