GNOME Bugzilla – Bug 740426
Use a header bar
Last modified: 2017-08-10 20:05:43 UTC
Eye of GNOME is one of the last remaining core applications still in the "old" style. It would be really nice to give it a UI refresh, so it is consistent with the other apps, and to give it a more modern look and feel. A tentative design can be found here: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/eye-of-gnome/new/eye-of-gnome.png Some existing menu items are currently missing from the mockups. I've got notes on what's been removed and why, and we can discuss anything that needs adding to the design.
Is this already something being worked on by someone? I have been long looking to contribute to GNOME and this is a personal itch. It would be my first contribution, but I'm eager to have a go at this and by doing so, making my first contribution. It's not a GNOME Love bug, but I think it's achievable for me nonetheless. So, if nobody is working on this yet, I'd like to have a go at it if allowed.
You don't have to ask for permission - your contribution will be most welcome!
Awesome, thank you. I have already begun working. I will post back here when I have made some progress that is worth showing, to get some feedback as to whether I'm on the good track.
Created attachment 291848 [details] [review] Work-in-progress patch for GtkHeaderBar Here's a work-in-progress patch. I'm posting it now because there is enough work done to get some feedback to see if I'm on the right track. So far, I have completely removed the toolbar and those menu items that can now be found in the new headerbar or gear menu. Some items, however, are still missing in the gear menu (recent images, open with and slideshow). That is because I haven't figured out yet how to do submenus with the current code. I guess I'll have to switch everything over to the new GtkPopoverMenu? I would also like to know what to do with the menu items that are missing from the mockups. If you could be so kind to share your notes, Allan. Lastly, there is a large amount of deprecated GtkAction objects, some of which can perhaps be dropped if they do not fit in the new design. Still, many of them will have to be ported over to GAction. However, I'm not sure if that is directly related to this bug. Thus, my question is: should I open a new bug report for that, or can I make the required changes in this patch? I would prefer the latter to keep things simple for me, but I understand the "change one thing per patch"-approach. Well, that's that! I look forward to your feedback and to continue with my first contribution :)
I just noticed that a second mockup has been added. I take it the design is not fully decided on, then. Is there a public discussion on this? I'd like to follow it, if possible. I'll go ahead and implement a GtkPopoverMenu meanwhile, and will leave the rest as is as much as possible until the design is decided on and I've had some feedback from someone :)
Hi Jente, I only noticed just now that you started working on this. Yes, the "full" design (which the header bar is a part of) is still a work in progress. However there's no talk of not migrating to a header bar. So, help is appreciated here. :) I had no time to look at your code yet, sorry. I'll try to do it over the next days. But I gave it a quick build and the result looks promising. You also already included the zoom scale which is great as that's a piece of the design we wanted to try out how it works. The GtkActions can probably go as there's not much use for them once the UI fully switches to GAction. But that should be done in a separate ticket as it is a deeper change and also affects the plugins. It's also possible to have the GtkAction ticket block this one to have a clean GtkAction->GAction migration if you like. Also don't hesitate to attach patch series. Smaller patches are easier to review. Submenus in Popovers can be done using a <submenu> tag in the ui description. I haven't seen a GtkPopoverMenu in action yet, so I can't tell right now how it differs from the standard popover. Regarding the menu contents, we might have to iterate a bit anyway. For example I am not sure if the "classic" recent files menu works well in a popover or whether we might need something like gedit has.
Hi Felix, Sure, take your time with reviewing! I must admit that I couldn't wait to continue the work, so I have already started on porting GtkAction to GAction. It is relatively straightforward and the basics seem to be working. I only need to fix the {action,toggle}_entries_image and there are some minor fixes left in the other (stateful) actions. A slight problem is that I made these adjustments in the same branch as the headerbar, to avoid having build problems with the menu- and toolbar. I don't know if making a selective diff is possible, so I guess I'll checkout a new branch and copy my changes one by one. Oh well, the pain of enthusiasm! (and being impatient, of course :p) I have indeed found out how to make submenus, but as of yet have not filled them. Before I do this, I am going to finish the GtkAction -> GAction port but I will make a separate ticket for this and migrate my work tomorrow. Is there a place to read the discussion on the new design?
(In reply to comment #5) > I just noticed that a second mockup has been added. ... Hi Jente! I'm really sorry for not keeping you more informed about this; I've been discussing the design with Felix in the past couple of days, and didn't get chance to update the bug. I don't have great connectivity right now, but I'll definitely try your patches as soon as I'm able. Thanks for working on this!
Review of attachment 291848 [details] [review]: Allright, so far I couldn't find anything really bad. :) Something that is a bit awkward is that some internal defines of EogScrollView are now public. That can be made nice later though. ::: src/eog-window.c @@ +488,3 @@ + zoom = eog_scroll_view_get_zoom (EOG_SCROLL_VIEW (priv->view)); + g_signal_handlers_block_by_func (priv->zoom_scale, + eog_window_zoom_scale_value_changed_cb, window); Not sure whether you really have to block the signal handler, as the zoom factor you get from the scroll view shouldn't change when you re-set it again in the handler. @@ +4684,3 @@ + if (toggled) { + eog_scroll_view_set_zoom (EOG_SCROLL_VIEW (priv->view), + EOG_WINDOW_INITIAL_ZOOM); This shouldn't be the initial but the current zoom factor. @@ +4824,3 @@ + + priv->zoom_scale = gtk_scale_new_with_range (GTK_ORIENTATION_HORIZONTAL, + MIN_ZOOM_FACTOR, 1.0, IMAGE_VIEW_ZOOM_MULTIPLIER); This limits the zoom to 100% while the current upper limit is 2000%. Might become interesting though if the scale doesn't become to "fast" then. :/ We might also have to set a marker to easier find 100% later. @@ -5420,3 @@ } - case GDK_KEY_Return: - if (gtk_container_get_focus_child (tbcontainer) == NULL) { I don't think you have to remove the Return hotkey at this moment. Removing the focus child check should be enough for now.
(In reply to comment #9) > ::: src/eog-window.c > @@ +488,3 @@ > + zoom = eog_scroll_view_get_zoom (EOG_SCROLL_VIEW (priv->view)); > + g_signal_handlers_block_by_func (priv->zoom_scale, > + eog_window_zoom_scale_value_changed_cb, window); > > Not sure whether you really have to block the signal handler, as the zoom > factor you get from the scroll view shouldn't change when you re-set it again > in the handler. > > @@ +4684,3 @@ > + if (toggled) { > + eog_scroll_view_set_zoom (EOG_SCROLL_VIEW (priv->view), > + EOG_WINDOW_INITIAL_ZOOM); > > This shouldn't be the initial but the current zoom factor. Oh, oke. I misunderstood that from Allan's mockup where it says "the image is zoomed to a preset amount". Easy to change, tho :) > @@ +4824,3 @@ > + > + priv->zoom_scale = gtk_scale_new_with_range (GTK_ORIENTATION_HORIZONTAL, > + MIN_ZOOM_FACTOR, 1.0, IMAGE_VIEW_ZOOM_MULTIPLIER); > > This limits the zoom to 100% while the current upper limit is 2000%. > Might become interesting though if the scale doesn't become to "fast" then. :/ > We might also have to set a marker to easier find 100% later. Yes, I agree with this. It was already something that was on my mind: I had it to the full 2000% before, but the zooming via the slider is way too sensitive then. We could change the stepping, but then it won't be as smooth anymore. > @@ -5420,3 @@ > } > - case GDK_KEY_Return: > - if (gtk_container_get_focus_child (tbcontainer) == NULL) { > > I don't think you have to remove the Return hotkey at this moment. Removing the > focus child check should be enough for now. This is already reverted in my local version. Thanks for the review! I will incorporate your changes once I'm done with bug 741050
I've got the patch running now - looks very nice indeed. I can give design feedback when you're ready.
As noted in bug 741050, comment 45 the migration to GAction is now in a far enough state to start working on updating the UI. Allan, Felix: has there been a decision yet on which mockup has to be implemented?
(In reply to comment #12) > Allan, Felix: has there been a decision yet on which mockup has to be > implemented? As I've done some preparations already, I am currently targeting the variant with overlaid controls.
Alright. I'll start working on updating my headerbar patch, then. Which features specifically are you working on? Just so we won't overlap our efforts.
(In reply to comment #14) > Alright. I'll start working on updating my headerbar patch, then. Great! Thanks in advance. :) > Which features specifically are you working on? Just so we won't overlap our > efforts. I did some prototyping for the overlaid controls (since the toolbar's gone), which when combined with your headerbar was already able to look pretty close to Allan's design. They still need some cleanup and bugfixing and have ties to EogWindow, so I am not sure that we won't get in each others way. Also the metadata sidebar is going to get a different look (not visible in the linked image), but that work is completely independent from the headerbar. So, I guess I'll just continue working on the sidebar for now and update the rest once the headerbar is in.
Created attachment 294066 [details] [review] Implement headerbar An updated patch that cleanly applies to the current master. > Something that is a bit awkward is that some internal defines of EogScrollView > are now public. That can be made nice later though. This is still in. I rather define something globally once, than redefining something on multiple places privately. > @@ +4684,3 @@ > + if (toggled) { > + eog_scroll_view_set_zoom (EOG_SCROLL_VIEW (priv->view), > + EOG_WINDOW_INITIAL_ZOOM); > > This shouldn't be the initial but the current zoom factor. This is still in the current patch. The mockups say: "Zoom button acts as a toggle. When activated, the image is zoomed by a preset amount, and the slider is displayed. Pressing the zoom button again resets the view to the default zoom level and hides the slider", so I'm not sure about setting the current zoom factor. You can play around with this yourself and see what you like best, I'll leave this up to you and Allan. To test the other behaviour, simply comment out the above piece of code and change the "else" to "if (!toggled)" accordingly. > @@ +4824,3 @@ > + > + priv->zoom_scale = gtk_scale_new_with_range (GTK_ORIENTATION_HORIZONTAL, > + MIN_ZOOM_FACTOR, 1.0, IMAGE_VIEW_ZOOM_MULTIPLIER); > > This limits the zoom to 100% while the current upper limit is 2000%. > Might become interesting though if the scale doesn't become to "fast" then. :/ > We might also have to set a marker to easier find 100% later. The maximum zoom value is still 100%. I tried it on 2000% again, but the zooming goes way too fast for my taste. To try for yourselves, just change the 1.0 in the above line to 20.0 or MAX_ZOOM_FACTOR. What I think could be a nice middle way is to have the current step value when the range is [0, 100] and a larger step value for values > 100 - this would make zooming nice and smooth on (what I think are) the most used range while still enabling larger zoom values. I have also tried adding a mark: gtk_scale_add_mark (GTK_SCALE (priv->zoom_scale), 1.0, GTK_POS_BOTTOM, NULL); but this messes with the drawing of the slider and the headerbar, which I don't know how to resolve. Also, please take a look at the FIXME in the code: FIXME: the scale by itself does not take any width, so we manually set it here. There has to be a better way... Looking forward to the feedback! Also, can you give me some pointer on how to connect the "Recent images" and "Open with" menus?
(In reply to comment #16) > Created an attachment (id=294066) [details] [review] > Implement headerbar > > An updated patch that cleanly applies to the current master. > Hi Jente, it seems you forgot to include the eog-gear-menu.ui file in the patch. ;) It also introduces a few lines with trailing spaces in EogWindow. git usually tells you about them during committing. > > Something that is a bit awkward is that some internal defines of EogScrollView > > are now public. That can be made nice later though. > > This is still in. I rather define something globally once, than redefining > something on multiple places privately. > > > @@ +4684,3 @@ > > + if (toggled) { > > + eog_scroll_view_set_zoom (EOG_SCROLL_VIEW (priv->view), > > + EOG_WINDOW_INITIAL_ZOOM); > > > > This shouldn't be the initial but the current zoom factor. > > This is still in the current patch. The mockups say: "Zoom button acts as a > toggle. When activated, the image is zoomed by a preset amount, and the slider > is displayed. Pressing the zoom button again resets the view to the default > zoom level and hides the slider", so I'm not sure about setting the current > zoom factor. You can play around with this yourself and see what you like best, > I'll leave this up to you and Allan. To test the other behaviour, simply > comment out the above piece of code and change the "else" to "if (!toggled)" > accordingly. The button's desired behaviour is IMHO explainable from eog's current zooming behaviour. eog basically distinguishes two different zooming states: best-fit and free-zoom. Best fit is always the default for the image and thus would be the zoom button with the scale hidden. Clicking the button and showing the scale would enter the free-zoom mode, where the zoom factor can be more or less freely adjusted. Clicking the button again would return to best-fit mode (hiding the scale). > > @@ +4824,3 @@ > > + > > + priv->zoom_scale = gtk_scale_new_with_range (GTK_ORIENTATION_HORIZONTAL, > > + MIN_ZOOM_FACTOR, 1.0, IMAGE_VIEW_ZOOM_MULTIPLIER); > > > > This limits the zoom to 100% while the current upper limit is 2000%. > > Might become interesting though if the scale doesn't become to "fast" then. :/ > > We might also have to set a marker to easier find 100% later. > > The maximum zoom value is still 100%. I tried it on 2000% again, but the > zooming goes way too fast for my taste. To try for yourselves, just change the > 1.0 in the above line to 20.0 or MAX_ZOOM_FACTOR. What I think could be a nice > middle way is to have the current step value when the range is [0, 100] and a > larger step value for values > 100 - this would make zooming nice and smooth on > (what I think are) the most used range while still enabling larger zoom values. Yes, I was thinking about something like that too, but GTK's slider doesn't support such a behaviour currently I think. Maybe we have to rethink the slider anyways as zooming via mousewheel or touch gesture seems preferable to since the focus will follow my zoom target and for setting a specific factor the slider might turn out to be too imprecise (at least without a label showing the factor). > I have also tried adding a mark: > gtk_scale_add_mark (GTK_SCALE (priv->zoom_scale), 1.0, GTK_POS_BOTTOM, NULL); > > but this messes with the drawing of the slider and the headerbar, which I don't > know how to resolve. > > Also, please take a look at the FIXME in the code: > FIXME: the scale by itself does not take any width, so we manually set it here. > There has to be a better way... > > Looking forward to the feedback! Also, can you give me some pointer on how to > connect the "Recent images" and "Open with" menus? Hmm, regarding the recent files menu I only found that evince hat something[1] done for that, however I don't know how that looks as the menu was replaced by that new bookshelf view when you open Evince without any file. Maybe we can salvage something from that. I think it shows a way for how to make changing menu entries which is something we'll need for the open-with-menu as well. [1]: https://git.gnome.org/browse/evince/commit?id=8bb5fce0754aa0d2dc795d2d21ec778888dc3714
Created attachment 294126 [details] [review] Implement headerbar (In reply to comment #17) > Hi Jente, it seems you forgot to include the eog-gear-menu.ui file in the > patch. ;) Argh. Here's an updated one that adds it. > It also introduces a few lines with trailing spaces in EogWindow. git usually > tells you about them during committing. Weird, Git never told me anything. It should also highlight them in git diff, but it doesn't. I'll take a look tomorrow. > The button's desired behaviour is IMHO explainable from eog's current zooming > behaviour. eog basically distinguishes two different zooming states: best-fit > and free-zoom. Best fit is always the default for the image and thus would be > the zoom button with the scale hidden. Clicking the button and showing the > scale would enter the free-zoom mode, where the zoom factor can be more or less > freely adjusted. Clicking the button again would return to best-fit mode > (hiding the scale). So, what to do? Zoom to the current zoom factor on revealing the slider, and go back to EOG_ZOOM_MODE_SHRINK_TO_FIT when hiding it? > Yes, I was thinking about something like that too, but GTK's slider doesn't > support such a behaviour currently I think. Can't we use gtk_range_set_increments (GtkRange *range, gdouble step, gdouble page) conditionally, to update the step level? Or perhaps this only applies to the arrow/Pg* keys. > Maybe we have to rethink the slider anyways as zooming via mousewheel or touch > gesture seems preferable to since the focus will follow my zoom target and for > setting a specific factor the slider might turn out to be too imprecise (at > least without a label showing the factor). I know my preference for zooming is just scrolling with the mousewheel. Removing the slider does mean there is no more indicator of the current zoom level, as the statusbar is gone in the mockups. > Hmm, regarding the recent files menu I only found that evince hat something[1] > done for that, however I don't know how that looks as the menu was replaced by > that new bookshelf view when you open Evince without any file. Maybe we can > salvage something from that. I think it shows a way for how to make changing > menu entries which is something we'll need for the open-with-menu as well. > > [1]: > https://git.gnome.org/browse/evince/commit?id=8bb5fce0754aa0d2dc795d2d21ec778888dc3714 Thanks, I'll take a look tomorrow!
Review of attachment 294126 [details] [review]: (In reply to comment #16) > I have also tried adding a mark: > gtk_scale_add_mark (GTK_SCALE (priv->zoom_scale), 1.0, GTK_POS_BOTTOM, NULL); > > but this messes with the drawing of the slider and the headerbar, which I don't > know how to resolve. Hmm, yeah I tried myself, and although it draws the marker it is hardly visible. The elevated scale doesn't look so great as well, so far. And the slider also appears slightly offset when it stops at the marker. The former can probably be handled with some custom CSS. Not sure about the latter. > Also, please take a look at the FIXME in the code: > FIXME: the scale by itself does not take any width, so we manually set it here. > There has to be a better way... No, I think there isn't. Even GtkScaleButton seems to use a fixed size for the scale. And if it used the whole space it would extend all the way to the right and even move the title label away. The question here is what the likely best size would be. One also has to watch out for the minimum window size here, as it could force the window to enlarge if it doesn't provide enough space. (In reply to comment #18) > > It also introduces a few lines with trailing spaces in EogWindow. git usually > > tells you about them during committing. > > Weird, Git never told me anything. It should also highlight them in git diff, > but it doesn't. I'll take a look tomorrow. Well, it does at least for me when I git am your patch, so I assumed it would do while committing as well. Anyway, you can check your changes with git gui, it will mark them in the diff view. > So, what to do? Zoom to the current zoom factor on revealing the slider, and go > back to EOG_ZOOM_MODE_SHRINK_TO_FIT when hiding it? It shouldn't zoom at all when toggling. It should only reveal the scale with the slider set at the current zoom factor. Untoggling should return to SHRINK_TO_FIT and hide the slider. > Can't we use gtk_range_set_increments (GtkRange *range, gdouble step, gdouble > page) conditionally, to update the step level? Or perhaps this only applies to > the arrow/Pg* keys. No, these affect the arrow and page keys and the mousewheel only. Anyways, if the zoom slider is the only left holding the headerbar back we can think about letting the headerbar go in without it and try finding a better solution later. We could also temporarily add zoom control entries to the gear menu (or a temporary menu button just for zooming) until we sort that out. ::: data/eog-gear-menu.ui @@ +1,3 @@ +<?xml version="1.0"?> +<interface> + <menu id="gear-menu"> We probably also need the save, save as and properties entries, as they would be available via the right click menus only. @@ +4,3 @@ + <section> + <item> + <attribute name="label" translatable="yes">Open…</attribute> Oh, I think we should keep the underscore accelerators in the menu for now (e.g. _Open…, _Print, ...) @@ +22,3 @@ + </submenu> + <item> + <attribute name="label" translatable="yes">Print</attribute> This opens another dialog and thus requires an ellipsis (...) as well. @@ +26,3 @@ + </item> + <item> + <attribute name="label" translatable="yes">Set as Background</attribute> We should probably use the term "Wallpaper" instead here. The reason is, that the image as shown in eog also has a "background" layer (which is called like that in the preferences window), whose color can be changed by the user.
Created attachment 294313 [details] [review] Implement headerbar This version adds underscore accelerators back in. I generally picked those that are present the current stable release, except for the wallpaper option (conflicted with slideshow) and the recent images had none so I picked one. I also picked an order for the Save, Save As... and Properties... options in the gear menu myself, but feel free to change them as you see fit. The scale-toggling now acts as it should and the spaces are removed. Only thing left to do now is the slider.
Created attachment 294314 [details] [review] Implement headerbar Whoops, copy-pasta gone wrong. This is the proper new version.
Created attachment 294550 [details] [review] Implement fullscreen headerbar I went ahead and replaced the GtkToolbar that shows when Eye of Gnome is fullscreen, with a GtkHeaderBar. As far as I know, there is no mockup for this so I just replaced all the GtkToolItems et cetera with their regular counterparts and used symbolic icons. Is there a mockup available, or can one be created? Or we can decide on something and I can implement it right away. I looked at how Gedit does it, and their fullscreen headerbar basically mimics the regular one. I really like this approach. One difference I noted is that their headerbar has no rounded corners, which I think they did with some CSS magic.
(In reply to comment #22) > Created an attachment (id=294550) [details] [review] > Implement fullscreen headerbar > > I went ahead and replaced the GtkToolbar that shows when Eye of Gnome is > fullscreen, with a GtkHeaderBar. As far as I know, there is no mockup for this > so I just replaced all the GtkToolItems et cetera with their regular > counterparts and used symbolic icons. > Is there a mockup available, or can one be created? Or we can decide on > something and I can implement it right away. I looked at how Gedit does it, and > their fullscreen headerbar basically mimics the regular one. I really like this > approach. One difference I noted is that their headerbar has no rounded > corners, which I think they did with some CSS magic. Oh, wow. :) No, there's no mockup yet, but it's not a hard requirement. However, we should see whether Allan could provide some input. We should move this to a new ticket thoug, as the fullscreen toolbar is still independent from the normal tool/headerbar for me and this ticket starts to get pretty long for that one already. The fullscreen toolbar was actually something I wanted get back to later, as its concept will likely need some rethinking. Pretty much half of the current buttons can probably go once the overlaid controls are in place and we'll see what's missing once the new menus are in place.
Review of attachment 294314 [details] [review]: (In reply to comment #20) > This version adds underscore accelerators back in. I generally picked those > that are present the current stable release, except for the wallpaper option > (conflicted with slideshow) and the recent images had none so I picked one. I > also picked an order for the Save, Save As... and Properties... options in the > gear menu myself, but feel free to change them as you see fit. Yes, sure. Picking an unconflicting one is fine. They will change anyway when the menu is translated. > The scale-toggling now acts as it should and the spaces are removed. Only thing > left to do now is the slider. Allright. So far it looks pretty complete, so I guess we could let the current state go in for now. That way we can play a bit with the new design and experimenting with the slider will be easier. ::: data/eog-gear-menu.ui @@ +8,3 @@ + </item> + <item> + <attribute name="label" translatable="yes">_Save…</attribute> Oh, I think I mislead you a bit here. The ellipsis is not needed for every action that might open a dialog. It's just used when further user input is required before the desired action is executed. So, the simple save and the properties dialog don't need the dots. Sorry! I'll fix that. ::: src/eog-scroll-view.h @@ +16,3 @@ + */ +#define IMAGE_VIEW_ZOOM_MULTIPLIER 1.05 + We might have to change their naming later, if they stay here (depends on the slider), since the current labeling is not supported by GObject introspection. ::: src/eog-window.c @@ +87,3 @@ #define EOG_WINDOW_FULLSCREEN_POPUP_THRESHOLD 5 +#define EOG_WINDOW_INITIAL_ZOOM 0.85 This is not needed anymore. @@ +4515,3 @@ + gtk_header_bar_set_show_close_button (GTK_HEADER_BAR (headerbar), TRUE); + gtk_header_bar_set_title (GTK_HEADER_BAR (headerbar), + g_get_application_name ()); Some minor nitpicking about the indentation here: If you have to linebreak the parameters, try to align the new lines with the first one. That way it stays a bit more consistent with the rest of eog's code.
And pushed with the mentioned improvements. Thanks again, Jente! :) commit de5ee93aa325bc29a2e00e8d874af97cebb10cd6 Author: Jente Hidskes <hjdskes@gmail.com> Date: Wed Jan 14 23:09:31 2015 +0100 EogWindow: implement headerbar https://bugzilla.gnome.org/show_bug.cgi?id=740426
There is still a conflict with the underscore accelerators: Wallpaper and Open With both use _W. Sorry for not noticing this before!
(In reply to comment #26) > There is still a conflict with the underscore accelerators: Wallpaper and Open > With both use _W. > > Sorry for not noticing this before! No problem! Should be resolved now.
Oh, forgot to close this ticket. Just wanted to say thanks to both of you for the help that made this happen in time for 3.16. Thanks! :)
Felix Riemann: So, is there a bug for fullscreen headerbar?
(In reply to Mikhaylenko Alexander from comment #29) > Felix Riemann: So, is there a bug for fullscreen headerbar? Let's consider that a part of bug 780530