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

Option on how (and if) an instance label is changed #13982

Closed EC1000 closed 1 year ago

EC1000 commented 1 year ago

Is your feature request related to a problem? Please describe. https://github.com/darktable-org/darktable/pull/13149 by @TurboGit added an automatic renaming of instance label based on preset name. Having some automatic instance label is great; however I have a few issues about how it currently works:

  1. Because it reset the label when a parameter is changed, it breaks how I use multiple instances in conjunction with copy/paste (I wrote a comment in the initial PR, but I had not test it at the time and probably was not clear enough! I will try to explain better here);

  2. I like to set an instance label to reflect the purpose of the instance. However, some preset names make bad instance labels (for example because they are too long), plus the automatic resseting prevents me from using a preset and keeping the name/purpose if you change any parameter.

Here is an example:

Describe the solution you'd like I would like to suggest two options in preset definition, to improve the use case above:

Thanks for all the great work

TurboGit commented 1 year ago

You didn't talked about one easy way and why it is not OK for your workflow:

Isn't this sufficient?

  • add an option "define user instance label": off by default (the instance label would be the preset name as it works now), and if on a field called "instance label" can be defined: it would then be used to define the instance label instead of using the preset name.

This is not a good option to me, it will makes the UI code even more encumbered.

EC1000 commented 1 year ago

I forgot to write that these are auto-applied presets (I updated the example in the initial post to reflect this). So I think your suggestion does not work (or I did not understand it clearly).

Even for manually applied presets, it still a additional step to defined a manual label, so it is not as practical as an label that would just remain unchanged when a parameter is changed.

TurboGit commented 1 year ago

Indeed what I said do not work for auto-apply presets. That being said I see no good solution for now. Of course your second point could work for you "add an option "considered automatic label as if hand edited"... But I'm not sure it will be a generic solution. I'm not opposed to that, but I'd like to avoid another option. Let's see if we have more users voting for this.

TurboGit commented 1 year ago

In fact maybe the label of auto-applied preset should always be tagged as hand-edited? My naming for the auto-applied preset is often "default", "auto"... So having them stable would work for me.

@EC1000 : What's your naming scheme for auto-applied presets?

elstoc commented 1 year ago

I find that the auto names clutter the interface without (for me) really adding anything useful. I prefer not to have instance names on the first instance of a module, and mostly I'm fine with 1,2,3 as the instance names for the rest.

EC1000 commented 1 year ago

@EC1000 : What's your naming scheme for auto-applied presets?

Well, in fact it depends a little bit on how many presets I use for a module:

  1. something like "default" if it is just one auto-applied preset for all images.
  2. "some purpose" when there is multiple instances (see example above)
  3. more often, something with parameters and/or conditions for auto-presets (something like "Denoise Canon 40D ISO 800-1600 (1.2;1)").

In case of multiple instances (2.), having an automatic instance label really brings value, because otherwise it is hard to tell which instance does what. For a mono instance (1. and 3.), I find that using the preset name as an instance label does not add valuable information (if I just want to check where the current settings come from, I click on preset menu to see if a preset is in bold).

That is why in my initial proposition, I separated the different settings, so you could have different behaviors depending user preferences. But it indeed ends up being cumbersome!! :-1:

Another proposition, which I find lighter and better:

@TurboGit: do you have use cases where this would not work? How do you find this solution?

My evaluation:

elstoc commented 1 year ago

add only a new "instance label" field in preset definition. Empty, it would behave as before (empty label or 1, 2...). If set, it would be use to define the instance label.

I think that sounds like a neat solution, though for hard-coded presets you'd have to create a copy in order to force the instance label (you already need to do this if you want to auto-apply a hard-coded preset so I don't have a problem with this).

TurboGit commented 1 year ago

How do you find this solution?

This doesn't sound really good to me. When creating a preset we already have a name and a description field. Now we would have a third one that will be the label used for modules. This is adding complexity as we need to deal with this new field. I would not even fill easy to document this new field, and one will ask "why not use the preset name for the label".

For me what is still not clear is why you want to avoid the preset name as module's label for auto-preset. The motivation to add this was precisely to make it clear which preset has been applied automatically. Remember a preset can be applied conditionally based on image metadata, so it seems important to me when you open a new image to directly see which presets has been applied.

elstoc commented 1 year ago

For me what is still not clear is why you want to avoid the preset name as module's label for auto-preset.

For me, it makes the module view look cluttered and harder to read. If I name an instance it's for a reason and because I want it to be easy to spot that module in the sea of other modules. If a lot of the modules include descriptions that I don't care about (and they will mostly be ellipsized and so harder to read) I have to do more reading, when I used to be able to easily pick the named module out.

I have a bunch of auto presets that apply to every image (and indeed some of the hardcoded presets apply to every image). Including the preset name doesn't add anything for me and it makes it all look worse (for want of a more constructive word).

TurboGit commented 1 year ago

So maybe, instead of going to a hard to maintain change we can have a preference as I'm not we won't be able to please everybody on this one I feel. The preference would be "do not update the module's label, let me do it myself."

For me, it makes the module view look cluttered and harder to read.

And that's better to you than having a set of auto-preset applied without label on your picture so without knowing which one was applied? Really? I can understand, but this feels so wrong to me...

elstoc commented 1 year ago

And that's better to you than having a set of auto-preset applied without label on your picture so without knowing which one was applied?

I don't use a lot of presets. Where I do use them, they're usually either auto-defaults that always apply to every image or a starting point for further adjustment. Once I've chosen a preset I usually don't care what it was afterwards. If I really want to know what preset I used, right-clicking on the header is no effort, or I can name it. But I'd rather not be forced into having a label that I don't need 99% of the time.

The preference would be "do not update the module's label, let me do it myself."

I still prefer the idea of doing it as part of the preset. You could even have it as a tick-box "use this as the default instance name". I'm not the biggest fan of extra preferences (they're usually code for "we couldn't decide") but I'd prefer it than having to run my own private fork of darktable with personal tweaks (which is my other alternative for this)

ptilopteri commented 1 year ago

perhaps best would be option in preset to label or not and automagical or not

EC1000 commented 1 year ago

For me what is still not clear is why you want to avoid the preset name as module's label for auto-preset.

I completely agree with elstoc's answer https://github.com/darktable-org/darktable/issues/13982#issuecomment-1478063995 Mostly, it would not bring valuable info ("default") and can be very long ("wavelets 40D n°0 (0.5 chroma) - ISO 100").

The motivation to add this was precisely to make it clear which preset has been applied automatically.

Since it was in the same PR as multiple auto-presets, I must say that I initially though the main objective was to differentiate between these multiple instances! Do you often need to check which preset was applied? Whenever I need to check this, I just click on the preset menu and check if the currently applied parameters corresponds to a preset. image Isn't it sufficient?

Remember a preset can be applied conditionally based on image metadata, so it seems important to me when you open a new image to directly see which presets has been applied.

If this is important to you, I understand why you implemented it the way it currently works. But I feel it is diverting a function from its original purpose. For me, instance label is to (optionnaly) describe what a particular instance does (or sometimes why); while here it is becoming "how was it set".

EC1000 commented 1 year ago

Another proposition, which I find lighter and better:

  • remove the automatic instance label based on preset name,
  • add only a new "instance label" field in preset definition. Empty, it would behave as before (empty label or 1, 2...). If set, it would be use to define the instance label.

Also, I forgot:

TurboGit commented 1 year ago
  • remove "reset iop label if some controls are changed".

This will be wrong, you'll end up with a label with has nothing to do with the actual parameters.

TurboGit commented 1 year ago

Mostly, it would not bring valuable info ("default") and can be very long ("wavelets 40D n°0 (0.5 chroma) - ISO 100").

But you can at least rename them manually if it is important to you. But the other way around doesn't work, if there is no label it is hard to know what is applied. So until we find a solution that suit all parties it will be hard to progress.

EC1000 commented 1 year ago
  • remove "reset iop label if some controls are changed".

This will be wrong, you'll end up with a label with has nothing to do with the actual parameters.

