Re: Bookmarks reworked

[Peter Harvey <pah06 uow edu au>]

On Fri, 2004-09-03 at 16:04 +0200, Christian Persch wrote:
> Hi,
> 
> On Wed, 2004-09-01 at 14:41, Peter Harvey wrote:
> > USER VISIBLE CHANGES
> > ====================
> >       * The bookmarks bar no longer exists, and most references to it
> >         have been removed. Bookmarks and topics are added to the toolbar
> >         by drag-and-drop from the bookmark editor.
> >       * 'Open in tabs' is (temporarily) only available via the context
> >         menu for topics which are on the toolbar.
> >       * You cannot 'Move left' or 'Move right' a topic/bookmark on the
> >         toolbar. The only reason for removing this is to make code
> >         maintenance simpler. This can be done much more easily using the
> >         toolbar editor anyway. You can still 'Remove' topics/bookmarks
> >         from the toolbar by right-clicking.
> 
> I don't agree with those; the ability to manipulate the toolbar with the
> keyword is required for a11y, and we shouldn't regress the (limited)
> progress we've made in 1.3.x in this regard.

True. But I'd suggest that a 'Move' command which allows you to use the
cursor keys to move the items around the toolbars is required. Either
that, or an 'Up' and 'Down' command are needed. In removing this code
from topic/bookmark actions I was really just clearing the way for
adding more complete a11y support to *all* toolbar items.

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

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

> You also removed the favourites menu, what was the reason behind that?

Sorry, I couldn't see where it was used. Where is it used?

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

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

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 like that your patch unifies building the bookmarks menu and the menu
> of the topics on toolbars; that's something we should have in ephy.

That was what I originally set out to do. It just grew into the monster
that you see today... :)

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

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

I've tried to eliminate the resulting inefficiencies by calling the
get_action_group function infrequently. For example, we call it only
when we need to add something to the toolbar, or need to regenerate the
bookmarks menu. These are not common tasks, or are sufficiently CPU
intensive already that a quick search won't be noticeable.

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

> I hope you don't let this discourage you, and I hope I didn't come out
> too negatively -- moving away from rebuilding the whole bookmarks menu
> on any changes is indeed badly needed.

One thing I tell others when they learn to write programs - be prepared
to delete things. I'm very happy to receive the criticisms as avoid
becoming too 'attached' to code. I'll continue to work from it, but I'm
fine to make mass design changes (it's already had a few revisions).

Regards,
Peter.