Re: Bookmarks reworked

[Christian Persch <chpe gnome org>]

Hi,

On Mon, 2004-09-06 at 17:10, Peter Harvey wrote: 
> Hi,
> 
> On Mon, 2004-09-06 at 13:54 +0200, Christian Persch wrote:
> > We only allow bookmarks on main toolbar for backwards compatibility
> > reasons, not by choice.
> 
> But that's my favourite bit. Why is this "not by choice"? :(
Don't worry, it's not going to go away :)

> Is it going to be acceptable for an Epiphany extension to add signal
> handlers to the main toolbar view and model? I am planning to rewrite my
> current bookmarks patch as an extension, with the end result being that
> bookmarks exist on the main toolbar.

Yes, it's completely acceptable to add signal handlers to the toolbars
model, and to the toolbar.

> > No, bookmarks on the main toolbar are also stored as type=_NETSCAPE_URL
> > data=<url>. It is only due to a bug in Epiphany 1.0 and 1.2 that the
> > toolbars layout in those versions have bookmarks on the main toolbar
> > with type application/x-toolbar-item and data=GoBookmark-1234. (And
> > importing the toolbar layout from those versions in 1.4 doesn't fix this
> > up; we could have done it, but felt it wasn't worth it.)
> 
> I *think* the code that I stole from ephy-bookmarksbar.c will migrate
> from the GoBookmark system to _NETSCAPE_URL_. The get_item_id signal
> handler converts any older forms of toolbar data into an action name,
> and get_item_data converts action names back into _NETSCAPE_URL_ data.
> The end result (though I could be mistaken) is that if a GoBookmark-1234
> format entry is in epiphany-toolbars-2.xml it is automatically converted
> to be a _NETSCAPE_URL_ when the toolbars are saved.

Are you sure? IIRC it only converts between the data and the action names,
but doesn't change the _type_ (application/x-toolbar-item vs.
_NETSCAPE_URL). But maybe I'm wrong.

> > > I took this hint from Marco when I suggested auto-destroying topics.
> > > I've probably misinterpreted, but my understanding was that having too
> > > many signal handlers for a given object would cause a performance hit:
> > >         I'm not sure about autodestroy ... I remember Christian was
> > >         worried about the perf cost of connecting one signal per
> > >         bookmark. Maybe the group could take care of this ?
> > 
> > My concern then was about a connection to the favicon cache, which uses
> > gobject signals; I think ephy node signals are more light-weight. See
> > also below...
> 
> I was wondering why ephy nodes aren't GObjects. I thought it might've
> been a 'weight' problem, seeing as there are a lot of ephy node
> instances and not all the functionality of GObject is required. Is good
> to know.

Yes. During early 0.x, EphyNode was a GObject, but GObject instantiation
and finalisation are quite slow... too slow for our use in this case.

> > > My preferred solution would be to modify the EphyNode class by telling
> > > it to send a signal to it's own listeners. Each action instance could
> > > then listen directly to changes in each EphyNode. We could do this
> > > easily by adding the following to the bottom of ephy_node_set_property.
> > > 
> > >        ephy_node_emit_signal (node, EPHY_NODE_CHILD_CHANGED,
> > >                               node, property_id);
> > 
> > Yes, I agree.
> > 
> > I think this change is simple enough, and I've also always felt this
> > shortcoming of EphyNode... just hadn't had enough motivation to actually
> > do it :)
> 
> I'll include something like that in my patch and make ephy-bookmark-
> action and ephy-topic-action listen directly to their relevant
> EphyNodes. When a GtkAction is disposed does it notify any
> GtkActionGroups which contain it?

While a GtkAction is inside a GtkActionGroup it is owned (strong ref) by
the group, and will therefore not be disposed.

