Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style dialogs #18172

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Style dialogs #18172

merged 2 commits into from
Jan 21, 2025

Conversation

esq4
Copy link
Contributor

@esq4 esq4 commented Jan 7, 2025

  1. Makes copies of the module available for adding in 'edit style' dialog.
  2. Leaves the latest module settings for adding in the 'create new style' dialog.

@wpferguson
Copy link
Member

Could you explain what this supposed to do? I tried to test, but I can't find whatever changed.

@esq4
Copy link
Contributor Author

esq4 commented Jan 7, 2025

  1. Please try creating a Test style that includes the "exposure" module, and then add "exposure 1" (a copy of "exposure") to it. I can't do it.

  2. If there are several module iterations in the history (for example, on/off), then when creating a new style, they will all be offered in the list for selection. It seems illogical to me: offering intermediate settings is like roulette.

@wpferguson
Copy link
Member

Please try creating a Test style that includes the "exposure" module, and then add "exposure 1" (a copy of "exposure") to it. I can't do it.

You create a style with only exposure. Apply the style to an image. Create another instance of exposure (i.e. exposure 1) then create a style from that.

If there are several module iterations in the history (for example, on/off), then when creating a new style, they will all be offered in the list for selection. It seems illogical to me: offering intermediate settings is like roulette.

Always pick the top one.


I tested with the PR and without and I don't see any differences.

@esq4
Copy link
Contributor Author

esq4 commented Jan 7, 2025

You create a style with only exposure. Apply the style to an image. Create another instance of exposure (i.e. exposure 1) then create a style from that.

It seems to me that the point of styles is precisely to combine several modules. And what you suggest can be done without styles using profiles.

Always pick the top one.

Of course. But explain to me, why do it with your eyes if you can do it with a program?

@wpferguson
Copy link
Member

It seems to me that the point of styles is precisely to combine several modules. And what you suggest can be done without styles using profiles.

Profiles? Did you mean presets? And, no presets can't do that.

But more to the point, how does this PR allow you to add another instance of exposure to the style?

@esq4
Copy link
Contributor Author

esq4 commented Jan 8, 2025

history
изображение

w/o PR
изображение

with PR
изображение
изображение

@wpferguson wpferguson added feature: enhancement current features to improve scope: UI user interface and interactions labels Jan 8, 2025
@wpferguson
Copy link
Member

OK, now I finally understand, though I'm not sure how anyone would understand from

Makes copies of the module available for adding in 'edit style' dialog.

and

Leaves the latest module settings for adding in the 'create new style' dialog.

The better way to have done this would be

  • open a feature request
  • explain the problem thoroughly
  • add pictures if necessary (as in this case)

Then when you do a PR you can reference the feature request and people can understand what you're trying to do.

If they have to spend hours (compiling, testing, messaging, thinking) in order to understand your PR, they are going to just ignore it. They will figure if it wasn't important enough to you to take the necessary time and steps to make it understandable, then it's not important.


If this gets merged, it will also require a update to the styles module docs.

@esq4
Copy link
Contributor Author

esq4 commented Jan 8, 2025

OK, now I finally understand, though I'm not sure how anyone would understand from

Yes, I'm sorry, that was too laconic (but vacation is not soon :(.

If this gets merged, it will also require a update to the styles module docs.

I read the user manual but didn't see anything related to this fix.

@TurboGit
Copy link
Member

TurboGit commented Jan 8, 2025

I think I got the goal of this PR, but I had no time to report here before. So if I'm not mistaken the idea is to be able to get the duplicate instances into the style dialog when modifying a style. Right?

@wpferguson
Copy link
Member

I have an image and I edit it and create a style.

Then I go back to the image and create another instance of a module and further edit the image.

I go back to lighttable, select the image, then select the style and click edit.

Now I can add the extra instance of the module that I created after I had created the style.

@esq4
Copy link
Contributor Author

esq4 commented Jan 9, 2025

@TurboGit @wpferguson Absolutely right. (I note that when creating a style, the ability to add several instances of a module is initially available.)

I looked at creating a style in Darktable 3.4.1: it only offers the latest versions of module instances. (hmm.. it's been a while since I created styles).

изображение

@TurboGit TurboGit added this to the 5.2 milestone Jan 11, 2025
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I don't want to take any risk with 5.0.1 (I know this part is tricky and has already been broken in the past) so I've planned this for 5.2.

@TurboGit TurboGit merged commit 1dd26db into darktable-org:master Jan 21, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: enhancement current features to improve release notes: pending scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants