Re: Intent to reorganise

[Peter Harvey <pah06 uow edu au>]

On Wed, 2004-08-11 at 19:53 +0200, Marco Pesenti Gritti wrote:
> On Wed, 2004-08-11 at 03:42, Peter Harvey wrote:
> > The things I am currently doing/planning are: 
> >      1. Using only a single, persistent GtkActionGroup (called
> >         "BookmarkActions") per UI manager so that actions are shared
> >         between the bookmarks toolbar and bookmarks menu.
> 
> I wonder if there would be any issue in making the group global (shared
> by all windows). That would probably improve perf which is pretty bad
> atm.

Unfortunately there is an issue. Each action has to do something very
specific, which in the base of bookmark actions is "open *this* bookmark
in *this* window". We need one action per bookmark, per window.

> >      2. Replace the bookmarks menu class with a pair of functions which
> >         are used to generate action-based bookmark menus.
> 
> I guess the intent here is to reuse these for popups ? Maybe we should
> keep the class, just make it more flexible ...

Yep. I may change that class to be a kind of factory for bookmark
actions and the bookmark menus (including popups). Basically it would be
a wrapper around an action group, and functions for building menus.

> >      3. Modify the topic action class to use this (which I presume would
> >         solve bug #148714).
> >      4. Modify the bookmark action and topic action classes to
> >         automatically destroy themselves if a bookmark/topic is deleted.
> 
> 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 ?

True. If the bookmarks menu class is kept, then what you suggest can be
achieved. At the moment (looking at what is in CVS) it looks like each
action already does listen for change events. I can remove that and
replace it with the more efficient system you suggested.

> >      5. Move the bookmarks_tree_changed_cb from the old bookmarks menu
> >         class into EphyWindow.
> 
> This is related to 2 ? I think the menu class autoupdating itself on
> tree changes is ok ...

Yes they are are related. If the menu class was removed then someone had
to receive the bookmarks_tree_changed signal (and EphyWindow was the
natural choice). If the menu class is kept, then it will autoupdate
itself and bookmarks_tree_changed_cb will stay out of EphyWindow.

> > The result should be a system which is much cleaner as the code to
> > generate all bookmarks menus (popups or otherwise) would become common.
> > If I was to develop such a patch would there be any interest in
> > accepting it? (Note: this is separate from my other bookmarks work, and
> > this patch would leave the user interface mostly unchanged).
> 
> I could definately use some help on this. Cleaning up the code and
> improving perf of bookmarks menu is one of the higher priority.
> 
> I dont fully understand your plans yet, let's keep discussing it ;)

I'm still sorting out some details as I code. I was initially trying to
do minimal changes, but I think in the end the resulting patch is going
to be huge. The only thing I can hope for is that the resulting code is
clean and simple (and therefore has less chance of containing bugs).

The way I envision the end product is:
      * A collection of bookmark and topic actions per window, which are
        used for the main menu, the popups on the toolbar, and the
        toolbar itself.
      * Bookmark and topic actions knowing which window they are
        targeting and handling their own events (rather than using
        signals). It seems to me that the design of Epiphany has matured
        to the point that bookmark actions can be standardised (ie. work
        the same in the menu, in the toolbar, or in popups), which makes
        signals unnecessary.
      * Possibly removing the bookmarks bar as a special toolbar class.
        Since bookmarks can be hosted on the normal toolbar I don't see
        the reason for this class.
      * A general cleanup of code which seems to have grown in many
        different ways over a period of time. There are functions which,
        as far as I can tell, are never ever called (though they
        obviously were at some point in time). There are also bugs (like
        the fact that you can't right-click on a bookmark which is on
        the normal toolbar, but you can if it is on the bookmarks bar).

I haven't started changing much of the toolbar code yet as I got lost in
it very easily. Removing the bookmarks bar for a start might make things
easier. Of course, removing the bookmarks bar would require a new way of
adding bookmarks to the toolbar (perhaps dragging from the bookmark
editor directly on to a toolbar, or adding a special "Add a bookmark"
icon from the toolbar editor?). As I don't know the EEL stuff very well
this could take a bit more time.

Hope I'm not reading on toes with these comments. I'm coming with no
idea on 'proper' Gnome/GTK programming, and am learning GTK as I go. I
could be talking total crap, and am willing to be told so. :)

Thanks,
Peter.

PS. Won't be responding to email for the weekend. Be back Monday
(Australian time).