darktable-org / darktable

darktable is an open source photography workflow application and raw developer
https://www.darktable.org
GNU General Public License v3.0
9.77k stars 1.14k forks source link

Accelerators are only enabled in darkroom when their corresponding module is not hidden #4572

Closed dterrahe closed 4 years ago

dterrahe commented 4 years ago

Keyboard shortcut/(dynamic) accelerators are only enabled for iop modules in darkroom that are currently not hidden. This means that after setting up a shortcut in the preferences dialog a (novice) user may be surprised to find that it does not immediately work. Only after asking around (I've seen a few of such discussions) is it pointed out to them that they also need to unhide the module in the module list. One of the advantages of using shortcuts is that the darkroom can be used without having the module list visible, so all of this is not always immediately obvious.

Even when a module is actually enabled and used for a particular image (maybe it was found via search or another subset of modules was temporarily selected) the shortcuts might still not work if the module isn't currently "unhidden".

I'm not sure what the advantage is of disabling accelerators for hidden modules. It doesn't completely "disable" the module; it is still searchable and can still be activated individually or by changing subsets. Is "hiding" intended to have more far reaching consequences than just decluttering the module groups? Then that should be fully enforced. For me, having an accelerator set up means I expect to always have instant access to that feature. My preference would be to strip out the functionality of (temporarily) disabling accelerators.

If this for some reason is not desirable, it would be nice to give some feedback on what the problem is. A popup/toast saying "Module such and such is currently hidden" whenever an associated shortcut is pressed. But I don't think this would be easy to implement.

I'm happy to do the work on this but would first like to reach agreement on how we expect the UI to behave in a predictable manner. Any thoughts?

johnny-bit commented 4 years ago

I think the reason isn't that the shortcut is disabled for hidden module. It might be because hidden module isn't loaded and thus not initialized and doesn't have accelerators connected.

dterrahe commented 4 years ago

Looking at dt_iop_so_gui_set_state() in imageop.c, changing the hidden state of a module only connects or disconnects the accelerators. It doesn't load or unload the module. If the module is actually used for an image, it definitely still is loaded, even when hidden, but the accelerators are disconnected nevertheless.

My question wasn't really what may have been the (historical) reasons that accelerators are disabled for hidden modules, but whether that is actually what should happen, given that it leads to confusing behavior and that accelerators that were explicitly set up may stop working just because the subset of selected modules is (temporarily) changed.

elstoc commented 4 years ago

Now that you can search by module (including the hidden ones) the concept of a 'hidden' module does seem a bit odd. It means that it's still possible to enable those modules and modify them, even while not having shortcuts allocated to them.

Assuming that the sliders are still accessible (there aren't any of the initialization issues that @johnny-bit suggests) there would still be potential issues with some of the global shortcuts. I'm thinking specifically of the 'show/hide' shortcuts and wondering which group they should end up in when you showed them.

I suppose one way to reconcile these issues might be to create another module group, into which you place all of the 'hidden' modules.

Either way it might be easier to wait until after #4424 is merged because I've centralised some of that logic.

johnny-bit commented 4 years ago

but whether that is actually what should happen

IMO? No. It's unexpected alright.

dterrahe commented 4 years ago

Thanks @elstoc I'll wait for #4424 to land and then have another look. If you haven't implemented dynamic (mouse wheel) accelerators for combo boxes yet, I might have a look at those at the same time. And possibly #4570 too.

Obviously I would love to hear the perspective of @TurboGit, @AlicVB and @houz as well!

elstoc commented 4 years ago

See #4581 for implementation of dynamic combobox accelerators.