GNOME Bugzilla – Bug 350853
Visual indication the current page is bookmark
Last modified: 2016-09-28 16:32:56 UTC
From a comment in bug #324452 Wouter Bolsterlee (uws): However, I would really welcome a visual indication that the current page is one of my bookmarks. What about adding an indicator/icon/color to the location bar, eg. a bookmark icon at right side of the location bar? Diego Escalante Urrelo (dieguito): The icon is a good idea I would like to try doing this, is this still hack-eable or it has been fixed in 2.15?
How can this be done? A background color like secure sites? Maybe an icon?
A libsexy-like icon perhaps?
Created attachment 70921 [details] Mockup
I need to solve this doubts: 1. Where is the best place to add this (connect to a signal, inside an already existing callback) 2. What's the proper way to know if the url is already bookmarked About (2): Wouter pointed me to src/bookmarks/ephy-bookmark-editor.c, there's some calls to ephy_bookmarks_get_similar with a GPtrArray as an argument, but that seems overkill for this case. Ideas?
Perhaps some new API that does exactly what you want would be nice: ephy_bookmark_bookmark_for_url(...) or something along those lines. CC'ing harves.
Created attachment 79147 [details] [review] Implements a bookmark icon It's kind of dirty actually, I'm fooling ephy_toolbar_set_security_state... I think that function should be generic instead of security centric. But nevermind, what do you think.
Created attachment 79148 [details] Screenshot of the implemented icon :)
This looks fine in principle. The patch needs to be done differently though :) This has nothing to do with the security-state so let's not abuse that interface. You should instead add a set_is_bookmark interface that show/hides the bookmark icon. Also the icon needs to be present _in addition_ to the security icon if it's a secure page. And maybe clicking the icon should open the bmk properties windows for it?
Pure and delicious abuse :). I don't think there should be more than 1 icon, the current patch shows either the security or the bookmark icon, being the latest the lest preferred. The security thingy needs to be make more general.
Christian, is there a reason why a new interface is better than making the current one more general?.
Created attachment 84596 [details] Screenshot of new implementation See next attachment for patch.
Created attachment 84597 [details] [review] Implements the icon without abusing existing functions. Ok. This patch should be fine, I made a whole new set of functions for this (well I copy-pasted). There are some rough edges like the name of some variables and functions, I'm listening to critics. :)
So any decision about this? Xan suggested time ago that we are looking a problem for a solution. In my opinion, this doesn't produce clutter and is a nice detail. What do you think epiphaniers?
does this show the icon in the dropdown too, or only in the location bar? 'cause I'd think having this icon besides each bookmark in the dropdown would be very good for consistency *and* for quickly spotting bookmarks
Diego, i think that this is a very nice detail that i have not seen in other browsers, and is quite useful too! I'd like to see this in Epiphany.
Created attachment 95831 [details] [review] Updated patch
ephy_toolbar_set_is_bookmark only ever uses EPHY_STOCK_BOOKMARK as stock-id; so you can remove the book-stock-id property. +++ src/ephy-statusbar.h (working copy) These changes look unrelated to this bug ? +g_param_spec_string ("book-tooltip", "bookmark-tooltip" ? + g_print("sync show_book: %d;\n", priv->show_book); Remove this. + gboolean show_lock = FALSE; + show_lock = (gboolean) g_object_get_data (G_OBJECT (priv->actions[LOCATION_ACTION]), + "show-lock"); [...] + gboolean show_book = FALSE; + show_book = (gboolean) g_object_get_data (G_OBJECT (priv->actions[LOCATION_ACTION]), + "show-book"); Totally bogus :) I guess you meant to write g_object_get (action, "show-X", &show_X, NULL); ? + g_print ("%s; lock_is_visible?: %d; is_bookmark: %d\n", location, lock_is_visible, is_bookmark); Remove. + g_signal_connect_object (embed, "ge-location", + G_CALLBACK (sync_tab_is_bookmark), + window, 0); Use the notify::address handler for this. (Also the ge-location signal is going to be removed anyway.) - g_signal_connect_swapped (priv->statusbar, "lock-clicked", - G_CALLBACK (gtk_action_activate), action); Doesn't belong into this bug, does it? } +//bookmarks +void No C++ comments in C files. Add an empty line. + g_print("show_book: %d;\n", show_book); Remove.
Created attachment 96816 [details] [review] Non working update I can't get this to work on trunk, no idea why... not even the signals seem to be called, maybe I'm doing an obvious mistake... I'm a bit sleepy :)
Created attachment 96817 [details] [review] Non working update, for real Damn, wrong patch.
I can't get the signal to call sync_tab_is_bookmark, no idea why. I would blame myself. g_object_notify's doesn't seem to ever be emitted. Which is very weird. Check my g_debugs.
Created attachment 96818 [details] [review] Ok, now it works :) It was a silly mistake, but besides that seems like enabling debug at build time and later using g_debug doesn't work well :/. Or maybe just a mistake by me?
Created attachment 98900 [details] [review] Update to trunk (r7659), after EphyTab migration Update to trunk. There are some bugs: when changing tabs betweeen bookmarked and new pages, the icon disappears or stays there, even when that's not appropriate.
Marking needs-work based on comment 22.
Marking my old patch obsolete.
*** Bug 517060 has been marked as a duplicate of this bug. ***
Maybe we can consider the use firefox gave to the star in the location bar... give some similar use to this icon. I thought it was a good idea when I tried ff3.
I don't see why we should deviate from the stock bookmark icon!
Firefox 3 shows silhouette when current site is not bookmarked yet. Clicking it bookmarks the site (without any questions asked). Without this functionality it's just cute eye candy. I would need to keep the bookmark button on the toolbar anyway. :-/
Diego, ping?
Ok I implemented this with woohoo bar, attaching the patch that goes on top of it and marking this bug as a dependant of the other. See bug #541782.
Created attachment 126397 [details] [review] [PATCH] Include a bookmark indication in the woohoo bar lib/widgets/ephy-location-entry.c | 42 ++++++++++++++++++++++++++++++++++++- src/ephy-completion-model.c | 17 +++++--------- src/ephy-location-action.c | 2 - 3 files changed, 47 insertions(+), 14 deletions(-)
The completion popup thing is done in trunk and 2.26. Now, do we want an icon in the entry itself? If so I think we could show it when there was no lock icon to be shown, hence the priority would be: - lock icon - fav icon - nothing Sounds good?
Wouldn't that be a bit strange? Bookmarked SSL sites won't get an icon in that case...
Why do not show more than one icon?
(In reply to comment #32) > The completion popup thing is done in trunk and 2.26. Now, do we want an icon > in the entry itself? > > If so I think we could show it when there was no lock icon to be shown, hence > the priority would be: > > - lock icon > - fav icon > - nothing > > Sounds good? What about the page icon? Or are you just talking about the right hand side of the address entry? _If_ we could show >1 icon, we need to be careful to keep at least the most-used one (probably bookmark indication) in the same spot at all times.
If we want more than 1 icon, we have to ask GTK+ to add that to the new GtkEntry *now* before they freeze the code. Right now you can only put 1 icon to each side.
This epiphany user votes for more than one icon - bookmark on one side, and rss icon on the other (if rss auto discovery is present)
*** Bug 600444 has been marked as a duplicate of this bug. ***
@Diego, what's the status on this?
*** Bug 615088 has been marked as a duplicate of this bug. ***
Now got a yellow star!