Setting darkroom/ui/hide_header_buttons=active causes a reset of a module toggles the collapse of the module

[AxelG-DE]

Describe the bug

Clicking on the module reset button toggles the module collapse

bisecting my darktablerc (because a fresh darktablerc does not show that behaviour) gave me, that the setting
darkroom/ui/hide_header_buttons=active
is the culprit. Switching that to "always" as in a fresh darktablerc, the behaviour is normal

This is not only on current master, but also on 5.0.0

Steps to reproduce

1. have darkroom/ui/hide_header_buttons=active set
2. click on the reset button of a module
3. see the "error"

Expected behavior

mdules shall never toggle the collapse when restting

Logfile | Screenshot | Screencast

No response

Commit

No response

Where did you obtain darktable from?

self compiled

darktable version

5.1.0+27-g6063a61511

What OS are you using?

Linux

What is the version of your OS?

Gentoo

Describe your system?

• darktable version : 5.1.0-36-ge932ddb438
• OS : Linux - kernel 6.12.8-gentoo
• Distro : Gentoo Base System release 2.17
• Processor : Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz
• Memory : 32 GB (4 x 8 GB) + 5GB Swap
• Graphics card0 : NVIDIA GeForce GTX 1060 6GB
• Graphics card1 : NVIDIA GeForce RTX 2070 SUPER
• Graphics driver : nvidia-drivers-550.142
• OpenCL installed : Yes (opencl-headers-2024.05.08)
• OpenCL activated : Yes
• Xorg : xorg-server-21.1.14
• Desktop : KDE 6
• GTK+ : gtk+-3.24.42-r1
• gcc : 14.2.1_p20241221
• cflags : CMAKE_FLAGS="-march=native-O2-mtune=native-pipe"
• CMAKE_BUILD_TYPE : "Release"

Are you using OpenCL GPU in darktable?

Yes

If yes, what is the GPU card and driver?

see above

Please provide additional context if applicable. You can attach files too, but might need to rename to .txt or .zip

see above

[AxelG-DE]

it might be related to #17985 or its trigger or fix

[scorpi11]

I can confirm this with darktable 5.0.0 running on Debian Sid.

[dterrahe]

This is caused by the addition of a call to dt_iop_show_hide_header_buttons in dt_iop_gui_update in #9619. This call might not be needed because the values of module->default_enabled and module->hide_enable_button don't change after the call to dt_iop_show_hide_header_buttons in dt_iop_gui_set_expander. But maybe there was another reason @jenshannoschwalm ?

This issue similarly prevents using the scroll wheel while hovering over the presets button to try more than one preset (if hide_header_buttons != always)

[jenshannoschwalm]

@dterrahe thanks for pinpointing and inducing a brain-stress-test :-)

Indeed there was "some reason" but you are right, the "fix" was at the wrong place. The reproducer without that line in question would be:

1. Open a raw file in darkroom and keep mouse over demosaic-module headerbar.
2. Use space or BS to change image in darkroom - this must be any non-raw.
3. See the header keeps widgets simply no applicable. (Don't know if a callback could fail here ...)

The way to fix would be likely to put that line into darkroom.c -- _dev_load_requested_image()

      //  update the module header to ensure proper multi-name display
      if(!dt_iop_is_hidden(module))
      {
        // Make sure modules with a different applicable status are reset correctly
        dt_iop_show_hide_header_buttons(module, NULL, FALSE, FALSE);
        snprintf(option, sizeof(option), "plugins/darkroom/%s/expanded", module->op);
        module->expanded = dt_conf_get_bool(option);
        dt_iop_gui_update_expanded(module);
        if(module->change_image) module->change_image(module);
        dt_iop_gui_update_header(module);
      }
    }
  }

Will do some more testing before doing a pr

[dterrahe]

You are absolutely right of course and your solution looks ok.

I was wondering if maybe the "right" place would be dt_iop_reload_defaults (called last, grouped with dt_iop_gui_update_header), because reload_defaults is where most modules set the relevant fields.

colorreconstruction.c is an exception. It sets hide_enable_button in gui_update (it doesn't have a reload_defaults). Without having done further investigation, that looks like an error to me (that could cause problems with any of the above fixes?)

highlights.c, hotpixels.c and temperature.c set the field both in reload_defaults and gui_update for good measure (with, it seems, duplicated code, just to be sure lalala, which just complicates testing etc, because if a dev tests a change just with this module and it works, it might still fail with others. And it creates uncertainty when implementing new modules; which "example" module is the correct one? Better duplicate stuff again. And maybe let it get out of sync later, again complicating test coverage. But of course now (or ever) is not a good time to clean such stuff up).

[jenshannoschwalm]

I was wondering if maybe the "right" place would be dt_iop_reload_defaults (called last, grouped with dt_iop_gui_update_header), because reload_defaults is where most modules set the relevant fields.

Wouldn't this require making every module test for being applicable depending on image type (raw vs non-raw would be the most obvious) ?