Well, wrong if you want to have "how was is set" as a label. But with labels that describe the purpose of the instance, it would stay true. To keep the same example with denoise & sharpen: the "local contrast" instance would keep the same name if I add a few iterations. That is indeed what I would like! Parameters have changed, but the purpose of this instance is still the same.

EC1000 commented 1 year ago

But you can at least rename them manually if it is important to you.

  1. Move the mouse to "multiple instance actions" menu
  2. Click to open
  3. Move the mouse to "rename"
  4. Click to edit
  5. Enter new name with keybord and validate (Enter key)

But the other way around doesn't work, if there is no label it is hard to know what is applied.

  1. Move the mouse to "presets" menu
  2. Click to see the currently used preset
  3. Click to close the menu

My count is not at all rigorous :smile: It just would not say it is not possible to know what was applied even without label!

So until we find a solution that suit all parties it will be hard to progress.

Before PR #13149, did you already used instance label to described how it was set? Do you always and only use it for this? Did I understood and summarized correctly your main objective when I wrote that you want a label that reflect "how it was set" more than "what it does/why"? Thank you for your answer, I would like to be sure to understand your use cases before suggesting any new solution!

TurboGit commented 1 year ago

Ok, what about this as new feature:

So this way you can control the label of the preset independently of the preset name.

What do you think?

TurboGit commented 1 year ago

@EC1000 : The latest PR propose a solution to your problem. That is, you can now control the module label as you've asked. Instead of adding a new field or a check in the GUI dialog, the current module label is used. In fact, the module label was already stored (I did that when working on support for module label) into the preset table. But this was not used anywhere. That's in fact a sort of missing support or bug now fixed in my PR.

So if you enter an empty string as the module's label creating a preset (or updating an existing one) for this module will record this empty string (hand-edited). And applying this preset will never show a label until it is again hand edited if needed. If not label is entered, a preset will behave as today changing dynamically depending on params. If a text label is entered it will be used as it is today, the label will be set with the preset and will be always displayed.

Hopefully this will make everybody happy with nothing new in the GUI.

elstoc commented 1 year ago

Ok I see how this would work for user-generated presets. How does this work with the hardcoded presets? And to create a preset without auto-naming I need to edit the module label but not actually enter anything before I create the preset?

TurboGit commented 1 year ago

How does this work with the hardcoded presets?

That's a good question, the possibilities are:

  1. Do nothing, as in master the preset name is used as module label (if this is an issue people can create a preset based on it with a proper name).
  2. Add a multi_name for all the hard-coded preset, a kind of small aliases to differentiates them when applied
  3. Add an empty multi_name for the hard-coded preset to disable the naming of module when preset is applied

I'm ok with 1 & 2, I'll against 3 which will break my workflow.

I need to edit the module label but not actually enter anything before I create the preset?

You need to enter at least a space. There is no other way to differentiate between a module without label (nothing entered - clear the hand-generated flag so will be dynamically updated) and an hand-generated empty module label (at least a space - some content, hand-generated and not updated). Not ideal, but working.

EC1000 commented 1 year ago

@EC1000 : The latest PR propose a solution to your problem.

It makes sense to use the module label as a source to define... the module label! At least, it is consistent with styles, which also store module labels.
I will try to test the new PR as soon as possible and report here.

elstoc commented 1 year ago

Add an empty multi_name for the hard-coded preset to disable the naming of module when preset is applied

Yes, this. Because otherwise I have to duplicate every preset manually to get rid of it

You need to enter at least a space.

I'm sorry but this just sounds like bad UX. With my dtdocs hat on I feel it'll be really hard to explain clearly

TurboGit commented 1 year ago

I'm sorry but this just sounds like bad UX. With my dtdocs hat on I feel it'll be really hard to explain clearly

We can add an entry into the instance menu (we already have rename) to state that we don't want to use a name.

"clear name" or "unnamed instance" or ...

TurboGit commented 1 year ago

Yes, this. Because otherwise I have to duplicate every preset manually to get rid of it

Sorry that's the one I don't want :)

elstoc commented 1 year ago

I really don't understand why this convoluted UX is considered better than an extra entry / tick box in the preset UI

elstoc commented 1 year ago

