scopes: updates to be GTK4 ready#20465
Conversation
- Use dt_gui_{h,v}box() instead of gtk_box_new()
- Use dt_gui_box_add() instead of gtk_box_pack_{start,end}()
To make GTK 4 ready.
Be a bit hacky and reset the propagation phase as
the default phase in dt_gui_connect_motion() doesn't. Pull the
GdkEvent by hand to fix edge case of when user clicks between buttons
and a spurious leave event occurs.
Also clean up math in color harmony scroll behaviors -- replace
if/else with one-liner modulos.
Also reorder the _drawable_button_{press,release} functions to align
with order in which they are called.
Convert bauhaus _widget_scroll() to use GtkEventControllerScroll instead of GdkEventScroll. - If ignoring scroll, pass it on to the scrolled window. - Use delta from event controller, not from raw event - In scopes use discrete scroll events from controller, which aligns with how bauhaus does things and gives fine results Pass a GtkEventController to handle_event aka dt_dev_exposure_handle_event(). Also: Fix bauhaus memory leak by freeing events returned by gtk_get_current_event(). See: https://docs.gtk.org/gtk3/func.get_current_event.html, "The caller of the function takes ownership of the data, and is responsible for freeing it." Also: new dt_gui_connect_scroll() helper function.
- Use GtkGesture to handle clicking of mode buttons. - Use click event for color harmony buttons, Instead of button-press-event. - Use event controller for enter/leave color harmony buttons
TurboGit
left a comment
TurboGit
left a comment
There was a problem hiding this comment.
Different issues with this PR:
- the currently selected harmony icon is not light-up.
- sometime the selected harmony icon is not light-up.
- sometime the selected harmony is not displayed.
- sometime I have two harmony icons displayed.
- changing the vectorscope side crashed dt.
To reproduce: click on icons on the histogram display
Drat! I'm not seeing this here. Is this occurring on the split ("waveform/vectorscope") mode or in the vectorscope-only mode? I'm trying to think out what other differences I could have in my setup which isn't allowing me to see this...
Is there a helpful backtrace, maybe that could be a clue? |
It was on all modes I think for the vectorscope issue. I just tried again and it is a bit better, now I have the selected the harmony icon but still not the current histogram mode icon. And switching the vectorscope mode I had a freeze (5s) when clicking on the Luv icon... but I was not able to reproduce starting Darktable a second time to do a video. So something fishy to me, possible some memory corruption.
No, probably a loop somewhere as I had to hard kill Darktable process. |
Quite possible. I'll look at things more carefully. Do you recall how far you were able to get before failure? E.g. any of:
Not to ask you to bisect, but I believe that the first commit, d0992aa, should be utterly harmless. I'm assuming one of the later commits is the problem. I don't think the current implementation of the color harmony buttons is ideal (I'd rather see it a combobox than a homegrown radiobutton/scrolling hybrid), but I'd still rather have it not crash! |
Maybe 10 clicks, and probably doing at least once the 5 points above. Are you at least able to reproduce the current histogram selected icon no being highlighted? This is 100% reproducible on my side. The first fly over shows it selected but as soon as I select another histogram the status is lost. |
You're right, I can reproduce this! So far as I can see, 215911f is OK, but the final commit, 01e7145 broke this when I switched from using "button-press-event" to "clicked" event. I think there's also something wrong in that commit about handling of color harmony buttons. More in a bit... (I still can't reproduce crash/loop or problems with color harmony buttons.) |
- Handle mode and color harmony buttons via toggled handler. This is simpler than using gestures, and actually works. - Use signal blocking to group mode toggle buttons: Create a radio-button-like behavior of the GtkDarktableToggleButton group which allows for choosing scope modes. Whenever a user clicks on a mode/harmony toggle button, set the prior mode toggle button to inactive. When a user clicks on the currently selected mode toggle button, keep it active. When user clicks on the currently selected harmony button, turn off the harmony overlays for now. Use signal blocking to avoid toggle-handler re-entering. - Move mode activate button widgets out of own array and into dt_scopes_mode_t. - Rename "waveform/vectorscope" mode to "split". It is less verbose, and more accurate to the intent of split mode (which can be generic for multiple modes). - Eliminate crash at startup when showing vectorscope or split. At startup there is no current image, so don't store the color harmony guide state to NULL memory. - Don't use offset indexes into color_harmony_button array, just keep first element unused. - Don't use color_harmony_old and motion events for mouseover preview of color harmonies, instead take advantage of widget prelight behavior. - Now that scope scroll events are discrete, simplify the scrolling code. - Eliminate _color_harmony_button_on() helper. It is a relic of the prior signal architecture. We know what button we're turning off/on so don't have to loop through all buttons. - Use gtk_toggle_button_set_active() when harmony overlay (in scrolling or cycling or when loading new image settings). This will automatically turn off prior harmony. Need a bit of a special case to go to no harmony, in which case toggle off prior harmony. - Merge _update_color_harmony_gui() into _vec_signal_image_changed() and don't call it from _vec_mode_enter() as it isn't needed there. - In gui_init(), don't set the active mode until we have initialized more of the mode. Otherwise GTK will reference not-yet-extant widgets. - Don't bother to set initial harmony button setting as it will always change when load image. This should be fairly GTK4 ready, while using modern GTK3. For GTK4, gtk_toggle_button_set_group() will allow for getting rid of the signal blocking code.
|
Many fixes in the most recent commit. I can't guarantee that there might be other subtle errors, but this should be an improvement. |
TurboGit
left a comment
TurboGit
left a comment
There was a problem hiding this comment.
Thanks, looking good and I have not been able to crash it!
2 participants
dt_gui_{h,v}box()anddt_gui_box_add()for box init/packingclickedevent (which exists in GTK 4) rather thanbutton-press-event.dt_dev_exposure_handle_event()and always pass an event controller. Previously, in the case of scrolling, it would receive a GDK event.dt_gui_connect_scroll()GdkEventretrieved bygtk_get_current_event()without corresponding call togdk_event_free().This PR should produce no user-visible changes, but will allow for more maintainable and forward-compatible code.
There's a lot of finicky bits in bauhaus, particular about mapping events to actions. I tested everything I could find about this, but the biggest risk of this PR would be causing regressions there.