-
Notifications
You must be signed in to change notification settings - Fork 1.2k
toneEQ: move exposure/contrast compensation sliders to advanced tab #18682
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
Conversation
|
I haven't build this PR, so I can't see the layout. But I had an idea for a while. What if we get rid of the top 3 tabs? Make the advance be the default (without the word advance, just show the histogram) and make the simple tab and the masking options tab be an expand pull down (like you did in Agx POC). This way we save the vertical space most of the time and it keeps the histogram always showing. |
|
the gap is there because the minimum height of the module is determined by the sliders in simple tab. the histogram representing mask bar is no longer used, since the effect of the moved sliders can be seen directly in the histogram. |
|
Obviously I don't have anything working right now on my PR. Even though I haven't given up just yet, it looks like my stuff will take a lot longer. Just for inspiration, I wanted to show what my current UI looks like, after all the discussions in that forum thread. I went for a version, where the histogram is always visible and the tabs are placed below it. This is useful, because smoothing changes the histogram too, which is immediately visible here. I also put almost everything into collapsible sections, so people cen decide for themselves, how much vertical space they want to use. Regarding mask quantization: I don't think that this is an option that should be placed in a prominent location. I don't think that this is widely used, I regard it as an unsuccessful attempt to have a little bit of zone system in the module. |
|
This pr keeps the vertical space needed for the module, since there’re users running darktable on capable notebooks where screen size is limited. |
|
This pr isn’t intended to be a major change of the toneequalizer UI - so no further changes beside what’s needed to optimize the handling of the mask histogram. |
|
@MStraeten : You may want to try out #18656 (be ware that this will break all your ToneEQ presets). I think #18656 is the way to go, the auto-fit is especially fixing a itch I had for this module. |
|
This pull request has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please verify it has no conflicts with the master branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
31e751c to
1d9082e
Compare
|
Thanks for keeping the PR up-to-date, I've been using those changes for 5 months now and I'm really happy with the tone equalizer workflow improvements, which probably saved me already many hours of my precious lifetime 😅 |
…vanced tab since there're endless discussions on tweaking the tone equalizer a lowest common denominator might be simply moving the controls for mask exposure and contrast compensation to the advanced tab. since the real estate of the gui is driven by the simple tab, the additional required vertical space is taken from the graph and in masking tab there's unused space. the bar indication the spread of the histogram on masking tab is no longer needed, so removed
added the UI/UX change
|
@TurboGit since there’s no progress on the big changes for toneEQ maybe this makes sense to be a working solution for 5.4 |
|
I have thought about this already. My version won't be in time for the Christmas release. I am pretty busy right now, there is still a lot to do and afterwards we need testing, community feedback, write new documentation etc. My current worst-case plan is to code a release candidate version during the holidays. Then we would have full 6 months to make sure that everything is nice and polished. Temporary UI improvements are something that could be merged before. However there is a caveat: Your proposal would be moving sliders to the advanced page and I would be moving them back to their original place in the next release (and add similar but different sliders to the equivalent of the advanced page). I am afraid that this would confuse users. If the tone equalizer UI is changed now, we need to make sure that the changes are consistent. One such option would be to leave all slides where they are, but also show the graph on the masking page (for example in a collapsible section). In my future version the graph will always be visible and the tabs will be below it, but this requires considerations regarding vertical space. |
|
you‘re changing the UX so the sliders won’t be used as they are used in the moment. So keeping a weak solution just to be consistent with a future solution (no one knows when it’s ready to be released yet) is quite strange… |
|
I can just say that I intend to deliver a polished version for next summer. Everything else has to be decided by @TurboGit . |
I've been using this UI improvement for quite a while and would be very happy if we could merge it until Marc's PR is ready as it has been improving usability for me a lot. |
|
Like most people I use the stable DT versions for my productive editing, so I have to wait for my new toneeq as long as everyone else. I have not tried it, but I am pretty sure that I like @MStraeten's proposal more than the current state of things. So if changing something now and possibly changing it back in 6 months is not a concern, I am fine with this change. |
|
Or you could just assign keyboard shortcuts to the sliders and adjust them using the shortcuts while you're in the advanced tab. |
|
Ok, I have no string opinion. Really. @marc-fouquet arguments about the fact that the sliders will be moved again is a valid one. On the other hand the next version of the TE module will have lot more changes and so could be considered as a new UI requiring some adjustment from our users. The modifications proposed here are sound and limited to some sliders (at the same time removing a redundant display of the graph in masking tab). So, let's move on. Add this small usability fix now. @MStraeten : Sorry for the long delay. I was hoping to have some more pro/cons discussion here before merging. |
TurboGit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!




since there're endless discussions on tweaking the tone equalizer a lowest common denominator might be simply moving the controls for mask exposure and contrast compensation to the advanced tab. since the real estate of the gui is driven by the simple tab, the additional required vertical space is taken from the graph and in masking tab there's unused space. the bar indication the spread of the histogram on masking tab is no longer needed, so removed