GNOME Bugzilla – Bug 706050
Port the toolbar to GtkBox and make him as titlebar and add close button
Last modified: 2013-08-29 09:21:22 UTC
GtkHeaderBar no support in expand the child (he support just width fixed), so for style as headerbar should to port the toolbar to GtkBox instead. See patches.
Created attachment 251689 [details] [review] ephy-toolbar: Use GtkBox instead GtkToolbar
Created attachment 251690 [details] [review] ephy-window: Fix the margin of the toolbar
Created attachment 251691 [details] [review] ephy-toolbar: Add separator and close button
Created attachment 251692 [details] [review] ephy-window: Make the top bar as titlebar
Created attachment 251693 [details] Screenshot
I like the screenshot, thanks for doing this. I first need to know the opinion ofthe design team.
I Have a two problems: - the background of the top bar is becomes transparent (bug #706045) - I don't found the place to handle in web-application, for the mockup: https://raw.github.com/gnome-design-team/gnome-mockups/master/web-apps/web-apps.png - now, there not frame to window in web-application mode.
(In reply to comment #7) > - I don't found the place to handle in web-application, > for the mockup: > https://raw.github.com/gnome-design-team/gnome-mockups/master/web-apps/web-apps.png You need to create a web application first. For example, create one for bugzilla by going to bugzilla.gnome.org, ctrl+shift+A, save, and launch. This will probably launch your system's ephy, so you might need to launch it by hand. For the full command-line parameters, you should inspect the .desktop file created inside ~/.config/epiphany/app-[APPNAME]-[HASHTAG]/epiphany-[APPNAME]-[HASHTAG].desktop > - now, there not frame to window in web-application mode. So you managed to test app mode? Then I don't understand what your previous problem. Have you tried also the incognito mode? We theme it dark and it might not work out of the box, so perhaps some updates to the theme might be necessary. Please test that as well.
(In reply to comment #8) > (In reply to comment #7) > > - I don't found the place to handle in web-application, > > for the mockup: > > https://raw.github.com/gnome-design-team/gnome-mockups/master/web-apps/web-apps.png > > You need to create a web application first. For example, create one for > bugzilla by going to bugzilla.gnome.org, ctrl+shift+A, save, and launch. > > This will probably launch your system's ephy, so you might need to launch it by > hand. For the full command-line parameters, you should inspect the .desktop > file created inside > ~/.config/epiphany/app-[APPNAME]-[HASHTAG]/epiphany-[APPNAME]-[HASHTAG].desktop > > > - now, there not frame to window in web-application mode. > > So you managed to test app mode? Then I don't understand what your previous > problem. I noted only two problems. This part of the one problem - "there not frame to window in web-application mode". I not find ** in the code ** where window of web-application is defined. > > Have you tried also the incognito mode? We theme it dark and it might not work > out of the box, so perhaps some updates to the theme might be necessary. Please > test that as well. Yes, incognito mode is works find. you want screenshot?
Created attachment 251706 [details] [review] ephy-toolbar: The width of address bar need to be 530px This after comment from Allan Day.
Created attachment 251707 [details] Screenshot - after comment of Allan Day
(In reply to comment #11) > Created an attachment (id=251707) [details] > Screenshot - after comment of Allan Day This new look is quite nice. However, I was wondering what will happen then with the title provided by the webpage. From the screenshot it seems that we are now losing it. I suppose, it will be hold in the (small) space provided by the tab. Then, I suppose we should have the tabs bar open even when there is only one and we should provide a tooltip text with the title?
We already don't display the page title for anything that is docked if tabs are not visible, so in a sense we are suffering this problem already. But I agree it would be good to have into account this issue and how are we going to deal with when changing the chrome for the undocked-window case. Allan?
Created attachment 251717 [details] [review] ephy-toolbar: Add margin on the left and right of address bar This ensures always have space to drag the window.
How about doing something similar to (fullscreen) Totem or Firefox 23 mobile? We could slide in / out an infobar with the page title when the user reaches the top or bottom of the page.
Now, epiphany in fullscreen isn't showing the top bar and the notebook bar, and after my patch, this behavior not changes. Add slide in / out an infobar is not belongs to this issues. My patch isn't change many stuff. Just need to care in chrome for the undocked-window case. I also use the box as titlebar in web-application, with close button, by the mockup: https://raw.github.com/gnome-design-team/gnome-mockups/master/web-apps/web-apps.png
Created attachment 252063 [details] [review] ephy-window: Less padding in sides of the top bar
Created attachment 252064 [details] [review] ephy-toolbar: Fix the passing of the buttons in the top bar
Created attachment 252065 [details] Screenshot - after more comment of Allan Day
Created attachment 252066 [details] [review] configure.ac: require GTK+ 3.9.11
You need to squeeze these patches into a new series of atomic changes, so that it can be reviewed.
Created attachment 252077 [details] [review] ephy-toolbar: Use GtkBox instead GtkToolbar and add close button
Created attachment 252078 [details] [review] ephy-window: Make the top bar as titlebar
Created attachment 252087 [details] [review] ephy-toolbar: Add title and reload/stop button for webapp
Created attachment 252088 [details] [review] ephy-window: Always show the top bar and show/hide title according mode window I think need to remove "show-toolbars" setting, and always to show the top bar.
Created attachment 252093 [details] Screenshot - The new design of web-app The mockup: https://raw.github.com/gnome-design-team/gnome-mockups/master/web-apps/web-apps.png
Created attachment 252101 [details] [review] ephy-toolbar: Use GtkBox instead GtkToolbar and add close button
Created attachment 252538 [details] modified screenshot I've posted some layout guidelines for HeaderBars on the wiki: https://wiki.gnome.org/Design/Whiteboards/HeadeBarGuidelines I'm also attaching a modified version of one of Yosef's screenshots, to show what we're aiming for (original is on top, modified version is below).
Created attachment 252667 [details] [review] ephy-toolbar: Add title and reload/stop button for webapp This make the title of web-app Bold gray, as the mockup.
Review of attachment 252078 [details] [review]: OK.
Review of attachment 252667 [details] [review]: This fiddles with EphyToolbar and ads API and UI elements that are specific to the application mode and that will be present even when not in application mode. I am not sure this is the best approach. ::: src/ephy-toolbar.c @@ +201,3 @@ + /* Title of the window in web application - need + * settings of padding same the location entry */ + priv->label = gtk_label_new ("https://www.gnome.org/"); I don't think we need this default value? @@ +225,3 @@ + action); + gtk_container_add (GTK_CONTAINER (toolbar), priv->reload); + If these new UI elements are used only in web application mode, why do we always add them? Shouldn't that be conditional to the mode used? @@ +338,3 @@ + return toolbar->priv->label; +} + I don't think it is right to add these methods; what will be done with them when application mode is not in use? @@ +341,3 @@ +void +ephy_toolbar_set_title_web_app (EphyToolbar *toolbar, + const gchar *title) While I am not sure yet what this method is for, I think the right name would be ephy_toolbar_set_web_application_title(). I stil don't like the fact that is web application specific API in a toolbar that is used both for normal and app. mode.
Review of attachment 252088 [details] [review]: This and the previous patch need improving in terms of encapsulation. ::: src/ephy-window.c @@ +726,3 @@ + "visible", !show_address_bar, NULL); + g_object_set (ephy_toolbar_get_reload_web_app (EPHY_TOOLBAR (priv->toolbar)), + "visible", !show_address_bar, NULL); This is definitely not right. It is a very different thing to have a widget and change its visible property than to externally modify the visible property of widgets that belong to that widget. The former is OK, the latter is not clean. One different way to do this would be to add a mode property to the EphyToolbar which would be set here, and internally the toolbar would modify its composition. This way the methods exposing these internals specific to application mode, in the previous patch, would not be needed. @@ +1701,3 @@ + + gtk_window_set_title (GTK_WINDOW(window), title); + ephy_toolbar_set_title_web_app (EPHY_TOOLBAR (priv->toolbar), title); Maybe it is possible to do this by adding a title property to the toolbar and binding it with the window title property? @@ +3971,3 @@ + ephy_action_change_sensitivity_flags (action, SENS_FLAG_CHROME, + TRUE); + gtk_action_set_visible (action, FALSE); What is this for?
Created attachment 252684 [details] [review] ephy-toolbar: Use GtkBox instead GtkToolbar and add close button
Created attachment 252685 [details] [review] ephy-window: Make the top bar as titlebar I change the margin.
Created attachment 252686 [details] [review] ephy-toolbar: Add title and reload/stop button for webapp I happen this patch is better.
Created attachment 252687 [details] [review] ephy-toolbar: Add title and reload/stop button for webapp I happen this patch is better.
Created attachment 252688 [details] [review] ephy-window: Always show the top bar and show/hide title according mode window I added comment why I whoe the "PageMenu" action: The page menu not need to be dispalyed in application mode
Review of attachment 252066 [details] [review]: Please squash this into the first patch that requires 3.9.11 API.
Created attachment 252705 [details] [review] ephy-toolbar: Use GtkBox instead GtkToolbar and add close button
Review of attachment 252687 [details] [review]: Looks better. ::: src/ephy-toolbar.c @@ +259,3 @@ gtk_widget_show_all (button); + /* Add title just in application mode */ "only" instead of "just". @@ +263,3 @@ + { + /* The title of the window in web application - need + * settings of padding same the location entry */ /* The title of the window in web application mode must have the same padding as the location entry. */
Review of attachment 252688 [details] [review]: You need to improve the commit log message. There are some typos that need fixing: In normal mode: show address bar and hide title and reload/stop buttons. In web application mode: hidde the address bar and show instead the page title and the reload/stop button. ::: src/ephy-window.c @@ +696,3 @@ if (ephy_embed_shell_get_mode (ephy_embed_shell_get_default ()) == EPHY_EMBED_SHELL_MODE_APPLICATION) { + *show_toolbar = TRUE; If we're always showing the toolbar, perhaps this could be removed? @@ +3955,3 @@ if (mode == EPHY_EMBED_SHELL_MODE_APPLICATION) { + /* The page menu not need to be dispalyed in application mode */ /* We don't need to show the page menu in web application mode. */ @@ +3958,3 @@ + action = gtk_action_group_get_action (toolbar_action_group, "PageMenu"); + ephy_action_change_sensitivity_flags (action, SENS_FLAG_CHROME, + TRUE); Nit: the boolean can go in the same line.
Created attachment 252707 [details] [review] ephy-toolbar: Add title and reload/stop button for webapp
Created attachment 252713 [details] [review] ephy-window: Always show the top bar in app mode and show/hide title according mode window
As mentioned in irc, I tested this and it works well, however one issue remains: if ephy is opened with an unmaximized window, the titlebar is missing and one needs to maximize/unmaximize to get it visible. This needs to be fixed.
Review of attachment 252685 [details] [review]: The message should read: Use the top bar GtkBox as titlebar. The GtkEventBox is to ensure opacity (see bug #706045) Also fix the margins in the top bar. ::: src/ephy-window.c @@ +3697,3 @@ + gtk_widget_set_margin_bottom (toolbar, 6); + gtk_widget_set_margin_left (toolbar, 8); + gtk_widget_set_margin_right (toolbar, 8); Please split the changes to the margins into a different patch. @@ +3701,3 @@ + gtk_event_box_set_visible_window (GTK_EVENT_BOX (event_box), TRUE); + gtk_style_context_add_class (gtk_widget_get_style_context (event_box), + "titlebar"); This is not needed anymore, as gtk_window_set_titlebar() does it for you.
Review of attachment 252705 [details] [review]: Please split the changes to add the new close button into a new patch, to make this cleaner. ::: src/ephy-toolbar.c @@ +83,3 @@ + GtkWidget *toplevel; + + toplevel = gtk_widget_get_toplevel (GTK_WIDGET (button)); I don't like doing this. We should probably keep a pointer to the window as a property of the toolbar, like nautilus does. Otherwise it is a bit hackish to get the toplevel this way, IMHO.
(In reply to comment #47) > Review of attachment 252705 [details] [review]: > > Please split the changes to add the new close button into a new patch, to make > this cleaner. > > ::: src/ephy-toolbar.c > @@ +83,3 @@ > + GtkWidget *toplevel; > + > + toplevel = gtk_widget_get_toplevel (GTK_WIDGET (button)); > > I don't like doing this. We should probably keep a pointer to the window as a > property of the toolbar, like nautilus does. Otherwise it is a bit hackish to > get the toplevel this way, IMHO. This copy-paste of the handling in close button in headerbar in GTK+: https://git.gnome.org/browse/gtk+/tree/gtk/gtkheaderbar.c#n214
Review of attachment 252707 [details] [review]: ::: src/ephy-toolbar.c @@ +96,3 @@ + pango_attr_list_unref (attrs); + + attrs = pango_attr_list_new (); Shouldn't you use GTK_STYLE_CLASS_DIM_LABEL ? Also, is it OK that the application title is always dimmed? @@ +128,3 @@ + + return button; +} Is this code copy/pasted from another file, right? Bad idea. If we're going to reuse this we might need to make it a proper widget or figure out a way to do this without replicating code. @@ +136,3 @@ GtkActionGroup *action_group; GtkAction *action; + GtkWidget *toolbar, *box, *button, *separator, *label;; Extra semicolon at the end. @@ +213,3 @@ gtk_box_pack_start (GTK_BOX (toolbar), box, TRUE, TRUE, 0); + if (mode != EPHY_EMBED_SHELL_MODE_APPLICATION) + gtk_widget_show_all (box); Is there any chance that we don't create this box at all (nor its contents) if we don't need them? @@ +235,3 @@ gtk_container_add (GTK_CONTAINER (toolbar), button); + if (mode != EPHY_EMBED_SHELL_MODE_APPLICATION) + gtk_widget_show_all (button); Same as above. @@ +259,3 @@ gtk_widget_show_all (button); + /* Add title only in application mode */ Add a period at the end of the sentence. @@ +263,3 @@ + { + /* The title of the window in web application mode must have the same + * padding as the location entry */ Same here. Also, remove the padding * from the second line, we don't use that in comments. @@ +267,3 @@ + gtk_style_context_add_class (gtk_widget_get_style_context (label), "subtitle"); + smallify_label (label); + /* The title of the window in web application mode must have the same Since you do this only once and to the same widget, you could merge the two methods into one and avoid creating two PangoAttrList unnecessarily. @@ +284,3 @@ + G_BINDING_SYNC_CREATE | G_BINDING_BIDIRECTIONAL); + + gtk_widget_set_size_request (label, 530, -1); Period missing.
Review of attachment 252713 [details] [review]: This is probably right.
Created attachment 253439 [details] [review] ephy-toolbar: Use GtkBox instead GtkToolbar
Created attachment 253440 [details] [review] wphy-window: Use the top bar GtkBox as titlebar
Created attachment 253441 [details] [review] ephy-window: Fix the margins in the top bar
Created attachment 253442 [details] [review] ephy-window: Always show the top bar in app mode and show/hide title according mode window
Created attachment 253450 [details] [review] ephy-toolbar: Add close button
Comment on attachment 252707 [details] [review] ephy-toolbar: Add title and reload/stop button for webapp I suppose this one is obsolete.
(In reply to comment #56) > (From update of attachment 252707 [details] [review]) > I suppose this one is obsolete. Indeed.
Review of attachment 253439 [details] [review]: Looks good.
Review of attachment 253440 [details] [review]: Good. ::: src/ephy-window.c @@ +3701,1 @@ I have a patch to fix the issue with the invisible bar (the event box needs to be shown explicitly, it seems).
Review of attachment 253441 [details] [review]: OK.
Review of attachment 253442 [details] [review]: Good.
Review of attachment 253450 [details] [review]: I think it's good.
Review of attachment 253439 [details] [review]: pushed as 2f7bc20a4d95406df0bd1a1e4fd6cd98ba18d253 - ephy-toolbar: Use GtkBox instead GtkToolbar
Review of attachment 253440 [details] [review]: pushed as a93f0f4947ace896ab040d6ae7c54216786fa4c4 - wphy-window: Use the top bar GtkBox as titlebar
Review of attachment 253441 [details] [review]: pushed as d2b81dfca703ba9d6f0dc34a89884ca66c4aad74 - ephy-window: Fix the margins in the top bar
Review of attachment 253442 [details] [review]: pushed as d42ace6d967556cef7675de7414fb16938530497 - ephy-window: Always show the top bar in app mode and show/hide title according mode window
Review of attachment 253450 [details] [review]: pushed as 67bbe86ab6b15b7d3552d2d6d4d9519ba3136b65 - ephy-toolbar: Add close button