Hi again, and excuse me for stuffing the list.
Actually there is 1/2 error. The plugin toolbar items are inserted improperly, but added to plugin_items list in the right order. So using "Customize Toolbar" and adding/removing items or otherwise changing them fixes the order.
Patch attached. Not in git format, sorry.
Hi,
Le 29/04/2012 20:26, Dimitar Zhekov a écrit :
Hi again, and excuse me for stuffing the list.
Actually there is 1/2 error. The plugin toolbar items are inserted improperly, but added to plugin_items list in the right order. So using "Customize Toolbar" and adding/removing items or otherwise changing them fixes the order.
Patch attached. Not in git format, sorry.
If I read the thing correctly, the patch is wrong because it would possibly mixup tool items from different plugins if they aren't added at the same time, wouldn't it?
But you're right that there is a problem. Currently, it creates:
| Plugin_1_Item_2 Plugin_1_Item_1 | Plugin_2_Item_1 | Quit
and should create
| Plugin_1_Item_1 Plugin_1_Item_2 | Plugin_2_Item_1 | Quit
However with your patch, if plugins are added in the order Plugin_1_Item_1, Plugin_2_Item_1, Plugin_1_Item_2, it would give:
| Plugin_1_Item_1 | Plugin_2_Item_1 Plugin_1_Item_2 | Quit
Which is also wrong (more wrong if I could say).
Getting that right seems a bit harder and would need to be able to know what's the last item added by a given plugin. Maybe tagging the widget with the plugin it belongs to, and then search for the first non-matching one would do the trick:
def get_insert_position(plugin): pos = toolbar.get_default_insert_pos()
if plugin.autosep: # find the last item belonging to @plugin while pos < toolbar.get_n_items(): item = toolbar.get_item(pos) if item.get_data("plugin") != plugin: break
return pos
def add_item(plugin, item): pos = get_insert_pos(plugin)
if not plugin.autosep: plugin.autosep = create_sep() toolbar.insert(plugin.autosep, pos) pos += 1
item.set_data("plugin", plugin) toolbar.insert(item, pos)
Maintaining an index don't seem really a good idea since it would be one another thing to keep, and I don't think that adding a tool item is a performance-critical thing so the possible overhead finding the position (if there is already an item) should not be a problem.
Thoughts?
Cheers, Colomban
On Wed, 09 May 2012 00:19:15 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 29/04/2012 20:26, Dimitar Zhekov a écrit :
Actually there is 1/2 error. The plugin toolbar items are inserted improperly, but added to plugin_items list in the right order. So using "Customize Toolbar" and adding/removing items or otherwise changing them fixes the order.
Patch attached. Not in git format, sorry.
If I read the thing correctly, the patch is wrong because it would possibly mixup tool items from different plugins if they aren't added at the same time, wouldn't it?
Yes. But Geany does not really support mixed add order: the items are put in toolbar.c's plugin_items exactly in their add_toolbar_item order, so a recreation of the toolbar will mix them if not added at once.
BTW, I placed a git patch in tracker bug item 3522755.
But you're right that there is a problem. Currently, it creates:
| Plugin_1_Item_2 Plugin_1_Item_1 | Plugin_2_Item_1 | Quit
It creates Plugin_1_Item_2..N | Plugin_1_Item_1 for each plugin, so:
Plugin_1_Item_2 | Plugin_1_Item_1 | Plugin_2_Item_1 | Quit
And for 2 plugins and 2 items:
Plugin_1_Item_2 | Plugin_1_Item_1 Plugin_2_Item_2 | Plugin_2_Item_1 | Quit.
However with your patch, if plugins are added in the order Plugin_1_Item_1, Plugin_2_Item_1, Plugin_1_Item_2, it would give:
| Plugin_1_Item_1 | Plugin_2_Item_1 Plugin_1_Item_2 | Quit
Which is also wrong (more wrong if I could say).
With the patch, we have a proper order if the plugins add their 2+ items at once, and that order matches toolbar.c plugin_items.
With the current code, the order of 2+ items is always wrong. They are kept together, allright, unless the toolbar is rebuilt, but the separators are wrong too.
Hi,
I looked into plugin toolbar support code in more detail, and there seem to be three simple ways to fix the item order:
1. Apply the current patch and tell the plugin authors to add/remove their items at once. They are likely to do so in the first place, because mixed adding/removing means one must keep track of the items, to be able to remove them on plugin_cleanup. It's easier to add everything at once, show/hide as needed, and remove at once. A not shown / hidden item is just as good as not added / removed item.
2. Add an item_count to GeanyAutoSeparator. We already have the means required to track items, but ref_count is for show/hide. Having item count has the advantage of being able to destroy autosep->widget when it drops to 0. ref_count should be renamed to show_count or something.
3. Add a hidden separator item after autosep and always insert before it. Will work like the current positioning, except the 1st item will be OK. May be easier/simpler to implement than #2, but I don't like it.
Related to #1, note that mixed removing of items, too, is not supported well by Geany - if you destroy a never shown / hidden item, ref_count will be decremented anyway and autosep may be hidden even if visible items exist.
--
On Wed, 09 May 2012 00:19:15 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
def get_insert_position(plugin): [...]
def add_item(plugin, item): [...]
It will work, of course, but I think the above are simpler.