colorharmonizer: reset histogram validity and auto-detect on image switch

[masterpiga]

The problem

Base module instances are retained across darkroom image switches without
re-running gui_init(). Because colorharmonizer lacked a change_image()
hook, g->histogram_valid remained TRUE and the auto-detect button remained
sensitive when loading a new image. Clicking auto-detect before the new
preview finished (or when colorharmonizer was disabled on the new image,
where the preview pipe never runs process()) evaluated the previous
image's hue histogram and committed it to the new image's history, also
forcibly enabling the module.

In addition, _update_histogram() scheduled an idle callback using the
deprecated gdk_threads_add_idle() keyed on the GtkWidget rather than
self. This leaked widget references on aborted runs and prevented
cancelling pending sources on image transitions or teardown.

The fix

• Implement change_image() to drain pending idle callbacks, reset
  g->histogram_valid = FALSE, and clear pending auto-detection.
• Drain idle callbacks on gui_cleanup() to prevent running on torn down
  gui_data.
• Key idle callbacks on self using g_idle_add(), dropping the redundant
  widget ref/unref.
• Replace private g->histogram_lock with
  dt_iop_gui_enter_critical_section(), bringing the edge detection
  and validity checks inside the critical section.
• Support on-demand auto-detection: if clicked when the histogram is
  not yet available, enable the module and defer detection until the
  preview pipe completes.
• Consolidate button sensitivity and tooltip updates into a helper.

Fixes #22059

[kofa73]

RELEASE_NOTES.md update is missing, otherwise the bots are happy; they mentioned one pre-existing low-prio finding, maybe you can address that: "Pre-existing UX issue: when colorharmonizer is disabled, the auto-detect button remains insensitive and its tooltip says to wait for preview processing, although processing will not occur until the module is enabled. This commit does not introduce the behavior; it makes the existing initial state recur correctly after image switches while preventing stale-histogram use."

[masterpiga]

Thanks, @kofa73, both done

[kofa73]

You know them bots better than I do... I did not have time to build and test, I guess you have a working copy available.

• low [src/iop/colorharmonizer.c:1390]: auto-detect button state is never refreshed when the module is switched on or off

  The commit makes both the button's sensitivity and the click handler depend on self->enabled (gui_update() at :1167-1172, the idle at :1375, the click guard at :1390), but nothing re-evaluates that state when the user toggles the module's power button. _gui_off_callback() (src/develop/imageop.c:1214) sets module->enabled and calls dt_dev_add_history_item(), and _dev_add_history_item() (src/develop/develop.c:1492-1552) does not call dt_iop_gui_update() -- the only dt_iop_gui_update() sweep over modules is in dt_dev_pop_history_items() (src/develop/develop.c:1820), which a power-button toggle does not reach. Two consequences:

  • Dead button. Enable colorharmonizer, let the preview finish (button becomes sensitive, tooltip "analyze the image's hue distribution..."), then switch the module off. histogram_valid stays TRUE and the button stays sensitive, but _auto_detect_callback() now computes valid = histogram_valid && self->enabled = FALSE and returns at :1395 with no feedback. Before this commit the same click applied the detected harmony and enabled the module: dt_dev_add_history_item(self->dev, self, TRUE) sets module->enabled = TRUE (src/develop/develop.c:1354-1356). That is an introduced behavioural regression, and it is not mentioned in the commit message or the release note.
  • Stuck-insensitive button (narrow race). gui_update() maintains the invariant "button insensitive => histogram_valid == FALSE" because it clears the flag at :1168 whenever the module is disabled. _auto_detect_button_enable_idle() does not: if the module is switched off in the window between _update_histogram()'s g_idle_add() (:305, on the preview-pipe thread) and the idle's dispatch on the GUI thread, the idle leaves the button insensitive while histogram_valid remains TRUE. Re-enabling the module then reruns process(), but was_invalid is FALSE (:298), so no idle is scheduled and the button stays insensitive with the stale "not yet available" tooltip until some unrelated event triggers gui_update() (undo/redo, history navigation, reset, image switch). Reachability is narrow -- it needs a toggle inside a millisecond-scale window -- but the state is reachable and self-sustaining.

  Minimal correction, one of: (a) drop the && self->enabled term from the click guard at :1390 and let histogram_valid alone gate it, restoring the previous "auto-detect turns the module on" behaviour; or (b) keep the coupling and refresh the button when enablement changes -- call _set_auto_detect_button_state() from the same place the flag is invalidated, and make _auto_detect_button_enable_idle() clear histogram_valid when !self->enabled, so it upholds the same invariant gui_update() does.

[masterpiga]

I made so that the auto-detect button is always enabled, and if the histogram is not cached yet it first computes and then detects the harmony.

[TurboGit]

Thanks!