Re: Bookmarks reworked

[Peter Harvey <pah06 uow edu au>]

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"? :(

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.

> 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.

> > 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.

> > 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?

> > 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.
      * Only one bookmarks plugin is loaded at any time (to avoid UI
        manager namespace conflicts).
      * 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.

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?


FYI, I've been working on speed improvements to my current patch as
well. It now keeps a shared XML representation of the bookmarks menu (so
it only needs to be generated once). It also delays the update of each
EphyWindow until the user selects the 'Bookmarks' menu. The result is
that, for each window:
     A. A signal is received whenever the current bookmarks menu has
        been invalidated (tree changed, etc.). This signal handler:
             1. Removes the dynamically constructed bookmarks/topics
                parts of the 'Bookmarks' menu from the window.
             2. Sets the shared bookmarks menu XML string to "".
     B. A signal is received whenever someone accesses the bookmarks
        menu. This particular idea was discussed on mailing lists
        previously. This signal handler:
             1. Generates the shared bookmarks menu XML string if
                needed.
             2. Tests to see if there is already a dynamically
                constructed bookmarks/topics part of the 'Bookmarks'
                menu for this window.
             3. If there is none, it uses the XML string to add it.
     C. A signal is received whenever a bookmark/topic is added or
        modified, and the actions within that window are updated
        accordingly.


Thank you for all the useful comments. Am trying to incorporate
solutions to most of them right now.

Regards,
Peter.