Wrong Custom Action on Toolbar In Certain Circumstances

Reproducer:

1. Create custom action with Directories selected on Appearance tab
2. Add custom action to toolbar
3. Edit custom action and uncheck Directories option
4. Notice that the icon on the toolbar changes to next item in the Configure Toolbar items list, even thought it might not have been previously selected - some variability here depending on position in list.

A little bit of an edge case.

Running latest git version of thunar.

changed the description

added 5. Plugin - Custom Actions (UCA) 5. Toolbar labels

This problem was briefly discussed here.

Thanks for reporting, I can reproduce the problem !

Will look into it soonish

After some code-reading, I think there is a structural problem with UCA's used in the toolbar. If the UCA's are added on the very right, things are fine. Though, if you swap position with the other toolbar-items, and you have hidden toolbar-items on the right of the UCA, the described effect will happen. (Some hidden toolbar items will be shown, some shown items will be hidden).

The problem is, that while loading the toolbar-items, we just get a list of all folder-uca's. There is no information or trigger that a specific one was removed (we only trigger on a change of UCA.xml). So we don't have the possibility to reorder the list accordingly.

After loading, thunar_window_location_toolbar_load_last_order will be called to sort all loaded toolbar-items according to the xfconf variable. We don't know which entry of the xfconf list should be ignored at that point. (Same for thunar_window_location_toolbar_load_visibility)

(@lastonestanding, I suppose you were right with pointing at the problem prior to 4.18 release, while we did not have a concrete reproducer at that time ... now you have all the right to say 'I told you so' :D )

In #179 (comment 24143) I mentioned some unique ID which possibly could be used to address the UCA's ... I'll check if that can be used.

Other possible solutions:

• Force the name of the UCA to be unique and use that as a unique key.
• Use a separate *.desktop file for each UCA, like suggested in #179 and use that as a unique key.

I don't think it will be possible to provide a proper fix for this issue for the 4.18 branch.

Bug can as well be triggered by deleting an UCA which was in the toolbar. (If it's position got modified)

Just faced a related bug after adding 25 toolbar items (#1240 (closed)) and removing them one by one afterward. Thunar crashed at thunar_window_location_toolbar_createand won't start anymore.

Needed to reset both xfconf thunar toolbar settings to get it to run again. --> This issue is a must-have for 4.20

changed milestone to %Xfce 4.20

mentioned in merge request !443 (merged)

mentioned in issue #1338 (closed)

Here is a WIP branch: https://gitlab.xfce.org/lastonestanding/thunar/-/tree/wip-1028

• Toolbar items now have an id and their order plus visibility is stored in a single xfconf property.
• The code to read the old properties is still present and being used to migrate the saved configuration.
• The migration code is highly experimental and does not resolve shifting toolbar items (like introduced by !443 (merged))

Great that you are taking care on this! The issue was as well on my agenda, though I got continuously distracted by other things.

Using the last segment of the accel-path as id is a nice idea. For the UCA's, the used "name" property is build by using the unique-id

On the first start the "configure toolbar" dialog opens for me … is that intentional?

The migration code is highly experimental and does not resolve shifting toolbar items (like introduced by !443 (merged))

I suppose we cannot do much about shifting items when reading the old xfconf setting during migration. At least it will not be a problem anymore with the new format.

On the first start the "configure toolbar" dialog opens for me … is that intentional?

It is part of the experimental migration code. Currently, only opening and closing the Toolbar Editor will trigger a save of the toolbar configuration (and create the new xfconf property). Otherwise, Thunar will load the default configuration on the next start instead.

That part needs to be implemented better so that it does not require any user action. Either by moving code around or by automatically closing the Editor dialog (easier, but a hacky solution).

Ah, I see, thanks for the details!

Since that will be a one-time action, and the code will only b there for 1-2 mayor versions, a hacky solution would be fine for me.

Moving the code to some other location sounds not that good, since in the future, when we don't need the migration anymore, it would be required to move it back to its old location again.

Currently, thunar_show_toolbar_editor creates the dialog, does some other stuff and runs the dialog. Maybe we can decouple that a bit to have thunar_create_toolbar_editor which only creates and returns the widget, and the user can optionally call gtk_dialog_run or thunar_toolbar_editor_save_model on it. Possibly a constructor argument, migration-only might make sense in that case.

I have added thunar_save_toolbar_configuration which kind of does the job without changing any existing code.

Another thing that I was not too sure about are the old xfconf properties. Currently, they are unset as part of the migration code, but maybe they should stay untouched instead. The deletion indicates a successful migration though.

I have added thunar_save_toolbar_configuration which kind of does the job without changing any existing code.

Sounds good to me!

Another thing that I was not too sure about are the old xfconf properties. Currently, they are unset as part of the migration code, but maybe they should stay untouched instead. The deletion indicates a successful migration though.

Hmm, for people which just want to peek into the new thunar/xfce version, it indeed might be better to keep the old property. Though not sure if that is a realistic use-case.

So overall it might be more failsafe to keep the old setting.

How do we save the migration status in that case (so that we do not keep migrating the old settings on every start)?

Maybe in a separate xfconf property, or added to the new property as prefix?

Maybe use xfconf_channel_has_property to check if the new "last-toolbar-items" prop is available .. hmm, that might require a new method thunar_preferences_has_property. At least that feels the least hackish.

Added that and also some experimental changes:

• New default order of the toolbar items.
  • Icon/List/Compact View buttons enabled on the far right (basically copying Nemo).
• Migration path will use the 4.18 default order to make updating from version 4.18 to 4.20 a smooth transition.
  • Adding a path for 4.19.1+ (two new buttons) may be possible, but how could we detect that?

New default order of the toolbar items.

Fine for me

Migration path will use the 4.18 default order to make updating from version 4.18 to 4.20 a smooth transition.

I don't understand ... if we are migrating the old setting, there already will be an order. What has migration to do with the new default?

Adding a path for 4.19.1+ (two new buttons) may be possible, but how could we detect that?

!443 (merged) was not yet released. One reason why I am waiting with the 4.19.3 thunar release, is this issue (the others are #1357 (closed) and #1330 (closed)). So I guess you don't need to consider configurations which already have these two extra buttons ... these are only used by a handful of people which compile from source.

I don't understand ... if we are migrating the old setting, there already will be an order. What has migration to do with the new default?

Ah, my bad. I somehow got the date for the linked MR mixed up.

I guess it is now time to clean up the code and open a merge request.

mentioned in merge request !465 (merged)

closed with merge request !465 (merged)

changed milestone to %4.19.3