I'll against 3 which will break my workflow.

You're gonna break someone's workflow

TurboGit commented 1 year ago

What would the tick do?

Since we have a menu entry to name, why not at the same place for un-name it?

What would it solves for the hard-coded preset?

TurboGit commented 1 year ago

You're gonna break someone's workflow

By adding an information? At least one can create a preset to avoid that, if we do not label the module after the preset name then there is nothing a user can recover. So by all mean the solution I propose let more freedom.

TurboGit commented 1 year ago

And we are not talking about a simple tick, in the style dialog we need a tick for each module selected. A whole new column.

TurboGit commented 1 year ago

You're gonna break someone's workflow

How many reports do we have since it is has been merged?

elstoc commented 1 year ago

At least one can create a preset to avoid that

No, I need to create perhaps a dozen presets, for anything I might choose to select and all the automatic stuff too.

EC1000 commented 1 year ago

In addition to #13998, would it make sense to have one option in global preferences:

"automatically update module label" If false, instance label is only update:

If true, it work as current master + #13998, meaning it also add:

I know it is a new option, but I think the two use cases are really different, and a global option here would work better than a per preset option. What do you think?

TurboGit commented 1 year ago

No, I need to create perhaps a dozen presets, for anything I might choose to select and all the automatic stuff too.

As I said you have at least a solution.

elstoc commented 1 year ago

Sigh

TurboGit commented 1 year ago

I know it is a new option, but I think the two use cases are really different, and a global option here would work better than a per preset option. What do you think?

You are probably right. Let me think this over night.

EC1000 commented 1 year ago

"automatically update module label"

To complete with a couple of remarks with this option:

EC1000 commented 1 year ago

So if you enter an empty string as the module's label creating a preset (or updating an existing one) for this module will record this empty string (hand-edited). And applying this preset will never show a label until it is again hand edited if needed.

I just tested #13998. There is something I did not understand correctly from your explanations @Turbogit I think. I though a hand-edited label would be stored and applied as hand-edited with the preset. But it does not seem to be the case, because the label is still reset as soon as I change a parameter.

TurboGit commented 1 year ago
  • Any hard-coded preset should be created without a label.

As I already stated this is important to me to have.

If I summaries:

I don't see how to handle all those cases!

A solution would be a flag to "never use preset name as module's label". That is, an option where we are back as in 4.2. And I can have what I've implemented in current master. But I suppose this is not what you are looking for. So, do you see another way to support all the cases?

EDIT: Also a hard-code preset is many (all?) cases tweaked, and so as soon as a parameter is changed the label is reset. Maybe not a big issue after all?

TurboGit commented 1 year ago

I though a hand-edited label would be stored and applied as hand-edited with the preset.

Yes it should and from my testing it was working, maybe I broke something in the last iteration of my PR.

elstoc commented 1 year ago

For me the clearest and most obvious UX would be to manage this all within the preset maintenance window:

IMO these parameters don't need to be reflected in the preset summary window in preferences and can just appear in the maintenance window.

As a separate supporting enhancement it would also be great to have a less "clunky" way of overriding hard-coded presets without having to create a duplicate with the same name. Ideally you would make the preset name and module params uneditable but allow the user to modify the remainder of the preset configuration. This would allow users to just add an instance name (or auto-apply params) to an existing preset but still take advantage of any updates to that preset delivered in subsequent versions.

EC1000 commented 1 year ago
  • Any hard-coded preset should be created without a label.

As I already stated this is important to me to have.

I think I was not clear and that you misunderstood me. By "Any hard-coded preset should be created without a label", I was only referring to the creation of the preset (in darktable code), not to the application of the preset, for which I understood it is an important point for you!

If I summaries:

  • @elstoc don't want any module label and would prefer as it was in 4.2
  • I prefer having the module label in all cases
  • @EC1000: you propose to have a mix of both, using label, controlled with a pref but never use the hard-coded presets

I don't see how to handle all those cases!

I think the solution I suggested allow to handle nearly all those cases:

@elstoc are you okay with this exception? For system preset: you don't have to do anything, no label is applied. For user-defined preset, you just have to save/update it without a label, if you want it without a label when applied.

