Skip to content

imageop: fix blank module instance names when not on top of history - #21661

Merged
TurboGit merged 2 commits into
darktable-org:masterfrom
da-phil:fix_module_names_disappear_when_going_back_in_history
Aug 7, 2026
Merged

imageop: fix blank module instance names when not on top of history#21661
TurboGit merged 2 commits into
darktable-org:masterfrom
da-phil:fix_module_names_disappear_when_going_back_in_history

Conversation

@da-phil

@da-phil da-phil commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Currently, when you have IOP modules with custom names, they disappear once you go back in editing history, switch back to lighttable and open the same image again, see screencast:

Screencast.from.2026-07-26.13-15-29.mp4

The reason: _iop_panel_name() skipped painting the header label whenever history_end < the number of history entries, so every module's instance name stayed blank when an image was opened with its history slider below the top of the stack. multi_name is already restored per history position by dt_dev_pop_history_items(), so the label is correct at any position.

After the fix:

Screencast.from.2026-07-28.22-32-53.mp4

CC: @TurboGit
It might be good to get your feedback on this change, as you added it a long time ago in commit e93ec95
I assume you treated the panel name as the module's current identity and you didn't want it flickering/blanking during history navigation, so the guard froze the labels while browsing a non-top state. This was solving a cosmetic concern (stable names during scrubbing) in a way that both displayed stale names and left names permanently blank on a non-top open. Since multi_name is always kept correct for the current position, unconditionally painting it is both simpler and more accurate — which is what the change does.

Disclaimer: this work has been co-created with Claude.

@TurboGit

Copy link
Copy Markdown
Member

Really strange, indeed the comment is clear but I don't remember why this was done! Fixing something? No idea now but you proposed change look good to me.

One issue though, if you have history:

10 local constrast - toto
09 exosure
08 local contrast
07 AgX

Say you are on 10, the module is labeled "local contrast - toto" fine.

Now move directly to 07, the module is still labeled "local constrast - toto". This is probably because there is no more local contrast on the history and so the module is disabled. Worth fixing as part of this PR to be fully consistent.

@TurboGit TurboGit added the bugfix pull request fixing a bug label Jul 29, 2026
@TurboGit

Copy link
Copy Markdown
Member

BTW, worth testing with multiple instances to be sure there is no corner case explaining the current code.

@TurboGit TurboGit added this to the 5.8 milestone Jul 29, 2026
@TurboGit TurboGit added priority: low core features work as expected, only secondary/optional features don't scope: UI user interface and interactions labels Jul 29, 2026
@TurboGit
TurboGit self-requested a review July 29, 2026 19:58
@da-phil

da-phil commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Really strange, indeed the comment is clear but I don't remember why this was done! Fixing something? No idea now but you proposed change look good to me.

One issue though, if you have history:

10 local constrast - toto 09 exosure 08 local contrast 07 AgX

Say you are on 10, the module is labeled "local contrast - toto" fine.

Now move directly to 07, the module is still labeled "local constrast - toto". This is probably because there is no more local contrast on the history and so the module is disabled. Worth fixing as part of this PR to be fully consistent.

I cannot reproduce this issue, see screencast:

Screencast.from.2026-08-03.23-41-41.mp4

@da-phil

da-phil commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

BTW, worth testing with multiple instances to be sure there is no corner case explaining the current code.

Works like a charm, I couldn't find an issue. Essentially renaming a module is a dedicated entry in the history, and if you go back in your history, the renaming is reverted, exactly what I'd expect.

@TurboGit

TurboGit commented Aug 4, 2026

Copy link
Copy Markdown
Member

I cannot reproduce this issue, see screencast:

Because my description is not precise enough :)

The num 11 must be disabled, see:

image
  • So you are on 14 - module is "Contraste local - toto"
  • You click directly on 11 - module is still "Contraste local - toto"

As there is not more "Contrast local" in the history, so module should be just "Contraste local"

@da-phil

da-phil commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

The num 11 must be disabled, see:

Hmmm, turns out I cannot disable actions in the history on current master (with my fix applied).
I would just click on the power button symbol, correct?

@Arecsu

Arecsu commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The num 11 must be disabled, see:

Hmmm, turns out I cannot disable actions in the history on current master (with my fix applied). I would just click on the power button symbol, correct?

You mean, the power icon to the right of each element in the history? They just reflects the state of the module that produced that history entry. They go like enabled normal, disabled a bit dim along with the text label.

I would argue the UI/UX of this power icon being there is weird if they are in every single element whether they are enabled or disabled, and we already have the text labels changing opacity as well.

Besides that, to sum it up, no, these power icons do nothing. I have to confess I'm deeply paranoid now about any of those things after my event controllers PR, and I had to do a research to come up with that answer 🤣

@da-phil

da-phil commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

I cannot reproduce this issue, see screencast:

Because my description is not precise enough :)

The num 11 must be disabled, see:
image

* So you are on 14 - module is "`Contraste local - toto`"

* You click directly on 11 - module is still "`Contraste local - toto`"

As there is not more "Contrast local" in the history, so module should be just "Contraste local"

You're right, I can reproduce this behavior, this is really weird...
I'll also try to address this glitch.

_iop_panel_name() skipped painting the header label whenever
history_end < the number of history entries, so every module's instance
name stayed blank when an image was opened with its history slider below
the top of the stack. multi_name is already restored per history position
by dt_dev_pop_history_items(), so the label is correct at any position.
@da-phil
da-phil force-pushed the fix_module_names_disappear_when_going_back_in_history branch from c8eabe0 to e49902d Compare August 6, 2026 22:28
dt_dev_pop_history_items_ext() reset params, blend params and enabled
for every module but not multi_name, which is only written when
replaying history up to history_end. A module renamed at a later
position therefore kept that name when rewinding to an earlier one.
Reset multi_name/multi_name_hand_edited in the reset loop like the
other per-position state.
@da-phil
da-phil force-pushed the fix_module_names_disappear_when_going_back_in_history branch from e49902d to 55d9ec5 Compare August 6, 2026 22:30
@da-phil

da-phil commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@TurboGit I think I was able to fix the issue, I pushed another commit

@da-phil

da-phil commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@Arecsu

I would argue the UI/UX of this power icon being there is weird if they are in every single element whether they are enabled or disabled, and we already have the text labels changing opacity as well.

I know this is a tangent to the scope of this PR but I'd agree, I don't like those power icons either, they suggest a possible interaction in a UX context, which they are not, hence they can easily confuse users.

@TurboGit TurboGit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works for me now, thanks! The fix is clean and simple.

@TurboGit
TurboGit merged commit 6e15409 into darktable-org:master Aug 7, 2026
5 checks passed
@da-phil
da-phil deleted the fix_module_names_disappear_when_going_back_in_history branch August 7, 2026 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix pull request fixing a bug default-behavior-change priority: low core features work as expected, only secondary/optional features don't scope: UI user interface and interactions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants