Re: Required egg-editable-toolbar changes for extensions

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

Hi,

On Tue, 2004-05-25 at 05:01, Adam Hooper wrote:
> Desired UI:
> 
> Right now, the toolbar has a fixed set of actions and toolbar buttons
> associated with them. They are all loaded when Epiphany starts. We want
> to add/remove custom buttons at any time extensions are loaded/unloaded
> (at any time during normal operation). However, the user doesn't want to
> *lose* those custom buttons forever when the extension providing the
> button is unavailable. Imagine the following scenario:
> 
> 1. User loads Epiphany and customizes his toolbar
> 2. User loads "Foo" extension
> 3. User adds "Bar" button (provided by "Foo" extenison) to his toolbar
> 4. User unloads "Foo" extension
> 5. User loads "Foo" extension
> 
> The button should show up in the exact same place on step 5 as it did on
> step 3. This is imperative, since steps 4 and 5 will occur every time
> Epiphany is restarted, and the toolbar must look the same across browser
> restarts.
> 
> Conclusion: the XML file epiphany-toolbar.xml must store toolbar buttons
> which are in non-loaded extensions.
I completely agree.

> Code
> 
> EggEditableToolbar already does pretty much all of what we need.
> EggToolbarsModel lets us add items with nothing more than a name (the
> GtkAction is only part of the actual GUI portion of the code).
> 
> First, loading the XML. In egg-toolbars-model.c, in parse_item_list()
> (which will get called on startup, conceivably before extensions are
> loaded [1]), there is a call to egg_toolbars_model_get_item_id(). It
> will simply return a copy of the passed 'name' whether a valid action or
> not. Would it be "good form" to load an id and name into the toolbars
> model which may not even have an associated action for the duration of
> the model's existence? If so, then the EggToolbarsModel will load fine
> with no changes, as far as I can see.
> Next, the toolbar will be constructed. This is in
> egg-editable-toolbar.c, egg_editable_toolbar_construct(). Halfway down,
> there is an "item = create_item (t, model, i, l, &action);".
> create_item() in turn calls create_item_from_action() (with the action
> name from the EggToolbarsModel), which will call find_action() with that
> name. This will return NULL (since the action hasn't been created yet).
> So the action will be removed from the EggToolbarsModel.
> 
> My proposed workaround is to simply *not* remove the action from the
> EggToolbarsModel. Any time an extension is loaded/unloaded, call
> egg_editable_toolbar_construct() again. Also, the EggToolbarEditor would
> have to reload its icons when extensions are loaded/unloaded.
> egg_editable_toolbar_construct() would need another temp variable to
> keep track of position and a few lines would need to be deleted. Problem
> solved.
> 
> Would this workaround break anything? Would it be considered "bad"?
> 
> After that, we'd need to change egg-toolbar-editor.c's
> parse_item_list(). egg_toolbar_editor_load_actions() could take a "const
> GtkUIManager *" instead of a "const char *xml_file". Instead of looking
> for "<available>" it could instead look for a '<toolbar
> name="AvailableToolbarButtons">' or something in epiphany-ui.xml. In the
> interests of abstraction, this may mean extending EggToolbarEditor for
> use in Epiphany.
I think it would be simpler to just extend the EphyExtension interface
with a method to insert available actions into the tbe.

> Would these changes be all we need? I can't think of what I'm
> forgetting; it seems to me like this would work!
What you have so far seems enough for extensions to put their own
buttons on the toolbar.  What's missing is extensibility of the toolbars
model itself to new types of items; we'll need this when we proceed with
the plan to make bookmarks pluggable. EphyToolbarsModel does this now by
providing support for topics and bookmarks items.
Simply making the EggToolbarsModel vfuncs get_item_[id|type|name] into
signals to which extensions can hook up to should allow this.

> On second thought, recreating toolbars for every extension load/unload
> may be too much. But it would be straightforward to optimize this.
Yes.

Regards,
	Christian