Skip to content

allow widgets in sections and collapsibles and curve widgets to mark tabs as changed - #22004

Draft
piratenpanda wants to merge 11 commits into
darktable-org:masterfrom
piratenpanda:tagreset2
Draft

allow widgets in sections and collapsibles and curve widgets to mark tabs as changed#22004
piratenpanda wants to merge 11 commits into
darktable-org:masterfrom
piratenpanda:tagreset2

Conversation

@piratenpanda

Copy link
Copy Markdown
Contributor

While working on the spektrafilm module I noticed that widgets in sections and collapsibles don't mark the tab state as changed and also don't allow widgets to be reset by double clicking. This PR brings deeper parsing and also provides functionality to exclude certain widgets like the ones in the color calibration collapsibles to not mark the tab as changed and also not resettable as this is wanted in this specific case. I tested AgX and Filmic and both behave like expected for me but some more testing on other systems might be good.

Co-created with Claude Opus 5.0

@anoderay

Copy link
Copy Markdown
Collaborator

Did some (limited) testing: Seems to fix what it sets out to fix :-) .

@TurboGit

Copy link
Copy Markdown
Member

@piratenpanda : The module color equalizer don't work - no highlight when curve change and no reset by double-click on tab - (not working before this PR), maybe a good time to fix the tab on this module. Likewise for "denoise profile" and "contrast equalizer".

@piratenpanda

Copy link
Copy Markdown
Contributor Author

@TurboGit latest commit adds this functionality. If it's the most elegant solution I don't know.

@piratenpanda piratenpanda changed the title allow widgets in sections and collapsibles to mark tabs as changed allow widgets in sections and collapsibles and curve widgets to mark tabs as changed Aug 27, 2026
@piratenpanda

Copy link
Copy Markdown
Contributor Author

loading presets does not trigger this, will investigate

@TurboGit

Copy link
Copy Markdown
Member

@piratenpanda : All this is working, but I feel that the distributed complexity is not good. I'm think (almost sure) we can handle this at the notebook level without changing the IOP modules. That is, passing self and using introspection to check for value not being default or reset them to the default value. Do you think you can try this path? I'm sure Claude will be able to do the hard work...

@TurboGit
TurboGit requested review from TurboGit and a lite review from Copilot August 28, 2026 12:30

@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.

Check if it is possible to use introspection for tab status and resetting to default.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends Darktable’s GUI “tab changed” + “double-click tab to reset” behavior to correctly handle:

  • parameter widgets nested inside sections/collapsibles,
  • controls that live outside the notebook page (e.g. slider stacks),
  • pages whose effective state is represented by non-widget data (e.g. curve/graph state),
  • and explicit exclusions for workflow-only controls that must not affect tab state or be reset.

Changes:

  • Add a unified notebook-page traversal API (dt_bauhaus_page_foreach) and use it to drive both tab-state detection and tab reset.
  • Introduce page↔external-content linking and per-page tab-state handlers, enabling empty tabs / curve-backed tabs to report changed/reset state.
  • Add mechanisms to exclude widget subtrees from tab-state/reset, plus refresh/update helpers for non-widget edits and for updated defaults after reload_defaults().

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/iop/denoiseprofile.c Adds per-tab curve-backed tab-state handler + forces tab-state refresh after graph edits.
src/iop/colorequal.c Links empty notebook tabs to slider-stack pages so tab state/reset operates on the stack content.
src/iop/channelmixerrgb.c Adds CAT tab-state handler for illuminant xy and excludes workflow-only collapsible section from tab reset/state.
src/iop/atrous.c Adds per-tab curve-backed tab-state handler + forces tab-state refresh after graph edits/resets.
src/gui/gtk.h Declares new tab-state APIs: exclude subtree, page↔content mapping, and per-page handler interface.
src/gui/gtk.c Updates tab reset to traverse full page contents (including external content) and adds tab-state plumbing.
src/develop/imageop.c Ensures bauhaus widget defaults are updated after reload_defaults() changes default_params.
src/bauhaus/bauhaus.h Declares page traversal, notebook refresh, and default synchronization helpers.
src/bauhaus/bauhaus.c Implements page traversal + improved tab highlight logic, plus notebook refresh and default update support.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/gui/gtk.c Outdated
@piratenpanda

Copy link
Copy Markdown
Contributor Author

@piratenpanda : All this is working, but I feel that the distributed complexity is not good. I'm think (almost sure) we can handle this at the notebook level without changing the IOP modules. That is, passing self and using introspection to check for value not being default or reset them to the default value. Do you think you can try this path? I'm sure Claude will be able to do the hard work...