TurboGit commented 1 year ago

For me the clearest and most obvious UX would be to manage this all within the preset maintenance window:

This would work for sure. What I'm not yet clear is what to do with styles? Creating a style is also taking a snapshot of a set of modules, we have the same issue here. We can say that we don't care about style and that the current behavior stays, but this won't be consistent with presets. So what is your view about styles?

Just a side note, I really really want to avoid adding more stuff into the style dialog. It is already quite crowded.

TurboGit commented 1 year ago

@EC1000 : Just saw your message, we cross-posted and so my message did not take into account what you said. I'll read and come back.

EC1000 commented 1 year ago

For me the clearest and most obvious UX would be to manage this all within the preset maintenance window:

  • A new field "instance label" that exposes the existing database column to the user
  • A second field "remove auto-label if parameters are changed" (which could appear only if the preset instance label is non-blank). If this parameter is unchecked then selecting the preset actually writes to the module's instance name (with a potential integer suffix to prevent duplicates), if not, and the instance name is blank, then the instance label is a display-only thing that only applies to otherwise unnamed instances (like the current master functionality).

For the first point, I am okay both with this solution or the one implemented by TurboGit where you can change it directly on the module. Yours is more explicit but add a redundant field (that was also my initial proposition); and the opposite for Turbogit's proposition.

For the second point, I really think a more global option "automatically update module label" is preferable:

EC1000 commented 1 year ago

So what is your view about styles?

The behaviour with styles is that it save the label of the module. Your implementation in https://github.com/darktable-org/darktable/pull/13998 make it consistent between styles and presets.

elstoc commented 1 year ago

I don't see why someone would want to change this behaviour per preset. At least, I don't see any use case for me or for Turbogit; do you have one?

Some of the auto presets really add no information and are not helpful. The biggest one I can think of is the auto preset for orientation. I can't see any point at all to seeing the preset information for this module when it's obvious whether the orientation is correct.

If the change was made as I suggested, I might even use the functionality for a small number of presets (diffuse-or-sharpen presets often have a single purpose and might be helpful to have an auto label on multi-instance). But the way it's designed at the moment I'd prefer to switch it off entirely. As I mentioned before, I'd rather not do this, but it's annoying enough that I'd feel the need to create my own private fork with the functionality disabled (which is the main alternative I'm considering).

So what is your view about styles?

I think styles have to retain the multi-instance name of the modules, because they can actually contain multiple named instances. But I really don't use styles much so I'm not entirely bothered one way or another.

elstoc commented 1 year ago

The behaviour with styles is that it save the label of the module. Your implementation in https://github.com/darktable-org/darktable/pull/13998 make it consistent between styles and presets.

Does it? My impression was that the preset functionality was just changing the label of the module and not its actual instance name (which was what I meant by changing the meaning of that field)

EC1000 commented 1 year ago

Some of the auto presets really add no information and are not helpful. The biggest one I can think of is the auto preset for orientation. I can't see any point at all to seeing the preset information for this module when it's obvious whether the orientation is correct.

I agree with you. But this is a pre-defined preset. So if the behavior about setting/resetting label is define per preset, we would have to choose if a pre-defined preset with this option or not. So it won't work both for Turbogit (who want to have the label) and you/me (who don't). That is why a global option works better here I think.

If the change was made as I suggested, I might even use the functionality for a small number of presets (most diffuse-or-sharpen often have a single purpose and might be helpful to have an auto label on multi-instance).

With my proposition, you would have the possibility to do that for user-defined presets. Not for pre-defined modules (no label would be applied); but with your proposition neither since it is pre-defined. In both solutions, there is still the possibility to duplicate a pre-defined module to save it with a label and/or your option.

But the way it's designed at the moment I'd prefer to switch it off entirely.

That is why I opened this issue in the first place, but I really think what I suggest could be satisfying for the different uses. I would really like to have your opinion about the question I asked at the end of https://github.com/darktable-org/darktable/issues/13982#issuecomment-1480950445. The option "false" in my solution is not to switch if off entirely, because as you stated it is helpful to have an auto label on multi-instances and a few other cases.