GNOME Bugzilla – Bug 685747
Show URI in the statusbar on snapshot hover
Last modified: 2014-03-06 09:05:12 UTC
Like we do in links. I think it makes sense, and I kind of miss it.
Giving this a shot.
Created attachment 231078 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. = I need a hand with getting the overlay widget to expand vertically and horizontally. If you apply the patch and hover you'll see that the overlay widget is not expanding, therefor the bar looks ugly :(
Created attachment 231079 [details] EphyOverview not expanding, why oh why?
Created attachment 231083 [details] [review] e-embed: use an overlay for the overview - Needed to get the following simple(r) solution: instead of adding another overlay and floating bar we reuse the one in EphyEmbed.
Created attachment 231084 [details] [review] e-embed: only add one statusbar timeout at a time Otherwise the statusbar can disappear even if it has a valid message being displayed. -- Bug in e-embed. The statusbar code was able to install more than one timeout at a time, turning useless the safeguard of removing it when adding a valuable message that we don't want to suddenly disappear.
Created attachment 231085 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. -- This should be ok. Works as expected: you get the statusbar on the bottom of the window everytime you hover a thumbnail. The same NautilusFloatingBar from EphyEmbed is reused via ephy_web_view_set_link_message(). I need help fixing the theming! Because of the GTK_STYLE_CLASS_OSD added to the GtkOverlay in EphyEmbed, the background of EphyOverview is a shade of grey. I am pretty sure our theming experts can fix this in a second :-). Help me!
Created attachment 231088 [details] wrong bg color when GtkOverlay is set to OSD This is the theming issue I am seeing with a fresh jhbuild.
Created attachment 231097 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. -- Cosimo pushed a theme fix that has no issue with this updated patch.
Created attachment 231099 [details] [review] e-embed: hide progress for about and ephy-about -- Prevent the progress bar from popping in the overview and other places.
Created attachment 231137 [details] [review] e-embed: use an overlay for the overview -- Update to correctly bind the properties. Fixes visibility in webapp mode.
Created attachment 231165 [details] difference in borders
Review of attachment 231097 [details] [review]: ::: embed/ephy-overview.c @@ +103,3 @@ + -1); + + if (url && (g_strcmp0 (current, url) != 0 || current == NULL)) Something here feels redundant. If url is not NULL and current and url are the same, then current can't be NULL, right?
Review of attachment 231084 [details] [review]: Seems to make sense.
(In reply to comment #12) > Review of attachment 231097 [details] [review]: > > ::: embed/ephy-overview.c > @@ +103,3 @@ > + -1); > + > + if (url && (g_strcmp0 (current, url) != 0 || current == NULL)) > > Something here feels redundant. If url is not NULL and current and url are the > same, then current can't be NULL, right? Yes, you are right. Updating.
Created attachment 231259 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. -- Updated for the redundancy issue in if().
Created attachment 231260 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. -- Due to recent changes an include for ephy-embed was missing. Claudio's comment is addressed.
Review of attachment 231099 [details] [review]: ::: embed/ephy-embed.c @@ +817,1 @@ return; What has this to do with this bug?
Review of attachment 231137 [details] [review]: Looks good.
(In reply to comment #17) > Review of attachment 231099 [details] [review]: > > ::: embed/ephy-embed.c > @@ +817,1 @@ > return; > > What has this to do with this bug? Because the EphyWebView is now not hidden when the overview widget is visible, you can see the load progress of the overview which looks weird because it's a flash like it was for blank pages.
Review of attachment 231260 [details] [review]: Looks good. ::: embed/ephy-embed.c @@ +928,1 @@ gtk_overlay_add_overlay (GTK_OVERLAY (overlay), priv->progress); Explain to me why is necessary. Better, explain it in the commit message :)
Created attachment 231291 [details] [review] e-embed: use an overlay for the overview Moves the GTK_STYLE_CLASS_OSD from the overlay to the progressbar, to avoid wrongly theming the EphyOverview. The class is necessary for theming the progressbar correctly. This permits reusing elements of EphyEmbed more easily.
Created attachment 231292 [details] [review] e-embed: hide progress for about and ephy-about The progressbar should only react to pages, not special locations.
Created attachment 231293 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites.
Review of attachment 231292 [details] [review]: ::: embed/ephy-embed.c @@ +822,1 @@ return; okay
Review of attachment 231293 [details] [review]: ::: embed/ephy-overview.c @@ +89,3 @@ + + /* embed → paned → overlay in embed → overview */ + embed = gtk_widget_get_parent (gtk_widget_get_parent (gtk_widget_get_parent (GTK_WIDGET (overview)))); I think we should find a way to do this without such a hack. @@ +105,3 @@ + + if (url && g_strcmp0 (current, url) != 0) + So, I think that this set_link_message() API shouldn't be in ephy_web_view() but wherever the actual tooltip is handled (is it EphyEmbed, no?). Then, both EphyWebView and EphyOverview should have either a signal or a property to advertise it ("someone's hovering on this link and we think someone might be interested in displaying this -- somehow, but it's not my business"). Then EphyEmbed would of course be listening to that signal / property changes and set the link-message.
Review of attachment 231291 [details] [review]: Okay
(In reply to comment #27) > Review of attachment 231293 [details] [review]: > > ::: embed/ephy-overview.c > @@ +89,3 @@ > + > + /* embed → paned → overlay in embed → overview */ > + embed = gtk_widget_get_parent (gtk_widget_get_parent (gtk_widget_get_parent > (GTK_WIDGET (overview)))); > > I think we should find a way to do this without such a hack. > > @@ +105,3 @@ > + > + if (url && g_strcmp0 (current, url) != 0) > + > > So, I think that this set_link_message() API shouldn't be in ephy_web_view() > but wherever the actual tooltip is handled (is it EphyEmbed, no?). Then, both > EphyWebView and EphyOverview should have either a signal or a property to > advertise it ("someone's hovering on this link and we think someone might be > interested in displaying this -- somehow, but it's not my business"). Then > EphyEmbed would of course be listening to that signal / property changes and > set the link-message. That's the current API actually. There's EphyWebView::status-message (triggered by setting link-message). EphyEmbed listens to that and reacts. EphyOverview could have a ::status-message too and we can make EphyEmbed listen to that.
That sounds much better.
Created attachment 231387 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. Adds ::status-message to EphyOverview, mimicking EphyWebView's same property. EphyEmbed listens to both properties and updates its statusbar when any of those changes.
Review of attachment 231387 [details] [review]: This is looking much better! Just a couple of comments: ::: embed/ephy-embed.c @@ +990,3 @@ + priv->overview_status_handler_id = g_signal_connect (priv->overview, "notify::status-message", + G_CALLBACK (status_message_notify_cb), + embed); I'm wondering whether you need to make sure to clean this up when the visibility of the overview is toggled? ::: embed/ephy-overview.c @@ +124,3 @@ + if (overview->priv->status_message) + g_free (overview->priv->status_message); + Here you check for NULL before calling g_free()... @@ +198,3 @@ + switch (prop_id) { + case PROP_STATUS_MESSAGE: + default: Is this necessary if the property is read-only? @@ +211,3 @@ + + g_free (priv->status_message); + guint prop_id, ..but you don't check here. Do you need to at all? :)
Created attachment 231393 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. Adds ::status-message to EphyOverview, mimicking EphyWebView's same property. EphyEmbed listens to both properties and updates its statusbar when any of those changes. -- Updated for comments. I don't think we should worry about the motion callback on the hidden widget. If we discover that the callback is called even when the widget is hidden, then we can report a GTK+ bug. Consider bug #690110, which removes ::link-message and makes EphyWebView correctly report link messages. Without that patch, we would only get status related messages (Loading gnome.org…) Thanks for the reviews :).
Created attachment 231401 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. Adds ::status-message to EphyOverview, mimicking EphyWebView's same property. EphyEmbed listens to both properties and updates its statusbar when any of those changes. --- Addresses your IRC comment. When going out of the overview_mode, we unset the statusbar message. What do you think?
Review of attachment 231401 [details] [review]: Looks good, besides the comment. I think we can push this after you fix that. ::: embed/ephy-overview.c @@ +94,3 @@ + GtkTreeModel *model; + GtkTreeIter iter; + model = gd_main_view_get_model (GD_MAIN_VIEW (gtk_widget_get_parent (widget))); Why can't you get the model from the iconview itself?
Are you committing this?
To commit this I need to know if bug #690110 removing ::link-message is going in too. The difference is that if that patch is not accepted I have to add an if EPHY_IS_OVERVIEW g_object_get ("status-message") else EPHY_IS_WEB_VIEW ephy_web_view_get_status_message () The reason is that API in EphyWebView is returning *either* of ::link-message or ::status-message, whichever is not NULL. I opted for removing ::link-message because the distinction between messages coming from load events (status-message) and hover events (link-message) is never used. What do you think? I rather commit the other patch but I can understand if you prefer more time to review the other one. Btw, I locally fixed your comment about getting the model. Thanks for the review
(In reply to comment #38) > To commit this I need to know if bug #690110 removing ::link-message is going > in too. > > The difference is that if that patch is not accepted I have to add an > if EPHY_IS_OVERVIEW > g_object_get ("status-message") > else EPHY_IS_WEB_VIEW > ephy_web_view_get_status_message () > > The reason is that API in EphyWebView is returning *either* of ::link-message > or ::status-message, whichever is not NULL. > > I opted for removing ::link-message because the distinction between messages > coming from load events (status-message) and hover events (link-message) is > never used. > > What do you think? I rather commit the other patch but I can understand if you > prefer more time to review the other one. > > Btw, I locally fixed your comment about getting the model. Thanks for the > review Let's get this in without waiting for the other patch, because I'm not really sure we want to get rid of the distinction.
Created attachment 231922 [details] [review] e-overview: add a statusbar for hovered icons Show the URL of the icon in a floating statusbar, like in regular websites. Adds ::status-message to EphyOverview, mimicking EphyWebView's same property. EphyEmbed listens to both properties and updates its statusbar when any of those changes. -- Updated for Claudio's model comment and not using the link-message patch.
(In reply to comment #39) > Let's get this in without waiting for the other patch, because I'm not really > sure we want to get rid of the distinction. Could you please explain in the other bug report why you don't see this possible?
(In reply to comment #41) > Could you please explain in the other bug report why you don't see this > possible? FWIW I'm not saying it's not possible, I'm only saying that the distinction between those things is real regardless of whether we are making a good use of it right now or not. So we should think it through a bit more before getting rid of it (ie, no "this is unused right now" but "this is unused and we really should never need it to know the difference).
Review of attachment 231922 [details] [review]: ::: embed/ephy-overview.c @@ +200,3 @@ + object_class->finalize = ephy_overview_finalize; + object_class->get_property = ephy_overview_get_property; Hey, wow, if we are bothering to make this a property we should really implement 'set_property' and use it instead of hacking around internally to set the data.
Attachment 231084 [details] pushed as 2a5f103 - e-embed: only add one statusbar timeout at a time Attachment 231291 [details] pushed as 1777a51 - e-embed: use an overlay for the overview Attachment 231292 [details] pushed as 5032d8a - e-embed: hide progress for about and ephy-about Committed this ones that are independent of the final patch. Note: Cosimo has a small patch for a 1px line that appears with this change just on top of the statusbar. I'm pinging him about this, he was waiting for me to commit this part.
I found a problem in the implementation, suggestions welcome: 1. Hover a thumbnail in the overview you get, say "http://twitter.com" 2. Click it, you get "Loading twitter.com" 3. You move the mouse out (when the thumbnail is gone, or to a blank space while it is still visible), the tooltip disappears completely So, in certain situations hovering-out of a thumbnail in the overview can destroy the "Loading…" tooltip, and since that is set only once you lose feedback about the page status.
With the new HTML implementation of the overview, the snapshots are now working as standard links, showing the target on the statusbar when overing them.