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) ?

[victoryforce]

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

Making every module test for being applicable depending on image type is a very good idea as it will greatly simplify such actions as copy-pasting history between images.

We just need to add one more function to the IOP API that will adjust the module status depending on the type of image being processed: disable if the module is incompatible with the image type and force-enable if the module is mandatory for the image type.

[dterrahe]

Wouldn't this require making every module test for being applicable depending on image type

I'm not sure what you mean with "wouldn't". Every module with special requirements does have to test if it is applicable (or required!) for the currently loaded image and set default_enabled and hide_enable_button accordingly. They do so (amongst other things) in reload_defaults, which is called after an image has been loaded. Some modules, as I said, also set these flags in gui_update presumably because the author was confused about when and in what order these methods are called. I would assume that if they change (set something different) there, it would be too late to correctly handle this and there would be bugs. But I think this situation doesn't currently occur, the code in most cases seems to be just duplicated, although haven't looked deeply.

Making every module test for being applicable

As described above, this information is already available for the currently loaded image, so could be tested when pasting history to it in the darkroom (maybe it already is, I haven't looked).

depending on image type

For images that have not been loaded it is a little more complicated, because the condition is not just simply image type; it could be dependent on which raw format (mosaic pattern), for example, or if it is monochrome. So the new IOP API method would have to take as input more than image type somehow. I think these are not necessarily known before the image has been loaded. Loading each image when pasting to a selection in the lighttable/filmstrip would, I assume, cause a significant slowdown.

I was wondering if maybe the "right" place would be dt_iop_reload_defaults

A small benefit would be that the function could be made local to imageop.c