GNOME Bugzilla – Bug 791090
page title not showed when only one tab open
Last modified: 2018-08-03 21:22:58 UTC
Created attachment 364763 [details] epiphany 3.20 With epiphany up to 3.22 (included) when using a single-tab the headerbar implementation always showed the page title. Since 3.24 the headerbar shows only the page url, displaying page title in the tab's label, but when only one tab is open, tap-labels are not shown, so the page title is not displayed anywhere. Previous design (up to 3.22) had some minor flaws to be fixed, but the current seems way worse.
Created attachment 364764 [details] epiphany 3.26 "flawed" headerbar
There's no room in the UI for this, sorry. A workaround, if you really need to see the page title for some reason, is to switch to the shell overview, where you can see window titles. The page title will be set as the window title. But I've never had any reason to do this, because page titles are not very important, except when deciding which open tab you want to visit. Now, there has been some desire to show the tab bar at all times, even when there's only one tab. That would fix this, but requires deciding to turn off GTK+'s default tab expand behavior, because a single-tab notebook that expands to fill the entire width of the notebook looks bad. I'm not sure if we're likely to do this (probably not).
Saying that the page title is not important seems like a very personal opinion to me and epiphany is the only browser not having such a feature, so it is even a minority opinion. I don't understand, the previous headerbar implementation totally made sense and there was plenty of room for the title, how this new design improves usability?
The problem was a significant number of users had trouble finding the address bar. See e.g. http://opensource-usability.blogspot.com/2015/03/hands-on-usability-improvements-with.html
Created attachment 364897 [details] Safari solution for the usability problem Safari solved the usability problem by setting a textbox-lie background for the title/url area.
Safari solved the issue by giving the title/address area a textbox-like background when not in edit-mode, an elegant and functional solution. Another solution could have been to add an "edit" button for the url. The adopted solution looks the less usable in my opinion.
I would be fine with a solution like that. Safari looks better than our current design, IMO. In fact, I was hoping to do something like that, before I realized it would be hard... at least too hard for me. So this would need a skilled developer to make it look good, and to not bring back annoying bugs like the lock icon being clickable when it's not supposed to be exposed. I won't have time for this anytime soon, so I won't reopen this bug. But if anyone wants to take on this task, and can make it look good and work seamlessly, that would be cool.
Thanks Micheal, I'll try to find sometime to look into it. I hope you don't mind if I'll bother you again, asking question on epiphany development. Just in case I'll be able to dedicate some time to the task, of course.
No that's fine, happy to help if you have questions! Easiest way in to ask either right here on Bugzilla, or #epiphany on irc.gnome.org. Tricky bits will be ensuring everything works properly when the top bar has two different modes, including when the page does not have a page title (e.g. text files), and ensuring a smooth transition when the title becomes available partway through page loading. Also, just setting on a design for this will be tricky. Christian Hergert has previously suggested (we've discussed this design in the past) that the only sane way to make this work would be to use a GtkStack to switch between location entry mode and "fake" location entry mode, where when displaying the title we show something that is not an Ephy(Gtk)LocationEntry but just looks like it. That's probably the way to go. It also caused tons of bugs for us in the past, before https://git.gnome.org/browse/epiphany/commit/?id=ad0951f6f6f9e457d0f2beb4db772ec0e5b5b4e7, but I think it's the best approach anyway. (Note: you don't want to revert that commit, because we need to keep the old title box around for web app mode.) Since you're planning to actively look into it, I think we can reopen this bug.
Hmm, what does Safari address bar really show in that screenshot? That isn't page title, that would be "Use a Terminal Command to Stop Your Mac From Sleeping"[1]. But that isn't URL either. Just site domain? 1: As seen here: https://www.tekrevue.com/tip/stop-sleep-mac-caffeinate/
Actually, that's a good point... Safari is obviously not displaying the page title there. Come to think of it, long page titles would probably not look very good in the address bar. We could try, though. I'm struggling to remember what our original plan for this had been.
Created attachment 365018 [details] a) solution mockup I probably chose the wrong screenshot from google :) If you look at https://www.apple.com/lae/safari/ you'll see that safari choose to show the titles only in tabs, with tab labels always shown. That's something apple can do, because even so the height of headerbar+labels is smaller than the epiphany's headerbar alone, when using Adwaita. In epiphany's case I'd rather a) hide the current tab label's text or b) hide the title from headerbar when more than one tab is open. Attached a mockup of the a) solution
I'm going to recommend coordinating with the GNOME design team (#gnome-design on irc.gnome.org) before starting any work on this... problem is, there's really a very small amount of space available in the header bar. I'm not sure that expanding the size of the entry is actually going to work out well.
It's just a mockup, I wouldn't change current space usage
Created attachment 368326 [details] Video demonstrating WIP patch As far as i understood the main concern about the old design was that a user wouldn't find the url entry without clicking the ui. What about changing the entry on demand and let the status change on mouse hover? Attached a video showing a small WIP patch. Discussion is open.
(In reply to Jan-Michael Brummer from comment #15) > Attached a video showing a small WIP patch. Discussion is open. I like it! Should we center the title?
No problem, the patch just needs some more love. I'll complete it now.
For the record: Centering the text results in a non-eye-pleasant jump during hovering: It is not recommended.
Created attachment 368525 [details] [review] WIP: Show page title within url entry This is a WIP patch for showing page title within url entry. Please have a look if the current implementation fits your architectural standard. Furthermore i would like to have a small feedback about the visual representation. The is currently a small issue left: OVERVIEW PAGE address is shown.
Created attachment 368534 [details] [review] Show page title within url entry Fix remaining issue. Please have a look if it fits for epiphany.
Review of attachment 368534 [details] [review]: The lock icon feels misplaced now, but this seems like a big step in a nice direction. It's also *way* simpler than I was expecting it to be. Good job. ;) Once you respond to the review comments, I'll push this to master so it can immediately reach Ephy Tech Preview, and also create a gnome-3-28 branch to keep it out of 3.28. ::: src/ephy-location-controller.c @@ +218,3 @@ + EphyLocationController *controller) +{ + gchar *title = controller->title; Use normal char *, see the HACKING file for a rule about this @@ +230,3 @@ + const gchar *address = controller->address; + if (!address || g_str_has_prefix (address, "ephy-about:") || + g_str_has_prefix (address, "about:") || !g_strcmp0 (controller->title, OVERVIEW_PAGE_TITLE) || !g_strcmp0 (controller->title, BLANK_PAGE_TITLE)) { Can you please break this conditional into more lines, to require less horizontal scrolling? @@ +231,3 @@ + if (!address || g_str_has_prefix (address, "ephy-about:") || + g_str_has_prefix (address, "about:") || !g_strcmp0 (controller->title, OVERVIEW_PAGE_TITLE) || !g_strcmp0 (controller->title, BLANK_PAGE_TITLE)) { + address = ""; Hm, it *sort* of works, but press Ctrl+T and notice your blinking cursor in the center of the address bar. What should we do about that? @@ +441,3 @@ notebook = ephy_window_get_notebook (controller->window); widget = GTK_WIDGET (controller->title_widget); + GtkStyleContext *style_context = gtk_widget_get_style_context (widget); Declare it at the top of the function, please. I know that's bad practice nowadays, but let's match existing code style. @@ +442,3 @@ widget = GTK_WIDGET (controller->title_widget); + GtkStyleContext *style_context = gtk_widget_get_style_context (widget); + gtk_style_context_add_class (style_context, "location-controller-entry"); There's probably a better way to do this using CSS selectors, e.g. you could style all GtkEntrys that are child of GtkHeaderBar. Or give the widget a name and style widgets with that name. But this is OK.
Also, a nit: if you could make the lock icon not highlight itself when unrelated parts of the location entry are hovered, that would be ideal. (In reply to Jan-Michael Brummer from comment #18) > For the record: Centering the text results in a non-eye-pleasant jump during > hovering: It is not recommended. Did you change your mind about this?
(In reply to Michael Catanzaro from comment #22) > Also, a nit: if you could make the lock icon not highlight itself when > unrelated parts of the location entry are hovered, that would be ideal. I can remove the opacity experiment, then it will not change at all and will always be at 1.0. > > (In reply to Jan-Michael Brummer from comment #18) > > For the record: Centering the text results in a non-eye-pleasant jump during > > hovering: It is not recommended. > > Did you change your mind about this? No, not really. I just aligned the address and title to center. Still think the left align would be nicer but this wouldn't fit with the notebook label layout... Just an another experiment.
(In reply to Michael Catanzaro from comment #21) > @@ +231,3 @@ > + if (!address || g_str_has_prefix (address, "ephy-about:") || > + g_str_has_prefix (address, "about:") || !g_strcmp0 > (controller->title, OVERVIEW_PAGE_TITLE) || !g_strcmp0 (controller->title, > BLANK_PAGE_TITLE)) { > + address = ""; > > Hm, it *sort* of works, but press Ctrl+T and notice your blinking cursor in > the center of the address bar. What should we do about that? This wouldn't be so obvious if we would left align the title/address entry.
(In reply to Jan-Michael Brummer from comment #23) > I can remove the opacity experiment, then it will not change at all and will > always be at 1.0. I think I like the 0.8 opacity though, it looks nice. I bet it would be possible to exempt the lock icon from it with a bit more CSS. > > (In reply to Jan-Michael Brummer from comment #18) > No, not really. I just aligned the address and title to center. Still think > the left align would be nicer but this wouldn't fit with the notebook label > layout... Just an another experiment. Let's go with whatever you think looks better for now. We've got until September to decide whether to center it.
(In reply to Jan-Michael Brummer from comment #24) > This wouldn't be so obvious if we would left align the title/address entry. We'll probably want to make sure it always gets left-aligned at least when the user is editing it.
In this case the text jumps and this is my concern: it looks bad.
(In reply to Michael Catanzaro from comment #25) > Let's go with whatever you think looks better for now. We've got until > September to decide whether to center it. Let's leave it left-aligned for now, then.
Maybe animate url on click so that the jump is at least smooth? Also, wouldn't hiding url until hover make it much harder to detect phishing pages?
Created attachment 368583 [details] [review] Show page title within url entry - V2 Updated patch based on your review. Currently no change to lock symbol highlight. This ui change can be done later?
(In reply to Alexander Mikhaylenko from comment #29) > Also, wouldn't hiding url until hover make it much harder to detect phishing > pages? Ummmmm yes, that should probably be a blocker. I should have spotted that problem immediately. Good catch. We don't need to show the entire URL, but the security origin <protocol, host, [port if nonstandard]> should always be visible. I don't think that's really possible with Jan-Michael's current design. That shouldn't stop us from experimenting, of course.
(In reply to Michael Catanzaro from comment #31) > We don't need to show the entire URL, but the security origin <protocol, > host, [port if nonstandard]> should always be visible. The protocol can probably be omitted if needed; we e.g. have the security indicator to warn about missing HTTPS. But the host is essential.
Well in case this is valid blocker for you we have only two options left: - Leave as it is now (without my patch) and document the decision - Adapt the patch to create a hybrid of the old epiphany design (page title/url) and add the mouse hover effect. This might be a little bit more tricky for touch input...
I think it's a blocker, unfortunately. Displaying the security origin is essential to prevent phishing.
Another experiment we could try is to keep this approach, but instead of switching the address bar to show the title, instead switch it to showing the security origin. We could strip out the protocol too, and show just host[:port] (which will usually be just the host, because websites don't often use nonstandard ports).
But this will not solve this issue, right?
That's right. I don't really strongly care about showing the page title, though. I just want it to look nicer than it does now. And I think what your patch looks nice. We might need designer help to reconcile your patch with the requirement to display hostname[:port] at all times.
I tried several different version of a combined title/url headerbar yesterday but all of them have some drawback (either looking weird, bad for keyboard input, bad for touch input, ...). To sum it up: It currently looks like the best solution would be always enabling notebook bar (even for one tab only): This would solve all known issues. Afterwards we can update the url entry design a little bit (e.g. mouse hover effect) so that it looks more solid. Something like i did in my previous patch. Is it good to go?
(In reply to Jan-Michael Brummer from comment #38) > To sum it up: It currently looks like the best solution would be always > enabling notebook bar (even for one tab only) If we're going to do this, then I think we need to disable tab expand, so tabs are always fixed-width. I know it's possible, but I'm not quite sure how to do that.
I'm not sure if i can follow. How should it look like?
Instead of a single tab stretching to fill the entire tab bar, it should consume only as much horizontal space as it would if a bunch of other tabs were open in the tab bar. (Probably. ;)
Ok, that's no problem. We just need to remove the hexpand of the label and it will collapse to "...". Now we would have to define a minimal/maximal width of a tab otherwise you will have tabs with different widths. How to choose the minimal width? Min/Max 20 chars per entry?
Created attachment 368730 [details] Notebook with a label limit of 20 chars Here is a screenshot showing the change
Meh. Wild idea: can we go back to the pretty patch you proposed the other day, with the nice opacity. The main problem with that is phishing. So instead of switching from the full URL to just the page title, we could instead display: hostname[:port] – page title e.g. www.google.com – Google (note that's an en dash rather than a hyphen) Or some variation on that. Would that look OK?
Created attachment 368844 [details] Combined base url + title location bar Attaching a screenshot showing your proposal. Feedback?
Bookmark button is almost invisible on the screenshot with Night Light enabled. Otherwise, looks nice to me! Some wild ideas: maybe it's possible to separate hostname from title with a full-width vertical line? Or even have the entire lock+hostname act as a button for security popover?
Yes, I think it mostly looks nice. (In reply to Alexander Mikhaylenko from comment #46) > Some wild ideas: maybe it's possible to separate hostname from title with a > full-width vertical line? I'm thinking that more separation between the hostname and the title would be desirable. I don't know how hard it would be to achieve this, though. Experimenting with different separator characters, or with adding extra whitespace, or, if possible, even: (In reply to Alexander Mikhaylenko from comment #46) > Or even have the entire lock+hostname act as a button for security popover? Yeah, I'm not sure how feasible that would be, but I think that might be nice.
Created attachment 368848 [details] Location options Here are some options with different separators. I like the version with a colon... IMHO: The layout should be very simple without adding to much noise using standard widgets. Of course we could switch to a GdTaggesEntry but this will look out of place...
Comment on attachment 368848 [details] Location options The vertical bar looks like the worst option. I guess the colon does seem best. Let's try that for now, and we can see if we still like it after a couple months. We have six months to change our minds. What does GdTaggedEntry look like?
(In reply to Michael Catanzaro from comment #49) > What does GdTaggedEntry look like? (No point in answering this if you say it's going to look bad.)
Created attachment 368850 [details] Mockup I meant sth like this :)
Take a look at nautilus/totem search entry. It looks good for search entries, but it might be out of place for a web browser...
(In reply to Alexander Mikhaylenko from comment #51) > Created attachment 368850 [details] > Mockup > > I meant sth like this :) Yes, but keep in mind that it a GtkEntry at the moment. It's not possible currently. This would be a new widget.
Hmm, that could be a linked button? But without that a dash is probably fine.
A linked button for the url? Which problem will it solve? This would mean we have to split the entry into two widgets.
Created attachment 368858 [details] Option 4: Title Box + Hover Last possible solution for today: Title box + hover effect. This would combine title box widget + title widget and functionality for webapps is still there (disable hover effect).
A linked button for the lock+hostname. But yes, I forgot about url editing when I said that, sorry :)
Let's not go back to the title box. It caused many minor bugs. Getting the hover working right would not be easy, either. (In reply to Jan-Michael Brummer from comment #52) > Take a look at nautilus/totem search entry. It looks good for search > entries, but it might be out of place for a web browser... I dunno. That might work, too. Not sure. (In reply to Alexander Mikhaylenko from comment #51) > Created attachment 368850 [details] > Mockup > > I meant sth like this :) That *looks* really good. But: (In reply to Jan-Michael Brummer from comment #55) > A linked button for the url? Which problem will it solve? This would mean we > have to split the entry into two widgets. It's a reasonable suggestion. I think the only problem is that it would be significantly harder to implement.
(In reply to Jan-Michael Brummer from comment #43) > Created attachment 368730 [details] > Notebook with a label limit of 20 chars > > Here is a screenshot showing the change I think this would look a lot better if the new tab button and the menu next to it floated just to the right of the last open tab, like in Firefox and Chrome, so that all the empty space on the tab bar is off to the right. Is that possible? Then we could actually do both your location entry style change *and* the tabs style change.
(In reply to Michael Catanzaro from comment #59) > (In reply to Jan-Michael Brummer from comment #43) > > Created attachment 368730 [details] > > Notebook with a label limit of 20 chars > > > > Here is a screenshot showing the change > > I think this would look a lot better if the new tab button and the menu next > to it floated just to the right of the last open tab, like in Firefox and > Chrome, so that all the empty space on the tab bar is off to the right. Is > that possible? Then we could actually do both your location entry style > change *and* the tabs style change. No it is not possible to clear the empty space with GtkNotebook as it only knows action widgets on START or END of tab bar. Should we summarize all solutions with pro/cons and make a decision based on that or is it now fixed with always visible tab- bar (my assumption because of your reply).
You can move the new tab button to the headerbar and drop the menu as gtknotebook adds the arrow buttons on demand. This would lead to a clean interface.
(In reply to Jan-Michael Brummer from comment #60) > Should we summarize all solutions with pro/cons and make a decision based on > that or is it now fixed with always visible tab- bar (my assumption because > of your reply). It's really two completely separate issues: 1) What to do to make the location entry look pretty 2) What to do to avoid the moving tab button They both got mixed into this bug, but we should decide how to handle them separately. 1) For the first issue: see my most recent opinion in comment #58, and Alexander's suggestion. Something like that might look quite nice, but it requires either splitting the GtkEntry or trying out GdkTaggedEntry. If those would be hard, then I suggested in comment #49 that I liked the hostname[:port]: title option that you already have working. We could always push that now, and experiment with split location entry or GdkTaggedEntry later. I do think that splitting the entry like Alexander suggested looks like the best option; we just need to figure out how to do it. 2) For the second issue: (In reply to Jan-Michael Brummer from comment #61) > You can move the new tab button to the headerbar and drop the menu as > gtknotebook adds the arrow buttons on demand. This would lead to a clean > interface. Yeah, good idea. That would be good to try if it's possible for us to detect whether the GtkNotebook has added arrows or not.
(In reply to Michael Catanzaro from comment #62) > It's really two completely separate issues: > > 1) What to do to make the location entry look pretty > 2) What to do to avoid the moving tab button Fixing either one would placate people who want to see the page title when there is only one tab. (I still fail to understand why the title is useful for anything other than choosing which tab to view, but whatever.)
(In reply to Michael Catanzaro from comment #62) > (In reply to Jan-Michael Brummer from comment #60) > > Should we summarize all solutions with pro/cons and make a decision based on > > that or is it now fixed with always visible tab- bar (my assumption because > > of your reply). > > It's really two completely separate issues: > > 1) What to do to make the location entry look pretty > 2) What to do to avoid the moving tab button > > They both got mixed into this bug, but we should decide how to handle them > separately. > > 1) For the first issue: see my most recent opinion in comment #58, and > Alexander's suggestion. Something like that might look quite nice, but it > requires either splitting the GtkEntry or trying out GdkTaggedEntry. If > those would be hard, then I suggested in comment #49 that I liked the > hostname[:port]: title option that you already have working. We could always > push that now, and experiment with split location entry or GdkTaggedEntry > later. I do think that splitting the entry like Alexander suggested looks > like the best option; we just need to figure out how to do it. It's a bigger change to the app i would like to avoid (I already did some experiments with the linked button (see screenshot)). For the moment i will prepare a patch using the minimal change: "Let's see how it flies". > > 2) For the second issue: > > (In reply to Jan-Michael Brummer from comment #61) > > You can move the new tab button to the headerbar and drop the menu as > > gtknotebook adds the arrow buttons on demand. This would lead to a clean > > interface. > > Yeah, good idea. That would be good to try if it's possible for us to detect > whether the GtkNotebook has added arrows or not. Yes, it is possible to detect the visibility. Why is it necessary? GtkNotebook will add the arrows and a menu (if selected) on demand.
Created attachment 368879 [details] Linked button example
Created attachment 368886 [details] [review] Show page title within url entry - V3 Updated patch which uses: base_url: "TITLE" layout.
Yellow bookmark star is still pretty much invisible with the transparent url bar. Also, it's probably desirable to be able to change the quotes in translations, because not all language use "quotes". (e.g. russian uses «quotes»). And maybe it'd look nicer if the urlbar was only transparent when not editing/hovering?
Also, it looks weird when the hostname: title just disappears on hover on newtab page. And is it ok to just manually edit epiphany.css instead of parsing sass?
(In reply to Alexander Mikhaylenko from comment #67) > Yellow bookmark star is still pretty much invisible with the transparent url > bar. I'm aware of that, but this also applies for the standard url input for me. There is a ticket for this issue (replacing it with a more visible blue color), but Michael want's the yellow star. > Also, it's probably desirable to be able to change the quotes in > translations, because not all language use "quotes". (e.g. russian uses > «quotes»). No problem. > And maybe it'd look nicer if the urlbar was only transparent when not > editing/hovering? I don't know, i will stay with the current design...
Created attachment 368889 [details] [review] Show page title within url entry - V4 Update: - Remove title quotes - Add place holder text
(In reply to Jan-Michael Brummer from comment #64) > It's a bigger change to the app i would like to avoid (I already did some > experiments with the linked button (see screenshot)). For the moment i will > prepare a patch using the minimal change: "Let's see how it flies". Your screenshot shows the general idea, but it's going to need some CSS to make it blend in nicer with the entry. Maybe try the "flat" style class? Might even be possible to use CSS to make it look just like the entry background? > Yes, it is possible to detect the visibility. Why is it necessary? > GtkNotebook will add the arrows and a menu (if selected) on demand. Ah, I see now. I did not know that GtkNotebook could do its own menu. If it looks good, then great. I wrote the current menu manually. Taking a quick look at the GtkNotebook API, it might be better than what we have now, because we can add arbitrary widgets there. That would allow adding favicons to the menu. (In reply to Jan-Michael Brummer from comment #69) > I'm aware of that, but this also applies for the standard url input for me. > There is a ticket for this issue (replacing it with a more visible blue > color), but Michael want's the yellow star. We should probably use a darker yellow, though. Or use some sort of outline.
(In reply to Jan-Michael Brummer from comment #70) > Created attachment 368889 [details] [review] [review] > Show page title within url entry - V4 > > Update: > - Remove title quotes > - Add place holder text Try making it left-aligned. You were right originally: the title jumping around looks too bad otherwise (as does the empty space between the security indicator and the hostname).
Review of attachment 368889 [details] [review]: I (almost) like the way it looks, so time for code review.... ::: lib/widgets/ephy-location-entry.c @@ +752,3 @@ + gtk_entry_set_placeholder_text (GTK_ENTRY (entry), + _ ("Search or enter address")); No space after the _. This is an exception to our usual rule. ::: src/ephy-location-controller.c @@ +122,3 @@ if (uris != NULL && uris[0] != NULL && *uris[0] != '\0') { gtk_entry_set_text (entry, (char *)uris[0]); + ephy_location_controller_set_title (controller, ""); Might make sense to add ephy_location_controller_unset_title(), for readability, since you use this in several places. @@ +219,3 @@ +static char * +get_base_url (char *url) For strings, we use const to indicate ownership transfer. This parameter should be const char *url, since the caller does not transfer ownership and get_base_url() is not supposed to free it. Anyway, I would rewrite it using SoupURI. Untested: static char * get_base_uri (const char *url) { SoupURI *soup_uri; char *result; if (url == NULL) return NULL; soup_uri = soup_uri_new (url); if (uri->scheme == SOUP_URI_SCHEME_HTTP && soup_uri->port != 80 || uri->scheme == SOUP_URI_SCHEME_HTTPS && soup_uri->port != 443) result = g_strdup_printf ("%s:%u", soup_uri->host, soup_uri->port); else result = g_strdup (soup_uri->host); soup_uri_free (soup_uri); return result; } @@ +224,3 @@ + char *ret; + + if (url == NULL) { Nit: our code style is to put braces only around multi-line conditionals (including, for clarity, multi-line conditionals that contain only one statement and don't actually need braces). @@ +249,3 @@ + base_url != NULL ? ": " : "", + controller->title); + gtk_entry_set_text (GTK_ENTRY (entry), text); It will break ephy_title_widget_get_address(), which you would expect to return the address, not the title. EphyLocationEntry always returns its text. @@ +262,3 @@ + const gchar *address = controller->address; + if (!address || + g_str_has_prefix (address, "ephy-about:") || Use ephy_embed_utils_is_no_show_address(). @@ +283,3 @@ g_signal_handlers_block_by_func (widget, G_CALLBACK (user_changed_cb), controller); ephy_title_widget_set_address (controller->title_widget, controller->address); + entry_set_title (widget, controller); Two problems here: (1) it looks bad to call two different functions in a row to set the title, and (2) it assumes the EphyTitleWidget is always an EphyLocationEntry, which is always true in browser mode, but in web app mode it will always be an EphyTitleBox. You need to use EPHY_IS_LOCATION_ENTRY() to check that before assuming. A good reminder that we need to test this change in web app mode as well. :) I guess we don't want any changes to the user experience in web app mode. So this needs cleaned up a bit. It's not immediately clear to me how. I'm not sure if it actually makes sense to keep ephy_title_widget_get/set_address() anymore, since EphyLocationEntry and EphyTitleBox no longer function similarly enough. Perhaps we should get rid of those. Alternatively, maybe EphyLocationEntry just needs to keep track of its address separately from its text, and implement the enter/leave operations itself, so you wouldn't have to do that up here in EphyLocationController. Not sure what would be best. @@ +349,3 @@ +enter_notify_event_cb (GtkWidget *entry, + GdkEvent *event, + EphyLocationController *controller) The proper alignment for the function parameters would be: static gboolean enter_notify_event_cb (GtkWidget *entry, GdkEvent *event, EphyLocationController *controller); @@ +360,3 @@ +leave_notify_event_cb (GtkWidget *entry, + GdkEvent *event, + EphyLocationController *controller) Here too. @@ +480,3 @@ widget = GTK_WIDGET (controller->title_widget); + style_context = gtk_widget_get_style_context (widget); + gtk_style_context_add_class (style_context, "location-controller-entry"); Try using gtk_widget_set_name() and using the name in the SCSS/CSS instead. Also, move this down below the EPHY_IS_LOCATION_ENTRY() check, so that you don't add the wrong name or style class to EphyTitleBox. @@ +760,3 @@ + g_free (controller->title); + controller->title = g_strdup (title); + entry = GTK_WIDGET (controller->title_widget); Again, you assume the EphyTitleWidget is an EphyLocationEntry. You'll need to return early if it's not.
(In reply to Michael Catanzaro from comment #73) > if (uri->scheme == SOUP_URI_SCHEME_HTTP && soup_uri->port != 80 || > uri->scheme == SOUP_URI_SCHEME_HTTPS && soup_uri->port != 443) (Note: scheme is an interned string, which is why it's OK to compare the strings by pointer address.)
(In reply to Michael Catanzaro from comment #71) > (In reply to Jan-Michael Brummer from comment #64) > > It's a bigger change to the app i would like to avoid (I already did some > > experiments with the linked button (see screenshot)). For the moment i will > > prepare a patch using the minimal change: "Let's see how it flies". > > Your screenshot shows the general idea, but it's going to need some CSS to > make it blend in nicer with the entry. Maybe try the "flat" style class? > > Might even be possible to use CSS to make it look just like the entry > background? I already tried flat style, but it looks bad as it does not have the combined/linked feeling. Also tried another entry instead of a button it is even worse... > > Yes, it is possible to detect the visibility. Why is it necessary? > > GtkNotebook will add the arrows and a menu (if selected) on demand. > > Ah, I see now. I did not know that GtkNotebook could do its own menu. If it > looks good, then great. I wrote the current menu manually. > > Taking a quick look at the GtkNotebook API, it might be better than what we > have now, because we can add arbitrary widgets there. That would allow > adding favicons to the menu. Ok, i will keep track of it in another ticket. > > (In reply to Jan-Michael Brummer from comment #69) > > I'm aware of that, but this also applies for the standard url input for me. > > There is a ticket for this issue (replacing it with a more visible blue > > color), but Michael want's the yellow star. > > We should probably use a darker yellow, though. Or use some sort of outline. Or just simply use color: orange as a solution as you don't like the blue color here. It's a good trade-off between yellow and blue.
After solving most of your review finding it is clearly visible that we need some patch refactoring here: Seems the best solution would be to move the code to EphyLocationEntry.
Yes, EphyLocationEntry should be best. (In reply to Jan-Michael Brummer from comment #75) > Or just simply use color: orange as a solution as you don't like the blue > color here. It's a good trade-off between yellow and blue. Yeah, that might look good.
Created attachment 368900 [details] [review] Show page title within url entry - V5 Updated patch: - Moved logic to ephy-location-entry - Update ephy-location-controller and ephy-title-widget to pass title information to ephy-location-entry
Review of attachment 368900 [details] [review]: Yes, this is good. Thanks! I have more review comments, but you're super close now. ::: lib/widgets/ephy-location-entry.c @@ +35,3 @@ #include "ephy-uri-helpers.h" #include "gd-two-lines-renderer.h" +#include "ephy-embed-utils.h" Alphabetize it @@ +145,3 @@ + +static char * +get_base_url (const char *url) It's not the right name, since what it returns is no longer a URL. One alternative name would be format_title(). You could pass it entry->address and entry->title, and the result would include the colon at the end... @@ +170,3 @@ +static void +entry_update_text (EphyLocationEntry *entry, + char type) Use int instead of char. That micro-optimization is not worth the extra confusion. @@ +176,3 @@ + gchar *text = g_strdup_printf ("%s%s%s", + base_url != NULL ? base_url : "", + base_url != NULL ? ": " : "", ...so you would not need the double ternary here. Alternatively, if you prefer, you could move this into a proper conditional. Either way would look better than having the same test two lines in a row. @@ +185,3 @@ + + /* Additional check for !address as ephy_embed_utils_is_no_show_address () + * returns FALSE for it */ I don't think you need this comment, it's clear that you're checking !address because you need to. ::: lib/widgets/ephy-title-widget.c @@ +121,3 @@ + + g_assert (iface->set_title); + iface->set_title (widget, title); Doesn't this crash if the title widget is an EphyTitleBox? I think it will, because iface->set_title will be NULL. Please test in web app mode. There are two obvious possible solutions: * Implement iface->set_title for EphyTitleBox; or * Just check if it's NULL before calling it But, thinking about this more, I would rather not add ephy_title_widget_set_title() after all. That's because, when in web app mode, the title of the EphyTitleBox is currently always the name of the web app, and not set based on the page title. So we would want this function to do nothing for EphyTitleBox. That's OK, but since this function is only needed in one place, I think it would be nicer expose it as ephy_location_entry_set_title() instead, and make EphyWindow cast to EphyLocationEntry. ::: src/ephy-window.c @@ +1152,3 @@ gtk_window_set_title (GTK_WINDOW (window), + title); + ephy_location_controller_set_title (window->location_controller, title); We shouldn't need to go through EphyLocationController for this. The location controller controls the location, right? It doesn't need to have anything to do with the title, especially now that you've moved the functionality down into EphyLocationEntry. Fortunately, this is easy to avoid: title_widget = ephy_header_bar_get_title_widget (EPHY_HEADER_BAR (window->header_bar)); ephy_title_widget_set_title (controller->title_widget, title); Then you do not need to add ephy_location_controller_set_title(). If you take my advice to replace ephy_title_widget_set_title() with ephy_location_entry_set_title(), that would look like: title_widget = ephy_header_bar_get_title_widget (EPHY_HEADER_BAR (window->header_bar)); if (EPHY_IS_LOCATION_ENTRY (title_widget)) ephy_location_entry_set_title (EPHY_LOCATION_ENTRY (title_widget));
(In reply to Michael Catanzaro from comment #79) > title_widget = ephy_header_bar_get_title_widget (EPHY_HEADER_BAR > (window->header_bar)); > if (EPHY_IS_LOCATION_ENTRY (title_widget)) > ephy_location_entry_set_title (EPHY_LOCATION_ENTRY (title_widget)); And yes, taking action based on the dynamic type of a polymorphic object is considered a polymorphism anti-pattern, but it seems a bit cleaner than having a title widget with a settable title where setting the title does nothing.
Created attachment 368978 [details] [review] Show page title within url entry - V6 Yes, it is easier to implement but not the best architecture design. Nevertheless i followed your design decision and have updated the patch.
Review of attachment 368978 [details] [review]: The problem I see with the previous design: ephy_title_widget_set_title() called on an EphyTitleBox would do absolutely nothing. That's... non-ideal. I like this way better.
OK, pushed. The only problem I see is that it looks weird when a long title runs off the edge of the title box, but it seems hard to implement ellipsization since we don't have any way to know the right place to put the ellipses. Oh well.
(In reply to Jan-Michael Brummer from comment #75) > > > Yes, it is possible to detect the visibility. Why is it necessary? > > > GtkNotebook will add the arrows and a menu (if selected) on demand. > > > > Ah, I see now. I did not know that GtkNotebook could do its own menu. If it > > looks good, then great. I wrote the current menu manually. > > > > Taking a quick look at the GtkNotebook API, it might be better than what we > > have now, because we can add arbitrary widgets there. That would allow > > adding favicons to the menu. > > Ok, i will keep track of it in another ticket. Still interested in doing this, btw
Regarding your question from IRC: "what's the reason to use a button combined with an entry instead of using two entries?" An insensitive entry would work fine, I suppose. But a button seems nicer, because it would be combined with the security indicator to form one large button, right?
I've reverted this because it's annoying Andres, and me too. :) There are a few kinks to work out. See bug #794090.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/epiphany/issues/445.