GNOME Bugzilla – Bug 556040
bookmark monitor
Last modified: 2015-08-21 03:24:00 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.
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.
(Oh, yeah, the patch totally needs locking and other details sorted too. Too early to review it.)
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.
(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...
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.
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.
(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
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.
(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.)
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.
*** Bug 555534 has been marked as a duplicate of this bug. ***
(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
Ugh, of course I'm off by one with the release numbers in comment 12; s/2.26/2.28/. Sorry about that.
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,
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.
(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.
seems clear at this point that we didn't want this enough to merge it