Claude says:
Introspection has no page or section metadata. dt_introspection_type_header_t carries type, type_name, name, field_name, description, size, offset: nothing that says which tab a field belongs to. DT_IOP_SECTION_FOR_PARAMS passes a section name for action grouping (.data = (void*)section) and never reaches introspection. So passing self to the notebook still leaves it unable to say which fields the "luma" tab governs.

And for the equalizers it's worse than missing metadata. An atrous luma tab governs x[atrous_L][], y[atrous_L][], x[atrous_Lt][], y[atrous_Lt][]: four slices of two 2D arrays, selected by a channel enum. A tab maps to rows of arrays, not to fields, so no per-field annotation scheme addresses it. It would need per-element tagging.

Making this work would mean extending the introspection generator with something like $TAB: alongside $DEFAULT/$DESCRIPTION, which handles colorequal and the CAT page but still not atrous or denoiseprofile: a bigger change that removes less than it looks.

@TurboGit want me to give this a try? I don't know if I understand everything, but I might learn on the way

@piratenpanda
piratenpanda marked this pull request as draft August 28, 2026 15:11
@TurboGit

Copy link
Copy Markdown
Member

Hum, maybe Claude is right, but I had the following idea in mind.

  • When clicking on a tag, the tab widget is known
  • You can iterate on all widgets in the tab
  • For each widget you then check the value and compare it with the default value using introspection

For iterating on tab widgets, see gtk.c:

static void _reset_all_bauhaus(GtkNotebook *notebook,
                               GtkWidget *box);

&

static void _notebook_button_press_callback(GtkGestureSingle *gesture,
                                            gint n_press,
                                            gdouble x,
                                            gdouble y,
                                            gpointer user_data)

@TurboGit

Copy link
Copy Markdown
Member

BTW, the highlight of changed tab was implemented in 9fb5a74. Probably worth looking at the code there.

@piratenpanda

Copy link
Copy Markdown
Contributor Author

implemented some changes you suggested and looked at the tab changed commit:

For atrous and denoiseprofile, iterating the widgets in the tab returns nothing, because there are none:

dt_ui_notebook_page(g->channel_tabs, N_("luma"), ...); // return value discarded
...
self->widget = dt_gui_vbox(g->channel_tabs, g->area); // the graph is a sibling

The pages are created for their labels only; one graph below the notebook is repointed at a channel by tab_switch(). No iteration over page contents can see it. That's the entire reason for the handler

@piratenpanda

Copy link
Copy Markdown
Contributor Author

I asked Claude for another way:

Reshape the params so a tab owns a contiguous block

The reason nothing generic works is the data layout, not the GUI. Atrous stores float x[atrous_none][BANDS] — channels are rows of one array shared by every tab, so a tab owns four disjoint slices of two arrays. If it were instead:

typedef struct { float x[BANDS], y[BANDS]; } dt_iop_atrous_band_t;
struct { dt_iop_atrous_band_t luma, luma_threshold, chroma, chroma_threshold, edges; }

then a tab is one offsetof and one sizeof. Changed becomes a memcmp, reset a memcpy, and a $TAB introspection tag would even work, because the tab now maps onto a field.

Cost is a params version bump plus legacy_params() conversion — atrous is at v2 and already has that machinery, denoiseprofile at v11+ with an incremental chain, so it's routine work rather than novel risk. XMP compatibility is preserved by the conversion. This is the only option that makes the problem disappear instead of routing around it.

@piratenpanda

Copy link
Copy Markdown
Contributor Author

latest commit removes the handler and follows another path which is better understandable in my opinion

@piratenpanda
piratenpanda marked this pull request as ready for review August 29, 2026 08:28
@piratenpanda
piratenpanda deleted the tagreset2 branch August 29, 2026 08:41
@piratenpanda
piratenpanda restored the tagreset2 branch August 29, 2026 08:42
@piratenpanda piratenpanda reopened this Aug 29, 2026
@TurboGit

Copy link
Copy Markdown
Member

I'm still a bit uneasy with all the distributed complexity. I'm still asking myself : Is all that worth the effort? Let me some time to think more about this.

@piratenpanda

Copy link
Copy Markdown
Contributor Author

I like the visual consistency but sure, it's up to you :)

@piratenpanda

Copy link
Copy Markdown
Contributor Author

latest commit reduces the complexity a bit

@piratenpanda
piratenpanda marked this pull request as draft August 29, 2026 12:27
@TurboGit

Copy link
Copy Markdown
Member

I like the visual consistency but sure, it's up to you :)

Me too, hence my questioning :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants