GNOME Bugzilla – Bug 727068
Use headerbar in all the dialogs and use popover
Last modified: 2014-04-14 08:11:22 UTC
See the patches.
Created attachment 272945 [details] [review] Bump requirements for GTK+
Created attachment 272946 [details] [review] Use headerbar in the commit dialog
Created attachment 272947 [details] [review] Use headerbar in the author details dialog
Created attachment 272948 [details] [review] Use headerbar in the clone dialog
Created attachment 272949 [details] [review] Use headerbar in the preferences dialog
Created attachment 272950 [details] [review] Use popver for the gear menu
Review of attachment 272945 [details] [review]: Looks good.
Review of attachment 272946 [details] [review]: See comment. ::: gitg/commit/gitg-commit-dialog.vala @@ +680,3 @@ Ggit.Diff? diff) { + Object(author: author, diff: diff, use_header_bar: 1); any reason to define this in the code and not in the ui file?
Review of attachment 272947 [details] [review]: Maybe I miss something here, but wasn't it the GtkDialog suppose to deal with the buttons redistribution by just setting the use-header-bar property?
Review of attachment 272950 [details] [review]: Looks good.
Review of attachment 272949 [details] [review]: Fine by me. Although shouldn't we remove at this point the response callback or override?
(In reply to comment #8) > Review of attachment 272946 [details] [review]: > > See comment. > > ::: gitg/commit/gitg-commit-dialog.vala > @@ +680,3 @@ > Ggit.Diff? diff) > { > + Object(author: author, diff: diff, use_header_bar: 1); > > any reason to define this in the code and not in the ui file? Since we use templates, we needs to define this also in the code. I don't know why, I just know if I removed this from the code I not see any headerbar. (In reply to comment #11) > Review of attachment 272949 [details] [review]: > > Fine by me. Although shouldn't we remove at this point the response callback or > override? You mean we needs to keep the follwing line? <action-widget response="-7">button1</action-widget> (I think we needs to remove it, because we removed the button1 widget.)
Created attachment 273485 [details] [review] Use headerbar in the author details dialog
ok, I recalled right now the issue about construct properties with templates. Just one change I ask you: use_header_bar: true instead of 1.
(In reply to comment #14) > ok, I recalled right now the issue about construct properties with templates. > Just one change I ask you: use_header_bar: true instead of 1. The use-header-bar property is gint, not gboolean. In Vala: gitg/gitg-author-details-dialog.vala:46.12-46.31: error: Cannot convert from `bool' to `int' Object (use_header_bar: true); ^^^^^^^^^^^^^^^^^^^^ https://developer.gnome.org/gtk3/3.12/GtkDialog.html#GtkDialog--use-header-bar
From the docs: For technical reasons, this property is declared as an integer property, but you should only set it to TRUE or FALSE. Lame... technical reasons should not affect good API
(In reply to comment #16) > From the docs: > For technical reasons, this property is declared as an integer property, but > you should only set it to TRUE or FALSE. > > Lame... technical reasons should not affect good API I think it work only in C, because in Vala true and false itn't an integer, right? I guess in the .vapi file there is int and not bool property, so it is.
Yes, you could maybe try (int)true instead of 1, but I don't think it's really worth it.
I think it more clean to write '1' and not try to convert types. btw, how I can to change the type of the last file I attached? I forgot to mark it at patch.
Thanks. When you finish to review the patches, I'll push them together.
Review of attachment 273485 [details] [review]: Looks good.
Review of attachment 272948 [details] [review]: Looks good.
Review of attachment 272946 [details] [review]: Actually it looks good.
Review of attachment 272949 [details] [review]: I meant this here: https://git.gnome.org/browse/gitg/tree/gitg/gitg-application.vala#n244 I think that callback is not needed anymore, can you check?
Created attachment 274236 [details] [review] Use headerbar in the preferences dialog
Review of attachment 274236 [details] [review]: Looks good.
Review of attachment 272945 [details] [review]: Pushed as bc2c61b - Bump requirements for GTK+
Review of attachment 272946 [details] [review]: Pushed as ce5c424 - Use headerbar in the commit dialog
Review of attachment 272948 [details] [review]: Pushed as 1b4b38e - Use headerbar in the clone dialog
Review of attachment 272950 [details] [review]: Pushed as 9504e8f - Use popver for the gear menu
Review of attachment 273485 [details] [review]: Pushed as 05564e6 - Use headerbar in the author details dialog
Review of attachment 274236 [details] [review]: Pushed as fd9d6c7 - Use headerbar in the preferences dialog