Bonobo UI code feedback

[Darin Adler <darin eazel com>]

Hi Michael. Here are my current thoughts on the Bonobo UI code. I posted
them here in the interest of greater openness, but we can continue this
discussion either privately or publicly.

================

The top priority for the Nautilus team (and for Eazel) is to get a suitable
Bonobo release for our PR2 release. Beyond what's in PR2, the main things we
need are fixes for:

    1) The problem that you, Maciej, and Martin discussed (way to make the
UI container work properly with our component adapter), which is covered by
our bugzilla.eazel.com bug 3691.
    2) Translatoin of text in XML files. Maybe this won't require any Bonobo
changes, but I'm still not 100% clear how to handle it in Nautilus; I'd like
some "exemplary" module to copy.
    3) The bug that's in bugzilla.eazel.com bug 3705. Some of our bookmark
icons aren't showing up and we need to figure out if it's a bug in Nautilus
or in Bonobo.

If we can get these three resolved, and get another Bonobo release, then I
think we're OK for the Nautilus PR2 release.

================

I'm on the record saying that I think it was late to make such a big change
to Bonobo. Here's a big part of the reason. These are my thoughts on the
current Bonobo UI API given a few weeks experience with it. I think that
addressing all of these problems is far too much work to be undertaken for
Gnome 1.4, and I'd suggest doing it for Gnome 2.0 instead. But as I
understand it, Miguel wants to freeze the Bonobo API between Gnome 1.4 and
Gnome 2.0, so perhaps we just can't address these.

================

API thoughts (big and small):

*_get_remote_ui_container is not a good function name. In the old days we
had to distinguish the remote UI handler from the local one. Now there is
only one UI container, so this should be named *_get_ui_container. The
"remote" is just confusing now.

BonoboWin is not a good class name. It should be BonoboWindow to be
consistent with all the other class names in Bonobo and Gtk.

BonoboControl automatically creates a BonoboUIComponent. But it does not
connect it to the BonoboUIContainer. It should do that work inside
bonobo_control_set_control_frame. Controls with fancier UI requirements
could just use their own BonoboUIComponent. This is tied to the strange
automerge feature. This area needs to be cleaned up. It's unnecessarily
confusing.

The calls in Bonobo*Frame interfaces are still called "get_ui_handler" but
should be called "get_ui_container".

Using strings for the property names in calls like *_set_prop may be handy
for the implementation, but it's a nightmare for clients. It means that many
errors that should be caught at compile time instead aren't seen until
runtime. We've seen demonstrations of how error-prone programming this way
is, for example with all the runtime warnings about "command/widget
separation". If there were at least covers for common use, the API would be
a lot easier to use correctly.

The same problem applies for using strings for property values. It's easy to
get the wrong result with a typo, rather than getting a compile that fails.
This is a problem for future expansion, because idiosyncrasies about how
strange strings are handled may be coded into applications without the
programmer realizing it.

Allowing and ignoring any unknown properties in XML and from set_prop calls
creates a bad compatibility situation. If people have wrongly named
properties, these sit dormant until a new Bonobo comes out that uses that
property name. A program can easily have a dormant bug that gets broken by
new API that's added. This is similar to a C program that has a name
conflict with some new code because it intrudes on a library's name space,
but it's even more likely. Imagine if you could accidentally call the wrong
function, and it would just compile and work now, but fail later at runtime
when a new "backward compatible" Bonobo release was done.

Our XML files have no DTD (not even a theoretical path) and don't use
namespaces. XML experts have told me that both of these are bad ideas.

The XML parsing is incredibly loose. In one case we had a bunch of junk in
there and the parser quietly accepted it and just created a strange blank
toolbar item that no one noticed for a while. If we had a DTD, at least we
could find some way to compile the XML files with a validating parser as a
part of builds.

It's hard to tell if you pass the wrong datadir to the set_ui family of
calls. There should be some obvious way to tell if you did it right or
wrong. I had it wrong in Nautilus, and the failure was something that only
happened in strange circumstances -- it should instead be caught right away
in some simple fashion.

The gnome-ui-compat.[ch] files and gnome-ui-handler.h should be removed
before the 1.0 release. There is very little code left using it, and we can
get that down to nothing very easily.

All GdkPixbufs that are used in widgets by putting the pixbufs themselves in
the XML use separate copies. This is particularly bad for things like the
"bookmark" menu, where there are tons of items, many with the same pixbuf (a
globe for the world, say), but the pixbuf isn't a stock one. Passing pixbufs
encoded as text means this is part of the API, not just the implementation.
One good solution for this might be to have separate named image objects and
refer to them by name. You can send across a complete pixbuf and name it,
then use it by name. Other possibilities might include matching the pixbuf
on the receiving side somehow (some kind of digest) to avoid sending all the
data across again.

There's no convenient API for making a series of potential changes quickly
(without performance problems). This is vital for the typical case of
updating all the sensitivities and labels of menu and toolbar items. There's
no way to avoid the CORBA call when setting an item back to the same value,
since nothing on the client side currently caches any state.

The current command mechanism is being used both as a way to bind a callback
with a particular item, and a way to share properties on related items (ones
that represent the same "command"). Unfortunately, the set of shared
properties is hard-wired. This means that you either have to share exactly
what the command system says you share, or you need to use separate
commands. This makes it hard to deal with cases where items are similar, but
not identical. With the current design, if the items share enough you can
use one command, if not you have to have multiple commands. The classic
example is the toolbar button that is used to toggle and the related toggle
menu item. I believe this is being currently discussed in the Gnome
components mailing list, although the discussion seems a bit garbled.

If would be *much* better if the C code could deal with all items separate
from location; it's a mistake to hardwire location ("path") into the API.
The commands are not specific to location, but the UI items themselves
should also have names that are completely separate from location. The only
time you should have to refer to the "path" for an item is when creating it,
and even then you should only have to mention the name of the enclosing
item. Without this feature, you can't rearrange your menus without also
editing your C code, and one of the main benefits of doing things with the
XML file is lost. Coming up with unique names is no problem, we already do
that for commands, and we can use "path" schemes of our own as necessary.

Because the C code is tied to widget location, the customization users can
do by replacing *-ui.xml files is greatly limited.

The *-ui.xml files should be named *.bonobo-ui or *.gnome-ui instead,
according to Maciej. He says that we shouldn't use the ".xml" extension --
we would not call C code files ".c.txt". Prior art: "*.oafinfo",
"*.gnumeric", "*.svg", and "*.html" (for XHTML).

The wildcard "/*" feature for deleting a set of items has a problem. It only
works when the items are all widgets with a common parent. You typically
have to walk through all the items anyway to remove the commands so the "/*"
feature doesn't work. The wildcard should work on command names as well as
item names, based on any string prefix, and the hierarchy-based one should
just go (since we want to deal with items by name anyway, not path).

It would be nice to have an XML way to specify at least the simplest
commonest cases of dynamic labels. For example, an item with two different
labels. Since this could never be complete, perhaps it's OK to just fall
back on C code whenever things need to be dynamic.

We'd like to use #defines in XML would be nice, to share code with C. But
it's not clear if this is compatible with the localization scheme. Ideally,
we'd use this in conjunction with global non-path names so the C code and
the XML code could use the same constant for the name of each menu item,
command, etc.

Radio item menus are missing the convenient interface that we have for
command-type menus. They shouldn't require the explicit signal handler the
way they do.

Do toggle items have a convenient-to-use interface like the verb ones?

Lots of things are flashing. Is this easily fixable without API change?

Functoins that take ev parameters that can be NULL are potentially
confusing. Functions need to document whether they can fail or not. If they
can fail, then ignoring that failure with NULL is rarely the right thing. If
they can't fail, then the ev parameter is just wastage.

================

Some of these might just be misunderstanding on my part. Let me know if you
want to discuss any of them.

    -- Darin