> > > I knew that avoiding the use of objects would probably go against
> > > Epiphany's design style. But I did this to attempt to simplify things in
> > > preparation for creating an extension. In the end I would like to have a
> > > single object (the extension) which:
> > >      1. Listens to changes in the bookmarks database.
> > >      2. Calls an external function to generate some kind of
> > >         representation of the bookmarks menu (XML maybe?).
> > >      3. Updates each EphyWindow using that representation.
> > > 
> > > Virtually all of the ephy_bookmarks_ui functions would become the member
> > > functions of the extension object. We would gain efficiencies by having
> > > the EphyBookmarksUI class store the current menu structure.
> > 
> >  You're right, separate objects for this is more in line with the design
> > of ephy. So I would do this like this: have the bookmarks extension add
> > the menu object to the window, and the menu object would auto-update,
> > instead of having the extension update the menus explicitly. This also
> > is more modular: you could override the bookmarks menu from _another_
> > extension, while keeping the bookmarks model itself:
> > So for example, you could have an extension which adds your special
> > auto-topic menus, while still using the same bookmarks as the regular
> > menus. And another extension could exchange the whole model (for
> > example, a hypothetical GaleonBookmarksExtension could provide the whole
> > of galeon's bookmarks system for ephy).
> > That also is another reason why I split the bookmarksbar from the main
> > toolbar: an alternative bookmarks extension might not use
> > EggEditableToolbar for its bookmarksbar at all.
> 
> I'm not sure I understand exactly what you've said, but from what I do
> understand you'd like to see the bookmarks presentation system and the
> bookmarks storage system modularised to allow alternative presentation
> systems for a given storage system. This would be very complicated, and
> would probably involve extension dependencies (for example, my
> AutoHierarchyExtension would refuse to load until
> EpiphanyBookmarkExtension or GaleonBookmarksExtension is loaded).
> 
> My preference would instead be:
>       * Each bookmarks plugin includes storage and presentation systems.
Ok, that simplifies things. I just thought we could reuse the same
storage code with different presentation systems (std. vs. your
auto-hierarchy).

>       * Only one bookmarks plugin is loaded at any time (to avoid UI
>         manager namespace conflicts).
Yes, only one bookmarks system at a time.

>       * Bookmarks plugins may use the same storage (for example, some
>         may use ephy-bookmarks.c code and store their bookmarks in the
>         same file in .gnome2/epiphany) or may use completely different
>         ones (Galeon, Mozilla, LDAP?)
> 
> This seems simpler to me, and allows the development of any kind of
> bookmarks system. As the presentation system is usually closely tied to
> the storage system I don't see a huge loss either. My final auto-
> hierarchical bookmarks patch will include various presentation options,
> one of which will be to build the default bookmarks menu.
> 
> > >  I wanted
> > > to allow any object or signal handler which has a pointer to the UI
> > > manager to be able to find and create bookmark/topic actions in a shared
> > > and common way. If a window doesn't have a bookmarks menu, a common way
> > > of finding and/or creating actions should still be available.
> > 
> > Hm, I'm not sure I understand... when you don't have a bookmarks menu,
> > why would you need to create bookmarks actions?
> 
> I was working on the presumption that bookmark actions and topic actions
> might be used elsewhere. For example, perhaps a toolbar would be created
> which doesn't belong to an EphyWindow. This doesn't seem to be the case,
> and as I move more towards developing an extension it seems that
> separating the idea of EphyWindow and GtkUIManager was unnecessary. Will
> fix.
> 
> 
> I'm still not convinced of the need for per-window objects. I see an
> 'extension' as providing a collection of signal handlers which are
> attached to a window to give alternative behaviours. If those behaviours
> don't require complex per-window state information I'd prefer to avoid
> the creation of yet more objects.
Ok.

> One query that I do have: if I create per-window objects when
> ephy_bookmarks_ui_attach_window is called, how should I find and destroy
> them when ephy_bookmarks_ui_detach_window (not yet written) is called?

You can use g_object_set_data to attach pointers to, and
g_object_get_data to retrieve pointers from a GObject.

Regards,
	Christian