After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 556040 - bookmark monitor
bookmark monitor
Status: RESOLVED WONTFIX
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
: 555534 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-10-12 18:07 UTC by David Zeuthen (not reading bugmail)
Modified: 2015-08-21 03:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wip patch, for discussion (111.97 KB, patch)
2008-10-12 18:25 UTC, David Zeuthen (not reading bugmail)
none Details | Review

Description David Zeuthen (not reading bugmail) 2008-10-12 18:07:38 UTC
As outlined in bug 147434 comment 14, it would be nice with an GObject based interface to bookmarks. Will attach some work-in-progress patch implementing this.
Comment 1 David Zeuthen (not reading bugmail) 2008-10-12 18:25:08 UTC
Created attachment 120454 [details] [review]
wip patch, for discussion

For now, disregard the noise from the gicon serialization patch in bug 555740 (this patch relies on that feature).

 docs/reference/gio/gio-sections.txt      |    2 
 docs/reference/glib/tmpl/iochannels.sgml |    8 
 gio/ChangeLog                            |   12 
 gio/Makefile.am                          |   23 
 gio/gbookmark.c                          |  430 +++++++++++++++++
 gio/gbookmark.h                          |  122 ++++
 gio/gbookmarkmonitor.c                   |  145 +++++
 gio/gbookmarkmonitor.h                   |  104 ++++
 gio/ggtkbookmark.c                       |  363 ++++++++++++++
 gio/ggtkbookmark.h                       |   73 ++
 gio/ggtkbookmarkmonitor.c                |  776 +++++++++++++++++++++++++++++++
 gio/ggtkbookmarkmonitor.h                |   81 +++
 gio/gicon.c                              |  338 +++++++++++++
 gio/gicon.h                              |   18 
 gio/gio-bookmark.c                       |  420 ++++++++++++++++
 gio/gio.h                                |    2 
 gio/gio.symbols                          |    2 
 gio/gioenums.h                           |   35 +
 gio/giomodule.c                          |    9 
 gio/giotypes.h                           |    8 
 gio/gnativebookmarkmonitor.c             |   34 +
 gio/gnativebookmarkmonitor.h             |   43 +
 gio/gunionbookmarkmonitor.c              |  344 +++++++++++++
 gio/gunionbookmarkmonitor.h              |   48 +
 gio/tests/Makefile.am                    |    6 
 25 files changed, 3433 insertions(+), 13 deletions(-)

