Re: Bookmarks reworked

[Christian Persch <chpe stud uni-saarland de>]

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

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

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

Regards,
	Christian