GNOME Bugzilla – Bug 711408
Difficult to see web site title with the new toolbar/header bar
Last modified: 2014-07-26 09:25:39 UTC
Now that we are using the header bar the only way to see the website title is the tab label which is usually not enough to show the whole title.
We could show the title in a tooltip when hovering a tab at least.
I will fix this in bug 708979 (when I have a patch). See also bug 709444. About a tooltip, I'll ask the designers.
Hey Carlos, are there any specific examples where the page title is essential? Either way, I think it would be nice to try and find a way to show the page title in the header bar without resorting to a not-so-hip tooltip. :)
I think the main problem is when there are no tabs, because it's impossible to see the title. In such case a tooltip wouldn't work either, since there's not tab to hover. I think many web sites show useful information in the title like gmail, fortunately in the specific case of gmail the most important information is at the beginning of the title. There's a case where the tooltip would be useful, though (does not apply to this specific bug because it happened with the previous version too, so I can file a new bug report if it makes sense). When you have multiple tabs opened and all of them show mostly the same information, like multiple bugzilla tabs. You see a lot of Bug 12345, and you have to manually visit all them to find the you one you are looking for (unless you remember every bug number, I don't :-P). I used to use the tabs menu for that specific use case. The tooltip is still suboptimal because you can only hover the tabs currently shown. Bringing back the tabs menu would solve both issues, I think. It would be a way to see the titles and would allow to quickly jump to any tab even if it's not currently visible. I don't mean to show the tabs menu exactly like we use to do it, because a submenu in the gear menu would still be hard to discover.
Today I have found myself looking for the title several times when working with different bugs if the views are scrolled down. At a first glance all bugzilla pages look quite similar, if you are at the top of the page you see the bug title, but if you are after the last comment, you have to actually read the comment to know which bug you currently are, because the tab only shows the bug number and the first character of the bug description.
Allan: are there many examples where the URL is more relevant than the title? :-) If we provide some small, always visible security assurance, I think that we could get away with only showing the URL entry on new tabs and when the user explicitly chooses to modify it. Jakub draw something similar some time ago: https://raw.github.com/gnome-design-team/gnome-mockups/master/web/url-entry.png One concern with that solution is that the title does not look clickable at all. A possible improvement would be to use a small toggle button to display the security information and enable/disable showing the URL. Something like this: https://raw.github.com/felipeerias/sketches/master/epiphany/url_toggle.png This is still not great because it conflicts with the current behaviour of the "lockpad" icon (show the Security Details dialog), but it can be a step towards a better solution.
I think it can to solve the problem (I have a patch for this, but the patch isn't completed still): https://bitbucket.org/yoseforb/expand-header-bar/raw/c1bb790695a1254105cfb1b4aaa6f8261e63b5c3/epiphany-title-box.png
Created attachment 263118 [details] [review] Add EphyTitleBox and and use it in the toolbar Now we have a title instead of a location bar when the web is loading, and location bar when the page is still not complete to loading, or in the ephy-about:* pages.
https://github.com/meghprkh/wintitle This nice extension shows the window title in a tooltip on hovering the 'Web' application menu.
Created attachment 263309 [details] [review] Add EphyTitleBox and and use it in the toolbar Now we have a title instead of a location bar when the web is loading, and location bar when the page is still not complete to loading, or in the ephy-about:* pages. Note: this patch use GtkStack, so we have not a problem with the height of the headerbar when switch between address-bar to title-bar.
Created attachment 263311 [details] [review] Add EphyTitleBox and and use it in the toolbar Now we have a title instead of a location bar when the web is loading, and location bar when the page is still not complete to loading, or in the ephy-about:* pages. Clean patch, rebase from master.
Created attachment 263457 [details] [review] Add EphyTitleBox and and use it in the toolbar In this patch I disconnect from the "load-changed" signal of the old web_view when I set new web_view in the EphyTitleBox. It fix some issues.
Created attachment 263486 [details] [review] Add EphyTitleBox and and use it in the toolbar Now we have a title instead of a location bar when the web is loading, and location bar when the page is still not complete to loading, or in the ephy-about:* pages.
Review of attachment 263486 [details] [review]: I don't think I understand the commit message.
Created attachment 263516 [details] [review] Sometimes replace the location bar with the title of the web-page * When the web-page in loading, the location bar is shown. * When the web-page finished to loading, the title of the web (and the uri as subtitle) is shown instead of the location bar. * If the entry of the location bar is focused when the wep-page finished to loading, the title will not shown, and the location bar will continue to show. * Click on the title (when it is shown) will switch to the location bar. * Type Esc when the entry of the location bar is focused will switch to the title. To add a comment about how it implement? Something of the signals, maybe on the GtkStack?
Created attachment 263620 [details] [review] Sometimes replace the location bar with the title of the web-page * When the web-page in loading, the location bar is shown. * When the web-page finished to loading, the title of the web (and the uri as subtitle) is shown instead of the location bar. * If the entry of the location bar is focused when the wep-page finished to loading, the title will not shown, and the location bar will continue to show. * Click on the title (when it is shown) will switch to the location bar. * Type Esc when the entry of the location bar is focused will switch to the title. This just removed not necessary call to ephy_title_box_set_mode() in notebook_page_added_cb() in ephy-window.c, so when open a page in a new-tab, without move to it, we not need to show the location bar.
I'm late to the party here but wanted to add a few notes. I agree with the problem. It isn't great that we can't see the page title. Trying out the latest patch I have a couple observations. The first is that there is no indication that the title is clickable and if we move to hover then that may be very flickery and hard to use on touch. Another is that the transition between the two states is abrupt. If we animate it would be better but slower. Another is that it is unclear how to go back to the title state when the url bar is showing. Since the url bar is kind of also a search bar, I wonder if we should use the search bar pattern and show it in a revealer. I think we could have an arrow indicator on the subtitle to suggest clicking. I think we could show the search/url bar by default on the Most Visited page. For regular pages I think we could show the url when the title is clicked and hide it again when the page is loaded, scrolled, or escape is pressed.
Created attachment 263739 [details] quick mockup
Created attachment 263740 [details] [review] Sometimes replace the location bar with the title of the web-page Just make the EphyTitleBox as GtkStack instead GtkBox. Before the notes from McCann.
(In reply to comment #17) > I'm late to the party here but wanted to add a few notes. I agree with the > problem. It isn't great that we can't see the page title. > > Trying out the latest patch I have a couple observations. The first is that > there is no indication that the title is clickable and if we move to hover then > that may be very flickery and hard to use on touch. OK, I'll add an arrow in the end of the subtitle. > > Another is that the transition between the two states is abrupt. If we animate > it would be better but slower. We just needs to play with the numbers, see gtk_stack_set_transition_duration. > > Another is that it is unclear how to go back to the title state when the url > bar is showing. > > Since the url bar is kind of also a search bar, I wonder if we should use the > search bar pattern and show it in a revealer. I think we could have an arrow > indicator on the subtitle to suggest clicking. I think we could show the > search/url bar by default on the Most Visited page. For regular pages I think > we could show the url when the title is clicked and hide it again when the page > is loaded, scrolled, or escape is pressed. I don't like this idea, because the search/url bar will show and hide all the time, a lot of times. Just try my patch and see a lots of times of switching between title to url bar. If we just switching between title and url bar in the headerbar it ok, but if we show and hide a revealer all the times, it not clearly to eye, it add and remove something big all the time, which it realy not convenient and confusing.
*** Bug 674810 has been marked as a duplicate of this bug. ***
*** Bug 708969 has been marked as a duplicate of this bug. ***
*** Bug 708979 has been marked as a duplicate of this bug. ***
Created attachment 264558 [details] [review] Sometimes replace the location bar with the title of the web-page
Created attachment 266117 [details] [review] Sometimes replace the location bar with the title of the web-page
Created attachment 266375 [details] [review] Sometimes replace the location bar with the title of the web-page This fixed some issues.
Created attachment 266378 [details] [review] Sometimes replace the location bar with the title of the web-page Don't show the address bar in application mode.
Created attachment 266379 [details] [review] Sometimes replace the location bar with the title of the web-page Fix the binding of the title and the urii-subtitle in application mode.
I think we need to move the reload button to the side, before the new tab button. I use this patch, and this is what realy bothersome me. More an issue with the patch: In some pages, like this bug, the arrow isn't adjacent to the page url, just with some spaces between (8-16px, more or less).
Created attachment 266481 [details] [review] Sometimes replace the location bar with the title of the web-page This fixes the issue with the arrow.
Created attachment 266496 [details] [review] Sometimes replace the location bar with the title of the web-page
This is shaping up pretty nicely. A few comments after trying it: * I think the transition animation time is too slow. Something around 150 seemed better to me. * The arrow is on the wrong side of the subtitle in LTR locales * The address bar is hidden when the page is completely loaded but this seems slow because the page is visible and usable well before then. I think we may want to hide the addressbar when the page title is available or shortly thereafter. This will also make it seem like a page loads faster. * If I click on the title after the page is loaded the only way I can hide the addressbar again is to click escape or reload the page. I think we should hide the addressbar after ~1sec after a focus-out event. * I really like that the URL in the entry is selected after I click on the title. That is a nice touch and makes it easy to replace it.
A couple more subtle issues I found: * When I'm on the overview screen with the search bar showing and then I switch to another already fully loaded tab, I see the address entry briefly fill in with the new tab's URL during the transition to the title for the new tab. * We seem to have lost the ability to Control+R reload a page * We should probably show the URL entry on the Oops load failed screens?
Created attachment 266501 [details] [review] Sometimes replace the location bar with the title of the web-page
Created attachment 266504 [details] [review] Sometimes replace the location bar with the title of the web-page This fixes (I hope) the issue when you switch from the about:overview to page is fully loaded.
Created attachment 266506 [details] [review] Make Ctrl+R reload Note there is a change between Ctrl+r to Ctrl+R. Also, part from the keyboard shortcuts not work on Hebrew and more some language. this why we need to port to GAction or something same.
About the crash or failed load page: After each one of them (the "load-failed" signal and the "web-process-crashed" signal) the "load-change" is emitted as load completed, so the address bar will hide, and I don't know how to deal with this.
*** Bug 722584 has been marked as a duplicate of this bug. ***
And how to check the title of the tabs in the background?
Created attachment 266737 [details] [review] Sometimes replace the location bar with the title of the web-page This fixes the bind of the title and the url in web-app mode.
I've tried the patch (haven't looked at the code yet), have some comments: - I haven't been able to make it work. When I click on the title, I see the location bar flashing, and then I see the title again and the mouse switches to drag mode. - I think the stop/reload button should be moved to the toolbar, like in app mode. That way you can stop a load after the committed state, it seems impossible at the moment. - The https status icon should *always* be shown and probably the favicon as well. - For pages without a title, we should probably keep the location bar visible, the patch shows a lot of "unsused" space and a very small non editable url. - The fixed hardcoded size is still annoying, but even more with this patch, because for long urls/titles we see the ellipsis, bu there's a lot of space available in the toolbar to show the full title/uri. See also #711407. I have my ephy patched just because of this. I'll comment also about the code when I review the patch
(In reply to comment #41) > I've tried the patch (haven't looked at the code yet), have some comments: > > - I haven't been able to make it work. When I click on the title, I see the > location bar flashing, and then I see the title again and the mouse switches to > drag mode. I can't reproduce this. When I click on the title, I see the loacation bar and I can to edit the url. I can to search and to see the drop down menu with the search result. I can't drag the window from the title or the location bar. I can to drag the window only from the space between the buttons and the title/location bar. > - I think the stop/reload button should be moved to the toolbar, like in app > mode. That way you can stop a load after the committed state, it seems > impossible at the moment. I can to do this, this really simple. I just want to know what the desinger think on this, because for me it look too laden, too, too many buttons in one side. > - The https status icon should *always* be shown and probably the favicon as > well. OK, I'll look how to show the status icon (as button or just as image?). Not sure about the favicon. > - For pages without a title, we should probably keep the location bar visible, > the patch shows a lot of "unsused" space and a very small non editable url. OK. > - The fixed hardcoded size is still annoying, but even more with this patch, > because for long urls/titles we see the ellipsis, bu there's a lot of space > available in the toolbar to show the full title/uri. See also #711407. I have > my ephy patched just because of this. > This also designer issue. I think for now we still want the fixed hardcoded size. > I'll comment also about the code when I review the patch Thanks!
(In reply to comment #42) > (In reply to comment #41) > > I've tried the patch (haven't looked at the code yet), have some comments: > > > > - I haven't been able to make it work. When I click on the title, I see the > > location bar flashing, and then I see the title again and the mouse switches to > > drag mode. > > I can't reproduce this. > When I click on the title, I see the loacation bar and I can to edit the url. > I can to search and to see the drop down menu with the search result. > I can't drag the window from the title or the location bar. I can to drag > the window only from the space between the buttons and the title/location bar. hmm, maybe it has to do with my gtk version or gnome-themes-standard, we should make sure this works with previous gtk versions too or bump gtk requirements if needed. > > - I think the stop/reload button should be moved to the toolbar, like in app > > mode. That way you can stop a load after the committed state, it seems > > impossible at the moment. > > I can to do this, this really simple. I just want to know what the desinger > think > on this, because for me it look too laden, too, too many buttons in one side. It's not only a matter of designing, we really want to be able to stop the load after committed state. > > - The https status icon should *always* be shown and probably the favicon as > > well. > > OK, I'll look how to show the status icon (as button or just as image?). > Not sure about the favicon. We need it to be clickable to show the certificate dialog > > - For pages without a title, we should probably keep the location bar visible, > > the patch shows a lot of "unsused" space and a very small non editable url. > > OK. > > > - The fixed hardcoded size is still annoying, but even more with this patch, > > because for long urls/titles we see the ellipsis, bu there's a lot of space > > available in the toolbar to show the full title/uri. See also #711407. I have > > my ephy patched just because of this. > > > > This also designer issue. I think for now we still want the fixed hardcoded > size. > > > I'll comment also about the code when I review the patch > > Thanks!
Created attachment 267508 [details] [review] location-entry: Make "lock-state" and "show-lock" redable
Created attachment 267509 [details] [review] Sometimes replace the location bar with the title of the web-page This just added the lock icon to the title.
Created attachment 267511 [details] WIP move the reload button to the end of the headerbar If you want to see how this look with reload button in the side.
We needs also a custom style for the title. Currently the patch use the "title" style, which come with some padding which we don't want: https://git.gnome.org/browse/gnome-themes-standard/tree/themes/Adwaita/gtk-3.0/gtk-widgets.css#n2629
Created attachment 267542 [details] [review] location-entry: Make "lock-state" and "show-lock" redable Fix a warning.
Created attachment 267543 [details] [review] Sometimes replace the location bar with the title of the web-page
Created attachment 267547 [details] [review] Sometimes replace the location bar with the title of the web-page
Created attachment 267552 [details] [review] location-entry: Make "lock-state" and "show-lock" redable
Created attachment 267553 [details] [review] Sometimes replace the location bar with the title of the web-page
Created attachment 267629 [details] [review] ephy-window: Make Ctrl+R reload
Created attachment 267630 [details] [review] ephy-location-entry: Make "lock-state" and "show-lock" readable
Created attachment 267631 [details] [review] Replace the location bar with the title of the web page
Created attachment 267686 [details] [review] Replace the location bar with the title of the web page
Review of attachment 267686 [details] [review]: ::: src/ephy-title-box.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ You should use the new style for new files, see the HACKING file for details. @@ +29,3 @@ + +#include <gtk/gtk.h> +#include <webkit2/webkit2.h> This is already included by the header. @@ +66,3 @@ +} EphyTitleBoxPrivate; + +G_DEFINE_TYPE_WITH_PRIVATE (EphyTitleBox, ephy_title_box, GTK_TYPE_STACK); This was added in 2.38 and we depend on 2.35.6, so we should either not use this, or bump the requirements. I persoanlly don't think it's worth it bumping the requs just for this, but I won't be opposed anyway. The trailing ; is not needed. @@ +99,3 @@ + case PROP_WINDOW: + priv->window = EPHY_WINDOW (g_value_get_object (value)); + g_object_notify_by_pspec (object, object_properties[PROP_WINDOW]); This is a construct-only property so there's no point in notifying it has changed, since you can't connect to the notify signal of an object before it's created :-) @@ +122,3 @@ + { + ephy_title_box_set_mode (title_box, EPHY_TITLE_BOX_MODE_TITLE); + /* don't return TRUE since we want to cancel the autocompletion popup too */ You could use GDK_EVENT_STOP/GDK_EVENT_PROPAGATE macros instead of TRUE/FALSE to make it even clearer. @@ +193,3 @@ + LOG ("button-press-event ebox %p event %p title-box %p", widget, event, title_box); + + if (event->button != 3) Use GDK_BUTTON_SECONDARY intead of 3 @@ +283,3 @@ + { + g_value_set_string (to_value, "channel-insecure-symbolic"); + } This could be simpler with something like this: g_value_set_string (to_value, state == EPHY_LOCATION_LOCK_STATE_SECURE ? "channel-secure-symbolic" : "channel-insecure-symbolic"); @@ +319,3 @@ + + if (natural_width) + *natural_width = 530; I still think that we should not use a fixed size in any case, but at least we should not use it when the title shown. When we have an entry it's still weird to see all the available space wated, but when using the title, we won't see th whole string for absolutely no reason. @@ +334,3 @@ + + /** + * EphyTitleBox:window: This is not correctly indented, * should be lined up @@ +344,3 @@ + (G_PARAM_READWRITE | + G_PARAM_CONSTRUCT_ONLY | + G_PARAM_STATIC_STRINGS)); I don't think you need parentheses for this. @@ +347,3 @@ + + /** + * EphyTitleBox:mode: Wrong indentation. @@ +357,3 @@ + EPHY_TITLE_BOX_MODE_LOCATION_ENTRY, + (G_PARAM_READWRITE | + G_PARAM_STATIC_STRINGS)); Same comment here about the () @@ +371,3 @@ + LOG ("EphyTitleBox initialising %p", title_box); + + priv->sig_id = 0; Struct instances are already filled with 0 when allocated by glib, so this is 0 already. @@ +496,3 @@ + priv->title_binding = g_object_bind_property (priv->web_view, "title", + priv->title, "label", + G_BINDING_SYNC_CREATE); Why doing this if we have already the title here and we are connecting to notify::title? It confused me that we were not setting the title in ephy_title_box_title_changed_cb. Ah, because we only connect for mode != app, hmm still confusing, but ok.
Created attachment 268209 [details] [review] Replace the location bar with the title of the web page
(In reply to comment #57) > Review of attachment 267686 [details] [review]: > > ::: src/ephy-title-box.c > @@ +1,1 @@ > +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ > > You should use the new style for new files, see the HACKING file for details. > I hope I use now the correctly style. However, I reallt prefer 8 spaces indent. > > @@ +66,3 @@ > +} EphyTitleBoxPrivate; > + > +G_DEFINE_TYPE_WITH_PRIVATE (EphyTitleBox, ephy_title_box, GTK_TYPE_STACK); > > This was added in 2.38 and we depend on 2.35.6, so we should either not use > this, or bump the requirements. I persoanlly don't think it's worth it bumping > the requs just for this, but I won't be opposed anyway. The trailing ; is not > needed. We depend on gtk+ 3.11.4 which it depend on glib 2.39.0, so I bump the glib version to 2.38.0 to make it clearer. > > @@ +319,3 @@ > + > + if (natural_width) > + *natural_width = 530; > > I still think that we should not use a fixed size in any case, but at least we > should not use it when the title shown. When we have an entry it's still weird > to see all the available space wated, but when using the title, we won't see th > whole string for absolutely no reason. I think there isn't a reason to use a fixed size for the entry and not for the title. Or the same fixed size to both (title and entry), or noone with fixed size. The designer say to set the fixed size, so I not remove this for now. > I fixed all the other comments. Thanks for the review ! (I need also review to the first two patches)
I've finally been able to test the patch after upgrading GTK+. Now it works as expected, but I've noticed another annoyance. I've tired to move the window, but I've ended up with the entry shown instead. The problem is that it's impossible to know where to click to move the window (or maximize it by double clicking, for example), I didn't click over the title text, but it's impossible to know where the title widget finishes in the toolbar. I think the entry should be shown on button release instead of button press, when there hasn't been any drag or double click.
Created attachment 268283 [details] Screenshot This is what I mean about the fixed size for the title case. We have enough space in the toolbar to show the whole title, but we are truncating the title for apparently no reason.
(In reply to comment #59) > > @@ +319,3 @@ > > + > > + if (natural_width) > > + *natural_width = 530; > > > > I still think that we should not use a fixed size in any case, but at least we > > should not use it when the title shown. When we have an entry it's still weird > > to see all the available space wated, but when using the title, we won't see th > > whole string for absolutely no reason. > > I think there isn't a reason to use a fixed size for the entry and not for the > title. > Or the same fixed size to both (title and entry), or noone with fixed size. > The designer say to set the fixed size, so I not remove this for now. Using a fixed size and showing truncated text with plenty of space available is rather ugly and makes the titlebar useless. There is a point in having a fixed size editable widget, as that leaves space to drag the window from the titlebar, but that's achievable, even if the label is occupying all the available space. I'd rather see us using sensible margins for the title and not a fixed size.
When using the entry, we could leave also margins to be able to move the window, and in that case it's pretty obvious where to press to start a window move, so I don't see the point of the fixed size even with the entry if it's only to be able to move the window.
Haven't tried the latest patch yet but wanted to agree with some of the previous comments. * I think I agree that it would be nicer to not ellipsize the title label if possible * Provided we can still drag the window around using the headerbar * As Claudio noted we'll have to be more careful to leave space when the entry box is shown * There is still a bug with the lock icon showing "internal parts". Jakub tried to fix this but not sure what is going on now. * I'm wondering if the lock icon would be better on the subtitle line since it makes more sense to be closely associated with the domain/site/url.
(In reply to comment #64) > * I'm wondering if the lock icon would be better on the subtitle line since it > makes more sense to be closely associated with the domain/site/url. Agree
(In reply to comment #65) > (In reply to comment #64) > > * I'm wondering if the lock icon would be better on the subtitle line since it > > makes more sense to be closely associated with the domain/site/url. > > Agree In where side? In the start we have the arrow. Also, there is a problem when the url is too long.
Created attachment 268624 [details] [review] Replace the location bar with the title of the web page Now it possible to toggle maximized with double click on the title.
Review of attachment 268624 [details] [review]: ::: src/ephy-title-box.c @@ +266,3 @@ + + if (natural_width) + *natural_width = 530; Still the fixed size even for the title @@ +294,3 @@ + mode = ephy_embed_shell_get_mode (ephy_embed_shell_get_default ()); + + if (mode != EPHY_EMBED_SHELL_MODE_APPLICATION && You should check this before adding the timeout. @@ +300,3 @@ + + g_signal_connect (priv->entry, "focus-out-event", + G_CALLBACK (ephy_title_box_entry_focus_out_cb), title_box); This is a problem, you are connecting the signal every time and never disconnecting it. I think you should connect to the signal always once, and check in the callback whether you should switch or not. @@ +320,3 @@ + + if (event->button == GDK_BUTTON_PRIMARY) { + g_timeout_add (double_click_time, For long double click time this could make the switch look slow and the app unresponsive, but I can't think of a better way. Maybe always starting the switch here and going back if the double clikc happens, but that could look even worse. You should check there isn't a switch already scheduled and cancel the switch if the double click happens @@ +347,3 @@ + } + + return FALSE; I think we want to duplicated the GtkWindow code, this should be done already by GtkWindow when we propagate the events. @@ +365,3 @@ + gtk_window_unmaximize (GTK_WINDOW (priv->window)); + else + gtk_window_maximize (GTK_WINDOW (priv->window)); Same here, this should be done by GtkWindow, not by ephy. This doesn't fix the case of moving the window by dragging from the title.
Created attachment 268657 [details] [review] Modified version of Yosef's patch This is a modified version of Yosef's patch that handles both moving the window and maximize/unmaximize it with double click without duplicating the GtkWindow code.
I think we can to take your modified, thanks Carlos :-) Now we have just the fixed size issue. There is something I can't to solve with the entry. I can't expand it. if I removed the override of get_preferred_width in eph-title-box and in ephy-location-bar, the size of the entry changed by the size of the title (GtkStack is homogeneous by default). To set the property "expand" or something not change anything. I have the same problem with nautilus. Any idea?
Created attachment 268732 [details] [review] Replace the location bar with the title of the web page For now I just added more 530px for the fixed size. As discussed on IRC, we needs something like a max-width-chars property in GtkEntry, which we have not yet. So I think we can to saved the fixed size for now, just not so small. Now we not see ellipsize at the end of the uri or the title (just for very long url), and the entry look like expand, but this still a hard code. I hope this good enough.
Created attachment 268736 [details] [review] Replace the location bar with the title of the web page
Created attachment 268784 [details] [review] Replace the location bar with the title of the web page
*** Bug 711407 has been marked as a duplicate of this bug. ***
Comment on attachment 267630 [details] [review] ephy-location-entry: Make "lock-state" and "show-lock" readable I don't think we need this. We should get the security information from the web view, not from the location entry
Review of attachment 268784 [details] [review]: ::: lib/widgets/ephy-location-entry.c @@ +964,3 @@ ephy_location_entry_new (void) { + return GTK_WIDGET (g_object_new (EPHY_TYPE_LOCATION_ENTRY, "max-width-chars", 350, NULL)); Still a harcoded fixed size, but fortunately long enough to not waste toolbar space in most resolutions. ::: src/ephy-title-box.c @@ +222,3 @@ + gtk_box_pack_start (GTK_BOX (box), hbox, FALSE, FALSE, 0); + + priv->lock_image = gtk_image_new_from_icon_name ("channel-secure-symbolic", GTK_ICON_SIZE_MENU); I don't think we should use secure as default value here, we shouldn't show any icon when security level is unknown.
Created attachment 268838 [details] [review] Replace the location bar with the title of the web page
Created attachment 268874 [details] [review] Replace the location bar with the title of the web page Thanks to mccann for the help and the review (in irc).
(In reply to comment #78) > Created an attachment (id=268874) [details] [review] > Replace the location bar with the title of the web page > > Thanks to mccann for the help and the review (in irc). I would appreciate if the reviews are done in bugzilla, so that we all can understand the reasons of the changes
Review of attachment 268874 [details] [review]: ::: src/ephy-title-box.c @@ +215,3 @@ + priv->lock_image = gtk_image_new_from_icon_name ("channel-secure-symbolic", GTK_ICON_SIZE_MENU); + gtk_widget_set_valign (priv->lock_image, GTK_ALIGN_BASELINE); + gtk_widget_show (priv->lock_image); This should be hidden by default. @@ +306,3 @@ + if ((priv->mode != EPHY_TITLE_BOX_MODE_TITLE) + || (event->button != GDK_BUTTON_PRIMARY) + || !priv->button_down) Why did you change this? I find it easier to read as different ifs like it was before, but anyway don't change it again, but remove the unneeded parentheses that clutter the expression even more. @@ +326,3 @@ + + if ((priv->mode != EPHY_TITLE_BOX_MODE_TITLE) + || (event->button != GDK_BUTTON_PRIMARY)) Ditto. @@ +355,3 @@ + ephy_title_box_cancel_switch_to_entry_after_double_click_time (title_box); + + g_clear_object (&priv->web_view); I think it would be better if you call set_web_view passing NULL, so that the signals are disconnected and the bindings are not leaked. @@ +398,3 @@ + EPHY_TITLE_BOX_MODE_LOCATION_ENTRY, + G_PARAM_READWRITE | + G_PARAM_STATIC_STRINGS); You should make this construct, so that set_mode is called with the default value during the object construction. @@ +448,3 @@ + label = g_value_get_string (from_value); + uri = g_strconcat (rtl ? "▾ " : label, rtl ? label : " ▾", NULL); + g_value_set_string (to_value, (const gchar *) uri); You can use g_value_take_string instead, so that you don't need the cast, and we avoid the string duplication. @@ +479,3 @@ + "transition-type", GTK_STACK_TRANSITION_TYPE_CROSSFADE, + "mode", mode == EPHY_EMBED_SHELL_MODE_APPLICATION ? + EPHY_TITLE_BOX_MODE_TITLE : EPHY_TITLE_BOX_MODE_LOCATION_ENTRY, This is not needed, ENTRY is the default value of the property and set_mode already checks whether it's in app mode and switches to title automatically, so you only need to make sure mode is a construct property and remove this. @@ +501,3 @@ + + priv = ephy_title_box_get_instance_private (title_box); + mode = ephy_embed_shell_get_mode (ephy_embed_shell_get_default ()); I think you can move this to where it's first needed since there are several early returns. @@ +527,3 @@ + + g_clear_object (&priv->title_binding); + g_clear_object (&priv->uri_binding); Move this to the block were signals are disconnected, so that when this is called with a NULL view the bindings are also cleared. @@ +553,3 @@ + G_CALLBACK (ephy_title_box_entry_key_press_cb), title_box); + g_signal_connect (priv->web_view, "focus-in-event", + G_CALLBACK (ephy_title_box_view_focus_in_cb), title_box); Why are we now using the focus in of the view instead of the focus out of the entry? @@ +602,3 @@ + if (!uri || g_str_has_prefix (uri, "about:") || + g_str_has_prefix (uri, "ephy-about:")) { + mode = EPHY_TITLE_BOX_MODE_LOCATION_ENTRY; Why do we force the entry for about pages? @@ +671,3 @@ + * their security infrastructure (broken cert, etc). For + * everything else, nothing is shown. + */ This comment is bad indented. Also, we are not using green/red colors at all. ::: src/ephy-toolbar.c @@ -93,2 @@ gtk_widget_set_visible (priv->navigation_box, chrome & EPHY_WINDOW_CHROME_TOOLBAR); - gtk_widget_set_visible (priv->location_box, chrome & EPHY_WINDOW_CHROME_LOCATION); Please, file a bug report to fix this.
(In reply to comment #80) > Review of attachment 268874 [details] [review]: > ::: src/ephy-toolbar.c > @@ -93,2 @@ > gtk_widget_set_visible (priv->navigation_box, chrome & > EPHY_WINDOW_CHROME_TOOLBAR); > - gtk_widget_set_visible (priv->location_box, chrome & > EPHY_WINDOW_CHROME_LOCATION); > > Please, file a bug report to fix this. Sorry, I hadn't see you already filed bug #724170. Thanks!
(In reply to comment #78) > Created an attachment (id=268874) [details] [review] > Replace the location bar with the title of the web page > > Thanks to mccann for the help and the review (in irc). Would you mind explaining here what actually changed and the rationale behind these? We're all in different timezones and with little time so we need to be more careful if we want this to move through quickly.
Created attachment 268893 [details] [review] Replace the location bar with the title of the web page I just merged some mccan's patches: http://paste.fedoraproject.org/76310/21628241/ and also make priv->web_view a ref. (In reply to comment #80) > Review of attachment 268874 [details] [review]: > > @@ +553,3 @@ > + G_CALLBACK (ephy_title_box_entry_key_press_cb), > title_box); > + g_signal_connect (priv->web_view, "focus-in-event", > + G_CALLBACK (ephy_title_box_view_focus_in_cb), title_box); > > Why are we now using the focus in of the view instead of the focus out of the > entry? From mccan's commit message: Switch to title mode when focusing the webview Instead of when leaving the entry. There are too many things that focus out tracking breaks. Context menus for example. > > @@ +602,3 @@ > + if (!uri || g_str_has_prefix (uri, "about:") || > + g_str_has_prefix (uri, "ephy-about:")) { > + mode = EPHY_TITLE_BOX_MODE_LOCATION_ENTRY; > > Why do we force the entry for about pages? I think this not new for this patch. I just think we don't want to show the title in about: pages. In about:overview for example, we needs to saved the entry to allow to search. What do you think? > > @@ +671,3 @@ > + * their security infrastructure (broken cert, etc). For > + * everything else, nothing is shown. > + */ > > This comment is bad indented. Also, we are not using green/red colors at all. I copied this comment from ephy-location-bar.c. I need to remember to look also on what I copy and also if it from other code :-) I removed this comment for now. Btw, ir bad indented because ephy-location-bar.c still use 8 space for tab... I merged the patches (explain to each thing in the commits message).
Created attachment 268897 [details] [review] Replace the location bar with the title of the web page I think we don't want to connect to "key-press-event" and to "focus-in-event" in web app mode, because the entry never not shown and the title always shown.
Review of attachment 267630 [details] [review]: We don't need this anymore.
Created attachment 268915 [details] [review] Replace the location bar with the title of the web page I change the max-width-chars of the entry from 350 to 100, after a discussed in irc.
Created attachment 268994 [details] [review] Replace the location bar with the title of the web page Rebase to git master.
Created attachment 269377 [details] [review] Replace the location bar with the title of the web page I just fixed some g_return_val_if_fail issue.
Created attachment 269410 [details] [review] Replace the location bar with the title of the web page Because the max-width-chars property is too bugy, we decide to drop it and override the get_preferred_width funftion instead.
Created attachment 269426 [details] [review] Replace the location bar with the title of the web page Now in app mode there is a title and not entry.
Review of attachment 269426 [details] [review]: Ok, let's land this and continue fixing the remaining issues in different bugs
Review of attachment 267629 [details] [review]: Moved to bug 724567.
Review of attachment 269426 [details] [review]: Pushed as a9e6505 - Replace the location bar with the title of the web page I did not believe it would happen ;-)
Without wishing to extend the commentary on this very pleasantly fixed bug: What encouraged the designers of GNOME Web 3.11.90 to show both (a) the page title and (b) the address/location/URL in the header bar? <http://ux.stackexchange.com/q/61972/16809> – answers there, please … … or refer me to a good starting point within a mail list (I did already try searching, nothing leapt out at me; from what's above, I assume that much of the rationale was in IRC). Many thanks