Notes

 - For now, only a bookmark monitor for ~/.gtk-bookmarks is available. We
   should add one for GBookmarkFile-based shortcuts (~/.shortcuts.xbel) when
   releasing the GLib that will go with GTK 2.16. We need to wait for this
   simply because GTK and GLib are on different release schedules. However,
   we can probably get away with patching stable GTK 2.14 to read the sidecar
   for stuff like icons.

 - The .gtk-bookmarks monitor utilizes a sidecar for storing things like icons

 - The GBookmark interface isn't complete; at least not for what is needed to
   support GBookmarkFile style bookmarks

 - Going back and forth whether we should have a transaction-based interface
   for atomic updates of many bookmarks at once. It would look something like
   this

     GTransientBookmark *bookmark;
     GList *transaction_objects;

     transaction_objects = NULL;

     // create bookmark
     bookmark = g_transient_bookmark_new ();
     g_bookmark_set_uri (bookmark, "/home/Hacking/glib", NULL, NULL);
     g_bookmark_set_title (bookmark, "GLib", NULL, NULL);
     g_bookmark_set_description (bookmark, "My GLib checkout", NULL, NULL);
     ...
     transaction = g_list_prepend (transaction, bookmark);

     // modify bookmark
     bookmark = g_transient_bookmark_new_from_bookmark (existing_bookmark);
     g_bookmark_set_description (bookmark, "New Description", NULL, NULL);
     transaction = g_list_prepend (transaction, bookmark);
     
     // delete bookmark
     bookmark = g_transient_bookmark_new_from_bookmark (existing_bookmark_to_delete);
     g_bookmark_delete (bookmark);
     transaction = g_list_prepend (transaction, bookmark);

     if (!g_bookmark_monitor_run_transaction_sync (monitor, transaction, &error))
       {
         // handle error
       }
     g_list_foreach (transaction, (GFunc) g_object_unref, NULL);
     g_list_free (transaction);

   where GTransientBookmark is an in-memory bookmark where operations can
   never fail. Also, there'd be an asynchronous interface for this.

   Anyway, we can add this later I think. Just a thought

 - Need an interface to create bookmarks. Again, no good way to do this
   without transactions. E.g. we don't want

    g_bookmark_monitor_create_bookmark (GBookmarkFlags flags,
                                        GFile *location,
                                        const char *title,
                                        ...

   since that will prevent us from extending the GBookmark interface in
   the future. I think we probably want transactions. I don't know.

 - GBookmarkFile has no notion of positions. We need that since the user
   can reorder bookmarks using drag+drop. Also, we probably need to be
   able to write arbitrary key/value pairs, e.g. we want to write both
   a GIcon tag and also the normal (more limited) icon tag.

 - We probably want the XBEL bookmark stuff in GVfs because we might want
   to take advantage of stuff like PolicyKit (which is fine for GVfs to
   depend on; not so for GIO proper) to create/modify/delete system-wide / 
   mandatory bookmarks.

 - Maybe we want gio-bookmark(1) to be part of GVfs (as gvfs-bookmark) since
   that seems to be what we do for other GIO commandline tools (such as
   gvfs-mount(1), gvfs-open(1) etc.)

Anyway, a bunch of unresolved issues. But most of the code is there now.
Comment 2 David Zeuthen (not reading bugmail) 2008-10-12 18:52:10 UTC
(Oh, yeah, the patch totally needs locking and other details sorted too. Too early to review it.)
Comment 3 Matthias Clasen 2008-10-13 21:16:00 UTC
Some comments from a face-to-face discussion:

- The transaction/transient-bookmark/async api seems a bit too much, better to
  have a simple sync api, and let implementations deal with it

- on the GtkBookmark interface: seems better to have regular properties, rather 
  all those custom may-fail setters

- positions are problematic in combination with merging. not obvious how to
  handle that

- GBookmarkFile needs to preserve unknown elements, not merely ignore them

- storing a file uri for "icon" and a more flexible "gicon" property which can
  handle themed icons needs some care when other implementations update "icon"
  and ignore "gicon"

- instead of the "server bookmark" flag, might be more obvious to simply have
  "show on desktop"/"show in places" flags.
Comment 4 David Zeuthen (not reading bugmail) 2008-10-13 21:50:13 UTC
(In reply to comment #3)
> Some comments from a face-to-face discussion:
> 
> - The transaction/transient-bookmark/async api seems a bit too much, better to
>   have a simple sync api, and let implementations deal with it

Yeah, agreed. Still leaves an open question on how creation is handled. Maybe we have a constructor with the most common stuff and apps need to use setters for the other stuff. Not atomic though. Maybe that's not a big deal.

> - on the GtkBookmark interface: seems better to have regular properties, rather 
>   all those custom may-fail setters

Right. So the flags (cf. GBookmarkFlags) needs to include information about what properties can be set. We'd also want to preserve the C bindings though, they can just never fail, e.g. no GError is passes. So if we do this, we force applications to check the flags before writing otherwise there will warnings spewed on stderr. I bet tons of apps will get this wrong, it's more obvious with GError.

Or maybe we just force all implementations to support everything, however in that case that's extensibility you see right there, going out the window.

> - positions are problematic in combination with merging. not obvious how to
>   handle that

Yeah. Need to think about that some more.. especially since system-provided bookmarks aren't writable at all. So maybe we keep all positions in a separate file somewhere. But that sounds nasty. I don't know.

> - instead of the "server bookmark" flag, might be more obvious to simply have
>   "show on desktop"/"show in places" flags.

The only way to implement this with GBookmarkFile would be using ShowOnDesktop and ShowInPlaces groups. Not sure that's what the GBookmarkFile authors intended when they added groups...
Comment 5 David Zeuthen (not reading bugmail) 2008-10-13 23:51:38 UTC
I thought a bit more about this when driving home.

(In reply to comment #4)
> (In reply to comment #3)
> > Some comments from a face-to-face discussion:
> > 
> > - The transaction/transient-bookmark/async api seems a bit too much, better to
> >   have a simple sync api, and let implementations deal with it
> 
> Yeah, agreed. Still leaves an open question on how creation is handled. Maybe
> we have a constructor with the most common stuff and apps need to use setters
> for the other stuff. Not atomic though. Maybe that's not a big deal.

So I think most implementations should probably just defer the updating of the bookmark file to an idle handler (but still perform the modifications locally in the GBookmark object, rolling back if the update fails in idle). That way all operations happening from the same mainloop slice will become atomic. If not called from the mainloop (e.g. if g_main_depth() returns 0) we always update the file instead of deferring the update to the mainloop.
Comment 6 Alexander Larsson 2008-10-14 12:06:25 UTC
I don't know if "GBookmark" is the best name, as it signals a pretty broad use. Would you use GBookmark if you're implementing bookmarks in your web browser? Probably not. Maybe it should be something a bit more specific like GFileBookmark?

I think the whole union monitor/extensions thing is way over-engineered. Since all the implementations we care about atm are implementable inside glib without any problems I don't see any gains, just complexity. And if we later really need to do this that could be added without changing the public API.

We probably want to have a can_delete() operation on GBookmark so we can handle mandatory bookmarks in the UI.
Comment 7 David Zeuthen (not reading bugmail) 2008-10-14 15:33:20 UTC
(In reply to comment #6)
> I don't know if "GBookmark" is the best name, as it signals a pretty broad use.
> Would you use GBookmark if you're implementing bookmarks in your web browser?
> Probably not. Maybe it should be something a bit more specific like
> GFileBookmark?

Can do.

> I think the whole union monitor/extensions thing is way over-engineered. Since
> all the implementations we care about atm are implementable inside glib without
> any problems I don't see any gains, just complexity. And if we later really
> need to do this that could be added without changing the public API.

Without a union monitor we'd have to implement support for .gtk-bookmarks and XBEL (or whatever) inside the same class. That can hardly be described as elegant.

If you consider that adding support for native shortcuts (e.g. as provided by e.g. Win32 or OS X, now or in the future) may be implemented in different way on different platforms this gets ugly fast. Especially if you still want to support a legacy .gtk-bookmarks file.

Also, if you consider that it might make sense to use these interfaces for Recent Files and Templates [1], then it makes even more sense to split the interface from the implementations (for example the Templates implementation may synthesize bookmarks from files in ~/Templates and other stuff from /usr/share/Templates). Of course you may now say that we only want to use this for shortcuts and never for Recent Files or Templates. That's fine, but I think that's a bit short-sighted.

Finally, care to explain exactly what kind of "complexity" is added by splitting the interface and the implementation? To me it seems a lot cleaner that putting all sorts of crap in the same class - that's just a recipe for #ifdef hell.

> We probably want to have a can_delete() operation on GBookmark so we can handle
> mandatory bookmarks in the UI.

That's already expressed in the flags (can_delete() would return TRUE iff G_BOOKMARK_FLAG_WRITABLE is TRUE).

[1] : http://mail.gnome.org/archives/nautilus-list/2008-June/msg00123.html
Comment 8 Alexander Larsson 2008-10-15 06:00:22 UTC
Also, the API needs to be more explicit about ordering, and allow manual reordering. The bookmarks menu needs to have the same order each time, and needs to be reorderable by the user.

This is actually somewhat complicated on the implementation side if we want to merge different files. We need to think about how to store the ordering information cross file boundaries.

Oh, and it sounds like (not sure here) you might be proposing merging data from different sources like gtk-bookmarks and xbel files at the same time. I don't think that is a good idea, if we switch we should just migrate on login the first time.
Comment 9 David Zeuthen (not reading bugmail) 2008-10-15 18:46:45 UTC
(In reply to comment #8)
> Also, the API needs to be more explicit about ordering, and allow manual
> reordering.

The way it works right now with the GGtkVolumeMonitor implementation in the patch is that all all affected bookmarks gets reordered (complete with ::changed signals) when you call

 g_bookmark_set_position (bookmark, 2);

I'm not sure we want a more complicated API than g_bookmark_set_position(); typically apps will do

 - get bookmarks using g_bookmark_monitor_get()
 - stuff bookmarks into a ListStore, one by one; then create a TreeView
 - in the drag'n'drop handler, set the new position of the dragged
   bookmark when it's dropped
 - wait for a ::changed signal on GBookmarkMonitor and redo the ListStore

Anyway, that was the motivation behind such a simple API.

> The bookmarks menu needs to have the same order each time, and
> needs to be reorderable by the user.
> 
> This is actually somewhat complicated on the implementation side if we want to
> merge different files. 

If we want to have global and global+mandatory bookmarks we need something like this _somewhere_ depending on whether we do union monitors or whatever. It seems more logical to me to solve the problem in a way that makes the bookmark monitor not care about it (see below).

> We need to think about how to store the ordering
> information cross file boundaries.

Yes. I've been thinking about this. I think we need to keep a ~/.gio-bookmarks-order file that specifies the order. The union monitor will maintain that file (update it when new bookmarks are added / removed etc.) and sort output of of it's g_bookmark_monitor_get_bookmarks() function according to it.

(Considering several processes in your session will be using a bookmark monitor, this can be a bit racy unless you're careful. But it's definitely doable, just need to be careful.)

With such separate monitor implementations will thus not need to care about ordering at all. 

So if we do this, we need to remove the concept of positions in GBookmark and move the set_position() method onto the GBookmarkMonitor class. Only the union monitor would implement it this method.

It's a separate question if the user can move global+mandatory bookmarks around (the user should be able to move global bookmarks that are not mandatory though). E.g., we might want to force these to be in the top of the list. I don't know. Maybe not.

> Oh, and it sounds like (not sure here) you might be proposing merging data from
> different sources 

Why, I am ;-)

> like gtk-bookmarks and xbel files at the same time. I don't
> think that is a good idea, if we switch we should just migrate on login the
> first time.

I'm not convinced why not merging is a bad idea and why some fragile migration script (might not get run, what happens for upgrades in the session etc.) is any better.

It seems to me that if we want to migrate .gtk-bookmarks to some other format, it's probably best done in the union monitor as part of it's ordering-file maintenance tasks (it would have locking etc. sorted already). 

(Heck, then the ordering file that the union monitor use could be named ~/.gtk-bookmarks instead of ~/.gio-bookmark-order for backwards compat.)

Comment 10 Alexander Larsson 2008-10-16 11:06:33 UTC
set_position seems good enough for the app API for me I guess.

How does the order file reference the bookmarks? By uri?

And how does ordering work. For instance, say I have global bookmarks a and b, and user bookmarks c, d. The initial order is probablu a, b, c, d.
Lets say the user rearranges to a, c, b, d.
Then the sysadmin adds a global bookmark. Where is this added?
after b? at the end?

As for migration. It just sounds confusing to have some of your bookmarks stored in one place and some in another (for backups, user/machine migration, or just plain knowing what the heck is happening). And I don't see how migration would be more fragile than having the same code do merging (any parser/writer bugs would affect that too). And if there is a problem with migration you can just fix it up once, but with the merged thing any problem will stick around.

If you add new features to the new format that are not supported by the old you need to migrate bookmarks anyway or suddenly some of your bookmarks magically won't support the new feature.
Comment 11 Alexander Larsson 2008-10-16 12:10:09 UTC
*** Bug 555534 has been marked as a duplicate of this bug. ***
Comment 12 David Zeuthen (not reading bugmail) 2008-11-04 04:01:45 UTC
(In reply to comment #10)
> How does the order file reference the bookmarks? By uri?

Yeah (but this code isn't written yet).

> And how does ordering work. For instance, say I have global bookmarks a and b,
> and user bookmarks c, d. The initial order is probablu a, b, c, d.
> Lets say the user rearranges to a, c, b, d.
> Then the sysadmin adds a global bookmark. Where is this added?
> after b? at the end?

Depends I guess. I'd say global bookmarks can be moved around (and also deleted and modified), mandatory global should always appear at the top. 

I'm not sure whether the interface should be making guarantees about this kind of thing, we simply just need to ensure the API is sufficiently clear about what guarantees it makes (and more to the point, what guarantees it doesn't make). 

There's some work left here, sure, any current patches attached are just meant to illustrate ideas (so when you ask questions, try to come up with concrete proposals).

> As for migration. It just sounds confusing to have some of your bookmarks
> stored in one place and some in another (for backups, user/machine migration,
> or just plain knowing what the heck is happening). And I don't see how
> migration would be more fragile than having the same code do merging (any
> parser/writer bugs would affect that too). And if there is a problem with
> migration you can just fix it up once, but with the merged thing any problem
> will stick around.
> 
> If you add new features to the new format that are not supported by the old you
> need to migrate bookmarks anyway or suddenly some of your bookmarks magically
> won't support the new feature.

But we can't just migrate away from ~/.gtk-bookmarks without a new GTK+ release (and other users of this feature, like gedit and the panel). That's what I've been trying to say the whole time.

So if you say can't live with e.g. the file chooser not taking advantage of the new feature (unless you patch a stable GTK+ release to do so as I've actually suggested) it's the same as saying we can't do this feature before there's a new GTK+ release. E.g. punt to 2.26.

One point about this whole proposal is to let us do this (bring back the connected server feature we lost with the gnome-vfs -> gio/gvfs migration) for 2.24 by using a sidecar for the GTK+ bookmarks file (that would contain the icons). 

Then apps can start using it (we'd have an API and all etc. etc.) and the special (and annoying case) of GTK+ being on a different release schedule can be fixed as a one-off patch against the stable release (or a distro patch, I don't care). For 2.26 we can then talk about migrating wholesale to a different format by just providing a different implementation of the bookmark monitor

If you want to wait with this feature for 2.26 that's fine, just say so and we can work on something that completely migrates the user away from gtk+ bookmarks. Or if you don't want bookmarks on the glib level at all, that's fine too.

I don't think I've made these finer points sufficiently clear, at least there's a lot of confusion in this bug. We probably need to resolve this question until we actually start talking about API details.

     David
Comment 13 David Zeuthen (not reading bugmail) 2008-11-04 04:27:17 UTC
Ugh, of course I'm off by one with the release numbers in comment 12; s/2.26/2.28/. Sorry about that.
Comment 14 Baptiste Mille-Mathias 2009-05-05 15:07:18 UTC
ccing myself, and just wanted to know if it still in the 2.28 target or not, if so I could set it in the bug.

Cheers,
Comment 15 Alexander Larsson 2009-05-06 08:52:03 UTC
We want it in, although there is a lot of stuff to do, so we'll have to see what gets done in the end.
Comment 16 Norbert Frese 2009-06-03 01:43:55 UTC
(In reply to comment #1)
> 
>  - For now, only a bookmark monitor for ~/.gtk-bookmarks is available. We
>    should add one for GBookmarkFile-based shortcuts (~/.shortcuts.xbel) 

Just because it has not been mentioned in the spec: The file name KDE uses is 

~/.local/share/user-places.xbel

I think a different name has been chosen, because "shortcuts" is a bit misleading.

Perhaps "GUserPlaces" might be a nice name for the API as well, because "GBookmark" seems a bit generic. 

If you want to see the KDE implementation in action, just install an app like KWrite and hit the "Open..." button. Btw, KDE also has to deal with the problem of "preset" system items, that's why there is another file "~/.kde/share/apps/kfileplaces/bookmarks.xml" to remember the position of those items, and the user-places.xbel is just a synchronized subset.
Comment 17 Matthias Clasen 2015-08-21 03:24:00 UTC
seems clear at this point that we didn't want this enough to merge it