allow widgets in sections and collapsibles and curve widgets to mark tabs as changed - #22004
allow widgets in sections and collapsibles and curve widgets to mark tabs as changed#22004piratenpanda wants to merge 11 commits into
Conversation
|
Did some (limited) testing: Seems to fix what it sets out to fix :-) . |
|
@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". |
|
@TurboGit latest commit adds this functionality. If it's the most elegant solution I don't know. |
|
loading presets does not trigger this, will investigate |
|
@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
left a comment
There was a problem hiding this comment.
Check if it is possible to use introspection for tab status and resetting to default.
There was a problem hiding this comment.
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.
Claude says: 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 |
|
Hum, maybe Claude is right, but I had the following idea in mind.
For iterating on tab widgets, see gtk.c: & |
|
BTW, the highlight of changed tab was implemented in 9fb5a74. Probably worth looking at the code there. |
|
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 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 |
|
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: 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. |
|
latest commit removes the handler and follows another path which is better understandable in my opinion |
|
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. |
|
I like the visual consistency but sure, it's up to you :) |
|
latest commit reduces the complexity a bit |
Me too, hence my questioning :) |
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