Re: bug, speed problem in Bonobo relating to commands

[Darin Adler <darin eazel com>]

on 11/14/00 1:40 AM, Michael Meeks at michael helixcode com wrote:

>> Working on popups more, I discovered that I need to mark the commands
>> dirty, as well as the widgets. (Dirt is propagated from commands to
>> widgets, but not the other direction, so the dirtying in add_popup has
>> to do the commands.)
> 
> They dirty bit signifies something has changed. Why do you want to
> emulate a non-existant change ? There should be no need to dirty the
> commands at all, unless you change them. If they are change then any view
> using them needs to be dirtied. This is why we do things as they are
> done. If the view changes; then there is no need to dirty the model, since
> we only need to update that single view.

I agree.

Here's the problem (sorry I wasn't clear in my first message). When the
pop-up is first created, properties need to propogate from the command to
the newly introduced widget. For example, the sensitivity flag is not
propogated, so a menu item in the pop-up for an insensitive command is still
marked sensitive.

The problem is that when a menu item is built by the sync. machinery, it
doesn't reflect the state of its corresponding command until the command's
state is propogated into the item by update_cmd_state. Perhaps the best way
to fix this is to call update_cmd_state explicitly for these newly created
widgets.

This doesn't have to be fixed by marking the commands dirty. The idea of
marking things dirty to get the command state to propogate into the newly
created menu widgets is a hack (that worked) -- I thought I was following
suit with an existing hack, but I now realize I must have misunderstood.

>> Once I did that I discovered a pre-existing speed problem. Updating
>> the commands takes more than a second, close to 2 seconds, on my very
>> fast machine. The problem is that walking the whole tree looking for a
>> particular command ID is a very slow operation, and this is exactly
>> what update_cmd_state does.
> 
> Please show me what code you are writing, what bug you are trying
> to fix and how you are trying to fix it. How do you mark the commands
> dirty ?

I sent you a patch under separate cover that includes the command dirtying.
Didn't you receive it?

The patch marks the commands dirty as a workaround for the above problem. I
hope you can come up with a better way to fix it. Finding a fix for the bug
is issue A.

Issue B is the speed problem. If many commands are dirty (for any reason),
update_cmd_state is very slow, because it iterates the entire XML tree,
looking up the command ID on each node (which requires iterating all the
properties of the node), for each dirty command. In my case, the function
took 2 seconds to run. This is an O(num_dirty_commands *
num_properties_in_entire_tree) operation, which is quite large for the
Nautilus UI XML tree, for example.

>> Your thoughts? Do you want me to fix this? Do you want to fix it?
> 
> I would like to understand the problem that you are trying to
> address, can you describe it ? I am extremely willing to believe there is
> a bug in the popup code of some sort; but not that the core code needs
> re-engineering drasticaly.

There's no "core code" that needs "re-engineering drastically", but I'm
pretty sure that update_cmd_state needs to be changed to make it faster.
Currently this function makes opening new windows in Nautilus noticeably
slow as the menu bar for the window is set up.

There's a trivial way to speed update_cmd_state up with a hash table, and
I'll do that and send you patch for issue B unless you tell me otherwise.

I need to hear from you on issue A.

    -- Darin