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.69k stars 1.14k forks source link

dynamic preset shortcuts #6651

Closed elstoc closed 2 years ago

elstoc commented 3 years ago

Provide the ability to cycle through a module's presets with a keyboard shortcut (similar to how dynamic comboboxes work). This would be particularly useful for modules like the channel mixer, where multiple film emulation presets are available and it would be great to be able to trial them one at a time without having to reopen the presets menu each time.

Raising as a FR rather than a PR because I'm aware that @dterrahe is already doing a lot of work around shortcuts and I didn't want to conflict with any of that work.

dterrahe commented 3 years ago

Yep! Already on my list of to-maybe-dos! Along with the individual options from the multi-instance menu. Some of these could be done separately for each module (your example of presets) whereas others might be for the current module (somewhat similar to multi-instance). I think all the blending parameters fall in the later category.

After #6648, let's discuss which ones we want to put on the list for the input-layer overhaul and which to implement now (making the overhaul larger/more complex). One thing this highlights is that we have so many shortcuts and so few keys! I definitely feel a strong need for modes/pages (for different stages in processing; reusing keys for shortcuts for a different set of modules). next-gen stuff.

Specifically for presets (this FR); I don't think we currently have the concept of an "active" preset. We just apply it when selected. So to be able to cycle to the "next" one, we have to remember it somewhere. And does that then get invalidated if any of the params are changed? History could have the previous manual changes, but we don't want to create a new history item each time we step through the list of presets. This needs thought! (but would def be nice to have...)

elstoc commented 3 years ago

One thing this highlights is that we have so many shortcuts and so few keys!

You're right. I think most people only use a small number of the available shortcuts though - this is more about having flexibility. Being able to use multiple letter keys in combination would be awesome. For example I might have a dynamic shortcut for exposure/black level by holding E and B together and scrolling. Gtk probably doesn't support that though.

I don't think we currently have the concept of an "active" preset

If you click on the preset menu and one is currently enabled, it is highlighted in bold, but I'm not certain how that's done right now. Presumably we could reuse that logic.

And does that then get invalidated if any of the params are changed?

It does, but then I think it's fine to start from the top of the list again. In terms of the history, it should do exactly what it does now if you select the preset from the list - no need to be too smart about it.

It's all probably implementable now, as a standard shortcut, and could probably steal much of the pre-existing code from the presets popup menu. You could probably include some sort of 'cached' list of presets that's invalidated when you edit a preset or the module's parameters.

johnny-bit commented 3 years ago

Concept of active preset is determined for example in presets menu by comparing current params and blending with all modules on list and marking the one matching as "active"... Quite computationally intensive, eh? ;)

dterrahe commented 3 years ago

I think most people only use a small number of the available shortcuts though

out of necessity...

Being able to use multiple letter keys in combination would be awesome. Gtk probably doesn't support that though.

It doesn't. So either we'd have to reimplement an emacs mode-like keyboard handler ourselves or we could use a first keypress to change pages and the second as now, but with a page-specific config. Do all keyboards allow multi-key presses for all combinations (physically)? Gaming keyboards pride themselves on that, but other keyboards get ghosting effects, I think.

I don't think we currently have the concept of an "active" preset

If you click on the preset menu and one is currently enabled, it is highlighted in bold, but I'm not certain how that's done right now. Presumably we could reuse that logic.

I stand corrected! Should have quickly checked first before speaking. ;-)

And does that then get invalidated if any of the params are changed?

It does, but then I think it's fine to start from the top of the list again. In terms of the history, it should do exactly what it does now if you select the preset from the list - no need to be too smart about it.

No history item, but undo works, so that's good enough.

It's all probably implementable now, as a standard shortcut, and could probably steal much of the pre-existing code from the presets popup menu.

Will have a look when #6648 is in. (if) If these become "top-level" standard shortcuts, the list gets rather large for each module. Not necessarily a problem, but it is starting to become desirable to group the standard shortcuts together so they don't "hide" the module specific ones (or get hidden by them). Could be done with some hackery in current setup; attention point for NG.

Or maybe I am misunderstanding and you'd just want to have two (non-module specific) shortcuts that select the next or previous preset for the active module?

You could probably include some sort of 'cached' list of presets that's invalidated when you edit a preset or the module's parameters.

I'm not sure what you mean. I've only looked at the presets code enough to get thoroughly scared of the dragons there, so this is somewhat new territory.

dterrahe commented 3 years ago

Concept of active preset is determined for example in presets menu by comparing current params and blending with all modules on list and marking the one matching as "active"... Quite computationally intensive, eh? ;)

I don't dare look up that piece of code now. Is it very scary? Or just memcmps for params and blend_params? That would not be at all at the same level of bad as other stuff I've seen. Gets done each time or only when opening preset list? Anyway; should be reusable.

A memcmp on a curve that had a node added and then removed thinks the resulting garbage at the end makes it different...

Don't know anything about the curve editing code, but it would be nice if it zeroes everything after last active node. Also helps (a little) with showing the diffs in history. Might be code all over the place though.

elstoc commented 3 years ago

starting to become desirable to group the standard shortcuts together

Seems like a good idea. Or put all of the slider/combobox stuff in its own subsection

Or maybe I am misunderstanding and you'd just want to have two (non-module specific) shortcuts that select the next or previous preset for the active module?

No. I mean per-module - at the same level as the activate/expand/reset generic ones.

I'm not sure what you mean.

I mean afaik that list of presets in the presets popup is regenerated each time you display the popup, as is the decision as to which preset is currently active. Store that list, along with the currently active preset once just once against the module instance and only regenerate it where required (using some sort of stale indicator), or something. If you're just iterating through the list you shouldn't need to do all that computationally intensive stuff every time, unless something else has made the presets list invalid. Only regenerate the list if a user chooses to display or cycle through an invalid list.

johnny-bit commented 3 years ago

I don't dare look up that piece of code now. Is it very scary? Or just memcmps for params and blend_params?

Nah, just memcmp

Gets done each time or only when opening preset list?

On menu building.

dterrahe commented 3 years ago

"computationally intensive" has a different meaning when you do it for all modules on all gui updates vs for 1 module in the context of user interaction. Users are slow...

elstoc commented 3 years ago

Users are slow...

And when you consider the pipeline has to be regenerated when you change the preset anyway it's probably not all that significant.

dterrahe commented 3 years ago

On menu building.

If it is fast enough for menu building, it is fast enough for cycling.

github-actions[bot] commented 3 years ago

This issue did not get any activity in the past 30 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

github-actions[bot] commented 3 years ago

This issue did not get any activity in the past 30 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

github-actions[bot] commented 3 years ago

This issue did not get any activity in the past 30 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

Nilvus commented 2 years ago

@elstoc: I don't need this FR so didn't test. But as this is quite old now and NG input system by @dterrahe had been implemented few weeks ago, is this fixed or not by this new input system?

elstoc commented 2 years ago

No it's still on the to do list (#9487). It's non-trivial because it involves querying the database to get the current/previous/next values.