Re: Bookmarks reworked

[Christian Persch <chpe gnome org>]

Hi,

On Sun, 2004-09-05 at 08:19, Peter Harvey wrote:
> > I also disagree about the removal of the bookmarksbar; the bookmarksbar
> > is behaviourally different from the main toolbar, so having it separate
> > makes sense to me.
> > This in no way hinders making bookmarks an extension; in fact I
> > separated the bookmarksbar from the main toolbar during 1.3.x
> > expressedly for the purpose of making this possible, i.e. removing as
> > much dependency on bookmarks in src/ as possible, and moving anything
> > bookmarks-related in src/bookmarks. The only thing which at the moment
> > makes bookmarks separation incomplete is the fact that the location
> > entry autocompletion depends on the bookmarks; and we don't know yet how
> > to remove that dependency.
> 
> The problem with having the bookmarksbar as a model/view pair of classes
> which are distinct from the main toolbar is that either:
>      A. You can't have bookmarks/topics on the main toolbar; or
>      B. You have to add signal handlers for the main toolbar which allow
>         it to fetch action names, etc from the bookmarks bar.
> 
> In current Epiphany CVS you follow option B and forward action_request
> events to the bookmarksbar code. Due to this decision, I don't see why
> the bookmarksbar is behaviourally different. From a user's point of view
> I am allowed to move bookmarks/topics between the different toolbars, so
> they should be behaviourally the same.

We only allow bookmarks on main toolbar for backwards compatibility
reasons, not by choice.

> In my patch I removed the bookmarksbar model/view and added signal
> handlers which would interpret data (either stored in XML, or drag-and-
> dropped) and create actions (shared by the bookmarks menu). The added
> bonus is that we don't end up with two different XML files containing
> different types of data (epiphany-toolbars-2.xml would have action names
> for bookmarks, and epiphany-bookmarksbar.xml would have URLs).

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

> > > MAIN SOURCE CHANGES
> > > ===================
> > > 
> > > ephy-bookmarksbar and ephy-bookmarksbar-model are no longer in use. They
> > > are replaced by a minor modification to the normal toolbar, and the code
> > > in ephy-bookmarks-ui.
> > > 
> > > ephy-bookmarks-menu now contains only functions to create (not maintain)
> > > action-based bookmark menus. With the current patch ephy-bookmarks-menu
> > > implements my hierarchical menu system, but could be easily switched
> > > back (will do this later).
> > > 
> > > ephy-bookmarks-ui is new, and contains signal handlers which:
> > >       * listen to changes in the bookmarks database and update menus and
> > >         actions accordingly.
> > >       * listen to requests for information coming from the toolbar
> > >         model, allowing it to read/write bookmarks and topics on the
> > >         toolbars.
> > > 
> > > ephy-bookmark-action and ephy-topic-action now focus on being actions
> > > and actions only (they don't listen for changes in the bookmarks
> > > database, they don't generate menus, etc.). There are functions to
> > > find/create actions for EphyWindows, to get a standard name for these
> > > actions, and to synchronise the information contained in the actions
> > > with that in the bookmarks database.
> > 
> > You removed the self-updating of bookmarks and topics actions when their
> > node changes, and instead use explicit foreign updating -- what was the
> > reason for that?
> 
> 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...

> In current Epiphany code, when an EphyNode's property is changed it
> notifies each of it's *parents* listeners. In current Epiphany code this
> means that a lot of signal handlers are called needlessly when a
> bookmark or topic changes a property. To me it made sense that only one
> signal handler was required, and it would identify the correct action
> (if it existed) and tell it to update.
> 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 haven't done this because I wanted to avoid modifying most of the code
> in the lib directory, especially those which are such core elements as
> ephy_node.c.

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 :)

> > What I don't like about your _current_ code is that you've made not much
> > use of objects; for example you make the bookmarks menu not a
> > self-contained object and instead explicitly rebuild it from elsewhere.
> 
> 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.

> > That also causes your code to have no place to store the data for the
> > menus (like which action group to use), so you have to search all action
> > groups of the ui manager to find the correct one.
> > IMHO the bookmarks menu should continue to be an object, maintaining the
> > bookmarks action group, and delegating the building of the actual UI,
> > like your code does now.
> 
> I decided against having a bookmarks menu object which maintains the
> bookmarks action group, because the bookmarks menu is attached to a
> window while the action group is associated with a UI manager.

window:UI-Manager correspond 1:1.

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

[...]
> My design ideas are of course in conflict with the fact that a central
> piece of code listens for bookmark/topic property changes and notifies
> the relevant actions. My design could be made consistent if EphyNodes
> would send notification events to their own listeners rather than their
> parent's listeners (see previous comments in this email about this).
Agreed, again :)

Regards,
	Christian