initial conversion to gtkgestures ("gtk4") and other experiments

[dterrahe]

Replacing #29 which became a mess due to rebasing.

This started as a general regexp cleanup to reduce boiler plate in pointer casting (especially in event handlers).

But then I wanted to experiment with transitioning to gtkgestures in preparation for gtk4 (and also to improve support of touchscreens under gtk3). Since there was so much overlap (in the function prototypes) I continued in this branch.

Obviously this has parallels to darktable-org#16919. So what are some of the differences?

• no memory leak. In gtk3 widgets do not own their associated event controllers, so they need to be cleaned up explicitly when the widget is destroyed. This can be done easily using g_object_weak_ref. The final port to gtk4 would simply adjust the code in dt_gui_connect_... to use gtk_widget_add_controller instead.
• the signal prototypes for gesture button_press and _release do not return a boolean to indicate whether the event has been consumed, like the widget event signals used to do. Instead, one needs to explicitly claim the gesture in order for it to stop propagating (otherwise a right click on a graph might delete a node and call up the presets menu) and to receive further (double) clicks or releases. This also clearly shows the danger when migrating; signal handlers are always cast to GCallback so there is no typechecking that catches changed interfaces. This only shows during testing when functions either malfunction or crash.
• for the converted graphs and bauhaus widgets, both button press/release and motion/enter/leave have been converted.
• an attempt has been made to avoid using constructs that would only work in gtk3. For example there's no explicit mention of GtkGestureMultiPress in the signal handlers, because gtk4 renames it to GtkGestureClick so a final port would have to revisit everything again. Instead, the signal source is passed as a pointer to one of the parents (GtkGesture or GtkGestureSingle, depending on what is convenient). In many cases, rather than continuing to use fields of the event object directly (after retrieving it using gtk_gesture_get_last_event), which will not work anymore in gtk4, alternatives have been used (where their API will be stable). For example using the x and y signal arguments, rather than event fields.

This very limited first step clearly shows how much work is involved. Though there is already an improvement in touch screen behavior (tested/tried only on Windows) clearly much more porting needs to be completed before it becomes useful. However, at the same time it seems infeasible to maintain this as a separate PR. Merges like darktable-org#17161, besides being mostly unnecessary, cause significant work in rebasing.

Deprecated modules will have to be converted too (and have been, in this limited PR), but testing them is not easy (and won't happen organically before/after merging a PR). "Luckily" a lot of the graph/curve code has been copy/pasted between modules, so much of the conversion was done on the auto pilot and has at least some chance of working.

What this exercise, as many of the previous ones I've worked on, also clearly shows is that any large maintenance work on the codebase benefits from a lot of cleanup work first. Especially deduplication/refactoring and coding style uniformity massively help (and actually are where most of the work goes). Often patches have been applied over patches, when refactored code might have been much more trivially ported. For example some of our custom widgets could have sent their own custom events rather than linking to standard gtk button press/release events (which is often done to catch modifier settings from the event->state, which gtk4 does not allow). I'm specifically thinking of GtkDarktableToggleButton which could support control&shift/right-click/long-press features directly rather than letting all the users deal with it (and possible touches as well). However, restructuring while porting introduces more opportunities for errors to creep in and mindlessly converting 10 cases is often faster than first unifying them and then converting once. Perpetual technical debt.

@TurboGit I know you are currently very busy so don't expect any in-depth response, but are you at all interested in this PR even in this limited format? It would have to be prioritised over other PRs that touch significant gtk code in order to be manageable.

@jenshannoschwalm @ralfbrown included you both to see how much interest there would be to carry this forward. Clearly this is not a one man effort (or at least not this man).

[dterrahe]

@TurboGit @jenshannoschwalm @ralfbrown

Started an experiment with preparing to migrate gtkbox to gtk4. Again, due to all other gtk related changes here, it made more sense to base it off this "fork". Gtk4 doesn't have the child expand/fill flags anymore that get passed in gtk_box_pack_start. Instead this is managed via gtkwidget hexpand etc. Which makes the process of adding widgets to boxes uniform and allows using generic helper functions that would be the "only" place to make the final change of moving from gtk_container_add to gtk_box_append. Those helper functions can also be nested, so often a multi-step definition of self->widget is no longer needed. See for example

darktable/src/imageio/storage/piwigo.c

Lines 1149 to 1158 in c4fcd6f

	self->widget = dt_gui_vbox
	(ui->account_list,
	dt_gui_hbox(dt_ui_label_new(_("server")), ui->server_entry),
	dt_gui_hbox(dt_ui_label_new(_("user")), ui->user_entry),
	dt_gui_hbox(dt_ui_label_new(_("password")), ui->pwd_entry),
	login_button, ui->status_label, ui->permission_list,
	dt_gui_hbox(ui->album_list, refresh_button),
	ui->create_box,
	dt_gui_hbox(dt_ui_label_new(_("filename pattern")), ui->filename_pattern_entry),
	ui->conflict_action);

To me this new approach would be beneficial even if the migration to gtk4 never happened as it makes defining "standard" ui widgets easier. (I'm sure the more complex ones and very tailored dialog boxes will still be a hassle, as they were before).

The commit "prepare to migrate box" shows a set of examples of what this conversion would look like, but of course there's no point in progressing this further if there is no interest.

[ralfbrown]

I'm about to disappear for two months due to work, but wouldn't mind looking at it again in spring/summer 2025.

[jenshannoschwalm]

1. For me all the "maintenance work" (standardize on g, avoid casting) would be extremely important/helpful as code would be far more readable for us all so i think we should do that asap. There seems to be enough time until next release to fix possible issues ... Will you do a pr on main darktable or would you want me to test on your fork?
2. About Gtk code. As you know i simply don't understand gtk and don't know anything about implications of gtk3/4 specifics. I trust on what you do but unfortunately i won't be able to help you on this part.

[dterrahe]

Not doing a PR, no. Please feel free to cherry-pick whatever you like. I have rebased the maintenance work, but I haven't at all checked if it is still "complete". I.e. if they old patterns have been reintroduced with recent commits, they'll have to be cleaned up again. Also, there's probably more maintenance to do...

I've just been doing the gtk stuff on top of this because there's so much dependency/overlap, but really for gtk3 cleanup/gtk4 preparation I'm just playing around and giving some suggestions on how this could be done without simply doing a line-by-line translation, which would lead to a very inefficient end result. Really, some stuff needs to be redesigned to take advantage of improved gtk4 stuff rather than setting the current status quo in stone. For the gestures, for example, since everything needs to be fixed anyway, why not think about how left/right/ctrl etc clicks (and touch support) really should be done and then implement that everywhere consistently. Looks like popup menu's will need a lot of work too, so same there.

[jenshannoschwalm]

Would you comment on this ? We reduce number of parameters - superfluous?

[dterrahe]

overzealous :-)

According to the signal signature, the user_data parameter should be there. But since it (currently) isn't used, there is no impact.

tbh don't know what I was thinking (if anything) when I edited this, except that in this case the "self" parameter is confusing, since it is actually the first parameter passed from develop.c when it throws the signal DT_SIGNAL_DEVELOP_MODULE_REMOVE and the user_data parameter is the real self (i.e. the overlay module) set by DT_DEBUG_CONTROL_SIGNAL_CONNECT.

So, I may be completely mistaken, but it looks to me that this handler tries to interpret any module being removed as if it was an overlay and looks up the imgid field (which would mean something completely different in another module's blob) and if it happens to be a valid id and used in an overlay, it will be dt_overlay_removed (which might be bad?). If however this is the current module being removed, then it probably does the right thing. I imagine there will be a bug when there is more than one overlay module and only one of them is removed, but haven't tested because I don't know what the impact of dt_overlay_remove is supposed to be.
@TurboGit ?

What doesn't help here is that the definition of in DT_SIGNAL_DEVELOP_MODULE_REMOVE in signal.c is actually marked incorrectly with DT_SIGNAL_MODULE_REMOVE, so not easy to find. This is fixed in 85956e1

[jenshannoschwalm]

In intensive care a overzealous doc is likely harming :-)

As this signal is special to this module i think self/user_data will always be a ptr to an overlay ...

Yes i have seen (and tried the signal fixing commit). This PR is by far large enough to be reviewed. I looked at my watch while enjoying and keeping concentrated , 2.5 hrs :-)

[dterrahe]

btw bit confused by your comment in PR additional fixes commit "A few places where i think we should keep the checks for allocations even if allocations errors are very unlikely due to requested size." since I don't see those removals in my "local" PR. Maybe they were introduced by rebasing/cherry-picking (which is always tricky/dangerous)?

You did a very good read-through to catch all the other fixes!

[dterrahe]

In intensive care a overzealous doc is likely harming :-)

You are absolutely right and that's exactly how I meant it! ;-)

As this signal is special to this module

It isn't? It is sent to history.c for any module being removed.

i think self/user_data will always be a ptr to an overlay ...

user_data yes, but not the second ("self") pointer. I suggest changing this to

static void _module_remove_callback(gpointer instance,
                                    dt_iop_module_t *removed,
                                    dt_iop_module_t *self)

and fixing the code accordingly. Probably by adding a check removed == self, but that's up to @TurboGit

[jenshannoschwalm]

Isn't overlay the only module asking to receive it - at least in current code?

[dterrahe]

$ grep CONNECT.*DT_SIGNAL_DEVELOP_MODULE_REMOVE src -R
src/iop/overlay.c:  DT_DEBUG_CONTROL_SIGNAL_CONNECT(darktable.signals, DT_SIGNAL_DEVELOP_MODULE_REMOVE,
src/libs/history.c:  DT_DEBUG_CONTROL_SIGNAL_CONNECT(darktable.signals, DT_SIGNAL_DEVELOP_MODULE_REMOVE,

Those 2.5hrs must have been exhausting... ;-)

EDIT: or in case you meant only iop module; it still means that each overlay instance receives this message for each module being removed. It doesn't just receive it if this module itself is removed. So simply looking if the removed module is an overlay (or whether the random number in its params that happens to be in the same spot as the imageid in an overlay module is a valid imageid) is not sufficient.

Or maybe I'm just completely confused in which case just ignore me; I haven't looked at, or used, overlays myself.

[jenshannoschwalm]

Well your work certainly took longer... It's just if I push that PR I wanted to be certain about what's happening.

Will check the overlay code an propose a correct solution.

[jenshannoschwalm]

Nice, Pascal has merged that big first part....

[jenshannoschwalm]

Just a note: your signals work (rebased on current dt code so minor "missings") has also been merged

[jenshannoschwalm]

Diederik, as you won't do the pr i cherry-picked the remaining 3 commits to a local branch, rebased on master without any changes, and tested here. Could not spot a single issue!

Also read through the code, for a non-gtk-speaking person like me it seems to me we reduce and simplify code significantly so for sure it's worth to integrate now, whoever might work on progress to gtk4 (definitely it won't be me and from the commit history related to gtk i fear you are the person knowing by far most how that could be done :-)

I feel a bit uneasy yet about pushing the PR as i might not be able to fix resulting issues or even discuss details. I would certainly need your support! Give me a "GO i will assist" before opening.

[dterrahe]

These GTK4 experiments are just that; experiments. They give an indication of how much work is involved and might give somebody inspiration to get involved. But this should be someone who really knows gtk3&4 and has a thorough understanding of how darktable works and ideally how it abuses gtk to do it. But very few people do anymore, so it may just need somebody to spend a lot of time getting up to speed. It is impossible to test changes if you don't understand how the program is supposed to work; quite often the effects of a ui interaction happen somewhere quite invisible.

So imho merging the commits here is pointless because most of the similar cases have not be converted yet and the way development (especially gtk/ui related) in darktable happens is that the non-converted cases will continue to be copied over.

I toyed with the idea of submitting this as a draft PR marked "RFA" (request for adoption). That would possibly give a somewhat larger chance that somebody will get involved and do the remaining work. But tragically the open PR list gets hardly any attention from anyone besides the three core developers, so I'm not holding out high hope. For this reason I do not believe that a full gtk4 transition has any chance of happening.

For fun I pushed one additional commit here that makes it possible to compile the codebase with Gtk4 instead of Gtk3 (after installing the necessary development libraries, obviously. I've only tested this under debian). Any deprecated/removed calls have been defined as macros in darktable.h so that they don't break compilation. Obviously it immediately crashes when you try to run the resulting binary. But at least this gives some idea of how many different aspects of the codebase need changing. Any of these functions should not be robotically be replaced by their "equivalent" gtk4 version but instead should be restructured to take advantage of new approaches or simply because old workarounds might not longer work or be needed.

I would certainly need your support!

I am still not planning on getting involved to that extend. In hindsight I consider submitting darktable-org#17451 to have been a mistake.

[TurboGit]

For this reason I do not believe that a full gtk4 transition has any chance of happening.

That means that darktable will not be available in some years from now. So I hope for the opposite :)

[dterrahe]

So I hope for the opposite

I'd like to have hope too, but it is unreasonable to base that on the efforts of one burned out developer. I've indicated what I feel would be required to achieve the goal. It is not impossible for new people to learn gtk, if they are willing to put the effort in. It doesn't require expert level to take the recipes I've suggested in this PR and apply them to the rest of the codebase. I've tried in my past PRs to provide explanations for people to learn along, but have no idea if anybody ever read them. Similarly, anybody can be involved in testing and feedback. But the attitude on github and pixls always is "I'll test it when it's done" and then I'll offer my superior insight and opinions. That may work for smaller enhancements, especially when one is in a position to ignore the feedback and merge anyway, but not for a two year effort.

Have you looked at

darktable/src/common/darktable.h

Lines 1315 to 1556 in 1368824

	#define cairo_pattern_get_rgba(...) NULL
	#define g_date_time_get_day_of_month(...) 0
	#define gdk_cairo_surface_create_from_pixbuf(...) NULL
	#define gdk_cursor_new_for_display(...) NULL
	#define gdk_cursor_new_from_name(...) NULL
	#define gdk_cursor_new_from_surface(...) NULL
	#define gdk_device_get_axis_use(...) 0
	#define gdk_device_get_mode(...) 0
	#define gdk_device_get_n_axes(...) 0
	#define gdk_device_get_n_keys(...) 0
	#define gdk_device_get_state(...) NULL
	#define gdk_device_get_window_at_position(...) NULL
	#define gdk_display_get_default_cursor_size(...) 0
	#define gdk_display_get_monitor_at_window(...) NULL
	#define gdk_display_get_monitor(...) NULL
	#define gdk_display_get_n_monitors(...) 1
	#define gdk_drag_status(...)
	#define gdk_event_free(...)
	#define gdk_event_get_axis(...)
	#define gdk_event_get_keyval(...)
	#define gdk_event_get_pointer_emulated(...) 0
	#define gdk_event_get_source_device(...) NULL
	#define gdk_event_handler_set(...)
	#define gdk_event_new(...) NULL
	#define GDK_IS_WAYLAND_DISPLAY(...) 0
	#define gdk_keymap_get_entries_for_keyval(...) 0
	#define gdk_keymap_get_for_display(...) NULL
	#define gdk_keymap_get_modifier_mask(...) 0
	#define gdk_keymap_get_modifier_state(...) 0
	#define gdk_keymap_translate_keyboard_state(...) 0
	#define gdk_monitor_get_workarea(...)
	#define gdk_property_get(...)
	#define gdk_screen_get_default(...) NULL
	#define gdk_screen_get_resolution(...) 0
	#define gdk_screen_get_rgba_visual(...) NULL
	#define gdk_screen_set_resolution(...)
	#define gdk_seat_get_slaves(...) NULL
	#define gdk_seat_grab(...)
	#define gdk_seat_ungrab(...)
	#define gdk_threads_add_idle(...)
	#define gdk_window_get_cursor(...) NULL
	#define gdk_window_get_device_position(...)
	#define gdk_window_get_display(...) NULL
	#define gdk_window_get_height(...) 0
	#define gdk_window_get_origin(...)
	#define gdk_window_get_state(...) 0
	#define gdk_window_get_toplevel(...) NULL
	#define gdk_window_get_user_data(...)
	#define gdk_window_get_width(...) 0
	#define gdk_window_move_to_rect(...)
	#define gdk_window_move(...)
	#define gdk_window_resize(...)
	#define gdk_window_set_cursor(...)
	#define gdk_window_set_transient_for(...)
	#define gtk_bin_get_child(...) NULL
	#define gtk_bin_get_child(...) NULL
	#define gtk_box_pack_end(...)
	#define gtk_box_pack_start(...)
	#define gtk_box_query_child_packing(...)
	#define gtk_box_reorder_child(...)
	#define gtk_box_set_center_widget(...)
	#define gtk_button_box_set_child_non_homogeneous(...)
	#define gtk_button_clicked(...)
	#define gtk_calendar_get_date(...)
	#define gtk_calendar_mark_day(...)
	#define gtk_calendar_select_month(...)
	#define gtk_check_menu_item_get_active(...) 0
	#define gtk_check_menu_item_new_with_label(...) NULL
	#define gtk_check_menu_item_set_active(...)
	#define gtk_check_menu_item_set_inconsistent(...)
	#define gtk_clipboard_set_text(...)
	#define gtk_container_add(...)
	#define gtk_container_child_get_property(...)
	#define gtk_container_child_get(...)
	#define gtk_container_child_set(...)
	#define gtk_container_foreach(...)
	#define gtk_container_get_children(...) NULL
	#define gtk_container_get_focus_child(...) NULL
	#define gtk_container_remove(...)
	#define gtk_container_set_border_width(...)
	#define gtk_container_set_focus_child(...)
	#define GTK_CONTAINER(...) NULL
	#define gtk_css_provider_load_from_data(...) 0
	#define gtk_dialog_run(...) 0
	#define gtk_drag_begin_with_coordinates(...) NULL
	#define gtk_drag_dest_set(...)
	#define gtk_drag_dest_unset(...)
	#define gtk_drag_finish(...)
	#define gtk_drag_get_source_widget(...) NULL
	#define gtk_drag_set_icon_pixbuf(...)
	#define gtk_drag_set_icon_surface(...)
	#define gtk_drag_set_icon_widget(...)
	#define gtk_drag_source_set(...)
	#define gtk_drag_source_unset(...)
	#define gtk_entry_get_layout(...) NULL
	#define gtk_entry_get_text(...) "test"
	#define gtk_entry_set_max_width_chars(...)
	#define gtk_entry_set_text(...)
	#define gtk_entry_set_width_chars(...)
	#define gtk_event_box_set_visible_window(...)
	#define GTK_EVENT_BOX(...) NULL
	#define gtk_event_controller_motion_new(...) NULL
	#define gtk_file_chooser_add_filter(...)
	#define gtk_file_chooser_button_new(...) NULL
	#define gtk_file_chooser_button_set_title(...)
	#define gtk_file_chooser_button_set_width_chars(...)
	#define gtk_file_chooser_get_current_folder(...) NULL
	#define gtk_file_chooser_get_filename(...) NULL
	#define gtk_file_chooser_get_filenames(...) NULL
	#define gtk_file_chooser_get_uri(...) NULL
	#define gtk_file_chooser_select_filename(...)
	#define gtk_file_chooser_set_current_folder(...)
	#define gtk_file_chooser_set_current_folder(...)
	#define gtk_file_chooser_set_current_name(...)
	#define gtk_file_chooser_set_do_overwrite_confirmation(...)
	#define gtk_file_chooser_set_extra_widget(...)
	#define gtk_file_chooser_set_filename(...)
	#define gtk_file_chooser_unselect_all(...)
	#define gtk_file_filter_add_custom(...)
	#define gtk_file_filter_add_pattern(...)
	#define gtk_file_filter_set_name(...)
	#define gtk_font_button_new_with_font(...) NULL
	#define gtk_font_button_set_show_size(...)
	#define gtk_gesture_multi_press_new(...) NULL
	#define gtk_get_event_widget(...) NULL
	#define gtk_grab_add(...)
	#define gtk_grab_remove(...)
	#define gtk_icon_theme_append_search_path(...)
	#define gtk_init_check(...)
	#define gtk_init(...)
	#define GTK_IS_CHECK_MENU_ITEM(...) 0
	#define GTK_IS_CONTAINER(...) 0
	#define GTK_IS_EVENT_BOX(...) 0
	#define gtk_label_set_line_wrap(...)
	#define gtk_layout_move(...)
	#define gtk_layout_new(...) NULL
	#define gtk_layout_put(...)
	#define gtk_main_do_event(...)
	#define gtk_menu_item_get_label(...) NULL
	#define gtk_menu_item_get_submenu(...) NULL
	#define gtk_menu_item_new_with_label(...) NULL
	#define gtk_menu_item_set_submenu(...)
	#define GTK_MENU_ITEM(...) NULL
	#define gtk_menu_new(...) NULL
	#define gtk_menu_popup_at_pointer(...)
	#define gtk_menu_popup_at_widget(...)
	#define gtk_menu_shell_append(...)
	#define gtk_menu_shell_insert(...)
	#define gtk_menu_shell_prepend(...)
	#define GTK_MENU_SHELL(...) NULL
	#define GTK_MENU(...) NULL
	#define gtk_native_dialog_run(...) 0
	#define gtk_overlay_reorder_overlay(...)
	#define gtk_overlay_set_overlay_pass_through(...)
	#define gtk_paned_pack1(...)
	#define gtk_paned_pack2(...)
	#define gtk_parse_args(...) 0
	#define gtk_popover_get_default_widget(...) NULL
	#define gtk_popover_get_relative_to(...) NULL
	#define gtk_popover_new(...) NULL
	#define gtk_popover_set_modal(...)
	#define gtk_popover_set_position(...)
	#define gtk_popover_set_relative_to(...)
	#define gtk_popover_set_relative_to(...)
	#define gtk_propagate_event(...)
	#define gtk_radio_button_new_with_label_from_widget(...) NULL
	#define gtk_scrolled_window_new(...) NULL
	#define gtk_scrolled_window_set_shadow_type(...) NULL
	#define gtk_search_entry_handle_event(...) 0
	#define gtk_selection_data_get_data(...) NULL
	#define gtk_selection_data_get_length(...) 0
	#define gtk_selection_data_set(...)
	#define gtk_separator_menu_item_new(...) NULL
	#define gtk_show_uri_on_window(...) 0
	#define gtk_stack_set_homogeneous(...)
	#define gtk_style_context_add_provider_for_screen(...)
	#define gtk_style_context_get_border(...)
	#define gtk_style_context_get_color(...)
	#define gtk_style_context_get_margin(...)
	#define gtk_style_context_get_padding(...)
	#define gtk_style_context_get(...)
	#define gtk_style_context_list_classes(...) NULL
	#define gtk_style_context_new(...) NULL
	#define gtk_style_context_set_path(...)
	#define gtk_style_context_set_screen(...)
	#define gtk_target_list_new(...) NULL
	#define gtk_target_list_unref(...)
	#define gtk_text_view_add_child_in_window(...)
	#define gtk_text_view_im_context_filter_keypress(...) 0
	#define gtk_tooltip_trigger_tooltip_query(...)
	#define gtk_tree_view_column_cell_get_size(...)
	#define gtk_tree_view_create_row_drag_icon(...) NULL
	#define gtk_tree_view_get_tooltip_context(...) 0
	#define gtk_tree_view_set_search_entry(...)
	#define GTK_TYPE_CONTAINER(...) 0
	#define gtk_viewport_set_shadow_type(...)
	#define gtk_widget_add_events(...)
	#define gtk_widget_destroy(...)
	#define gtk_widget_draw(...)
	#define gtk_widget_event(...) 0
	#define gtk_widget_get_preferred_height(...) 0
	#define gtk_widget_get_preferred_width(...) 0
	#define gtk_widget_get_screen(...) NULL
	#define gtk_widget_get_tooltip_markup(...) NULL
	#define gtk_widget_get_tooltip_text(...) NULL
	#define gtk_widget_get_toplevel(...) NULL
	#define gtk_widget_get_window(...) NULL
	#define gtk_widget_has_grab(...) 0
	#define gtk_widget_path_append_type(...) 0
	#define gtk_widget_path_free(...)
	#define gtk_widget_path_iter_add_class(...)
	#define gtk_widget_path_iter_add_class(...)
	#define gtk_widget_path_new(...) NULL
	#define gtk_widget_set_app_paintable(...)
	#define gtk_widget_set_can_default(...)
	#define gtk_widget_set_events(...)
	#define gtk_widget_set_has_window(...)
	#define gtk_widget_set_no_show_all(...)
	#define gtk_widget_set_visual(...)
	#define gtk_widget_size_allocate(...)
	#define gtk_widget_style_get_property(...)
	#define gtk_widget_translate_coordinates(...)
	#define gtk_window_get_position(...)
	#define gtk_window_get_size(...)
	#define gtk_window_get_window_type(...) 0
	#define gtk_window_move(...)
	#define gtk_window_new(...) NULL
	#define gtk_window_resize(...)
	#define gtk_window_set_attached_to(...)
	#define gtk_window_set_gravity(...)
	#define gtk_window_set_keep_above(...)
	#define gtk_window_set_position(...)
	#define gtk_window_set_transient_for(...)
	#define gtk_window_set_type_hint(...)
	#define gtk_window_set_urgency_hint(...)
	#define gtk_get_current_event(...) (GdkEventOld[]){{GDK_KEY_PRESS}}
	#define gtk_event_box_new() gtk_box_new(0,0)
	static void gtk_widget_destroyed(){};
	static void gtk_main_quit(){};
	static void gtk_main(){};
	static void gtk_widget_show_all(GtkWidget*w){};
	typedef void (*GtkCallback)(GtkWidget *widget, gpointer data);

to understand whats involved? One could JOIN this 242 word list with the codebase to see how many lines (at the minimum) would need to be touched. As mentioned, this should be done while restructuring in order to not end up with an unmaintainable mess full of ancient artifacts that noone knows are needed anymore. Especially in the area of cross-platform compatibility this again comes down to people (on Mac, wherever) being willing to get involved while a transition is ongoing to test solutions as they are being experimented with.

[dterrahe]

@jenshannoschwalm in case you are wondering which changes in ancil are just for my own use and entertainment and which should be generally applicable, I'm listing the relevant ones below:

straight up bug fixes
fix defaults for preference combos
set default for language combo
fix filter range entries tooltips
escape log message
fix right click slider fine tuning under wayland
remove disabled combo items from shortcuts

code base improvements
refactor dt_util_dstrcat to dt_util_str_cat
save shortcuts dialog pane split only when changed

Uncontroversial (probably) functionality improvements/additions
(need testing, obviously)
shift+click snapshot to show difference
scale and format changed values in history list tooltip
don't recalc subset of already available buffer
don't recalc on small zoom changes
turn toggles into actions in metadata and colorpicker
sort shortcuts in h screen by clicking on action header

Opinionated improvements
mirror map cursor keys from darkroom
in mask manager create new group for active module
simplify styles and presets shortcuts tooltip
let preview or module name sizes determine style name width in popup
in shortcuts list change background to indicate which fields can be changed
left align collections combo to avoid jump
(what I mean with this last category is that I think these are clear and worthwhile improvements, but others have decided to implement these things differently, so you may get pushback and discussions that may depress you.)

[jenshannoschwalm]

I am surprised i didn't find the reverts in the last part of the list :-)

Just opened a next PR. IIRC we discussed this

   dt_dev_invalidate(dev); // FIXME not needed? redraw should determines if needs new calculation
    dt_control_queue_redraw_center();

before. I thought about it and tested again, you seem to be right, that invalidate is indeed not required.

[dterrahe]

I am surprised i didn't find the reverts in the last part of the list :-)

Those undo an intentional and democratically decided action to break the carefully designed behavior that was intended to gracefully deal with (initialising and switching between) displays with wildly different resolutions (laptops and 4k screens). I like my vectorscope circles and navigation thumbnail to have constant aspect, but if the majority want to adjust their width and height separately, and do it each time they connect an external monitor, I have no desire to get in their way. That's the beauty of maintaining a "fork" for personal use; it's easy to build something exactly to your own liking. Of course there is the little extra hassle of having to revert each fix for now inconvenient behavior in mainline separately too, but it is a very small price to pay. Please don't, in my name, push anything upstream to change this.

Similarly, I did not list "click on expanded but unfocused module grabs focus" as ready for upstream inclusion. I think that's a personal preference and both approaches have drawbacks. It's only in my personal workflow that my local behavior makes more sense. If the PR gets merged as is, in a few months' time you will get people complaining about breakage and bisect will point at a commit with my name. You could label that PR with default-behavior-change, but it's not really just a default.

"mirror map cursor keys from darkroom" is somewhat different, in that I believe that a lot of what people call "intuitive" is actually just "consistency". When CTRL almost everywhere (as far as I've noticed and fixed) means smaller moves, then in this one specific case using it to make larger moves has the potential to break people's motor memory and mistrust their "instinct" everywhere else in the program. I think this is "bad". Also, implementing the feature as a move action with fallbacks, rather than as hardcoded commands, means that it is fully customizable; you can change the step size or add other modifier combinations and you can assign up/down or left/right moves to, for example, a midi knob in one step, automatically inheriting the shift/ctrl speed adjustments. Of course nobody cares about arrow keys in the map view, so it is largely irrelevant, but to me it's the principle that details matter. I'm still somewhat sad about giving in to making "no fallbacks" the default. It means a lot of people will not benefit from the extra functionality this makes automatically/easily available and makes it harder to assume the same behavior for everyone in documentation. But then, some people don't really want flexible shortcuts and see them as a potentially dangerous inconvenience. I think ansel has mostly stripped them out completely for months now and people there seem very happy about it. More power to them!

[jenshannoschwalm]

Please don't, in my name, push anything upstream to change this.

Absolutely not intention to do so and i am fully aware of the decision process - i think i also commented.

Ok, i'll remove the "focus" commit from the PR. Next time i will make me the author so blaming me would be the result. ok?

EDTI: Would you want me to take "ownership" whenever i open a PR with your work so blaming/bisecting would be on me? (Maybe you told me already and i forgot)

[jenshannoschwalm]

But then, some people don't really want flexible shortcuts and see them as a potentially dangerous inconvenience. I think ansel has mostly stripped them out completely for months now and people there seem very happy about it. More power to them!

How do you know what people think? For sure AP strongly disliked it so i guess that's why it got removed.

I sometimes had a look what he was doing in github and couldn't spot anything noteworthy. No idea how stable ansel is now.

[dterrahe]

Would you want me to take "ownership" whenever i open a PR with your work so blaming/bisecting would be on me?

You are doing the integration work so it would only be fair to take the credit. And then people also would know who to turn to for fixes/changes. But tbh it doesn't matter too much to me, so just do what is less work. And not doing contentious stuff saves a lot of work :-)

How do you know what people think?

Anybody who hasn't left yet or continues donating must be happy with the decisions? I'm all for choice and for giving people who don't like dt's direction other options than just complaining.

couldn't spot anything noteworthy

Some of the stuff he does with the cache is "interesting", but his approach often breaks things because they were "stupid" when really the basic design of the cache never meant to support everything that is done with it now, so workarounds are necessary and the only real solution is a full redesign (after analysis of all current, and likely future, use cases). A lot of work but chasing down each individual breakage is work too and unlikely to improve performance in the long run.

[jenshannoschwalm]

Yes , also saw his work on cache. From what I understood he mainly removed the code... May be wrong

[dterrahe]

Added another commit to ancil (04d2e7a) inspired by 13e2050 and many like it.
This automatically adds a newline to all dt_prints, so there's no need to tediously include it everywhere. I've removed probably like 99% of the explicit \ns, but it is likely I'll have missed some, which would cause empty lines to be printed which I think is less "bad" than multiple lines with their timestamps getting concatenated like might still be happening in mainline sometime (because hard to spot).

There's obviously the possibility that people were using "partial" dt_prints and then used dt_print_nls to complete the line. So far everywhere I spotted this seemed to be the case (i.e. a dt_print without a \n) it seemed to be in error, i.e. the next print was a dt_print (including the timestamp again on the same line). Especially camera_control.c could use a review in this respect. I have not attempted that myself.

It is also reasonable to conclude that this horse has bolted and that large trivial refactorings like this are just not worth it.

[jenshannoschwalm]

I check all changes, seem to be worthwhile. I also made sure, a log line doesn't end with a dot - some did, most not.

Will pr this in combination with the also-many-files-touching "dt_util_str_cat" (BTW your commit included changes in lua scripts, i left that out as it would of course require mods in that git.

[dterrahe]

I check all changes, seem to be worthwhile.

I hope you especially checked for dt_print statements that were not fixed ;-)

[jenshannoschwalm]

I hope you especially checked for dt_print statements that were not fixed

Indeed i missed quite a few, new pr. I think now were done with newlines.

[jenshannoschwalm]

1. A lot of more codebase maintenance got merged. Some dt_print() related
2. Something completely else - gtk related. Cou you have a look at issue 17684 ? There is a log with blue screen mentioned and at the end there are some gdk issue. gdk_seat_default_remove_tool Can you make anything out of that. (The system is definitely under massive system mem stress...)

[dterrahe]

Either some device (mouse, pen, whatever) is physically removed from the system or, more likely, memory is getting corrupted causing a tool driver to fail and remove itself.

I find the mention of DwmEnableBlurBehindWindow just before that more interesting, since it could be the cause of the corruption (or the additional memory strain). Sounds to me like windows is applying some blurring effect for which it might be using the GPU (and its memory) without asking anybody. Don't know anything about that though and trying to figure this thing out with just one user who might have a weird configuration (besides it being windows) without even knowing it is just not fun. If there are more people replicating, then maybe. Of course, a blue screen always means that something else (a display driver or windows itself) is messing up besides dt, so that already puts you at a massive disadvantage if you can't replicate the exact system.

Unclear to me "RAM fills up completely. This status takes very long time" is he talking about a slow increase in memory use (i.e. is there a leak somewhere) or just an immediate increase because he's using (two) large diffuse models?

If it used to work before, he could try installing an old nvidia driver.

[jenshannoschwalm]

Unclear to me "RAM fills up completely. This status takes very long time" is he talking about a slow increase in memory use (i.e. is there a leak somewhere) or just an immediate increase because he's using (two) large diffuse models?

It seems his issue is not related to opencl only.

In the bluescreen log we can find this

   185,0620 [memory] max address space (vmpeak):     13283960 kB
            [memory] cur address space (vmsize):      9545468 kB
            [memory] max used memory   (vmhwm ):      4205236 kB
            [memory] cur used memory   (vmrss ):      3917056 Kb

before the DwmEnableBlurBehindWindow happening. On a 8GB system the peak consumption is clearly very high.

Likely due to running two thumbnail pipe in parallel thus doubling consumption not checked in dt pipe processing.

[jenshannoschwalm]

I just saw deduplicate filter menu item lists, fix jumping of color calibration temperature slider and handle drag and scroll in histogram in exposure sliders.

Let me know if anything of those seem to be ready for me to check and possibly for dt master.

[dterrahe]

deduplicate filter menu item lists,

You know unbridled duplication, especially cases like this where large chunks of code need to be kept in sync, is one of my bugbears. So I can't help myself "fixing" them when I come across one. But there's really no point because new cases are committed into the codebase faster than anybody can keep up with.

fix jumping of color calibration temperature slider

I think this works, but I may not be aware of other cases where the result of a pipe run could cause a need for refreshing the sliders (since AI was removed). Also, of course haven't tested reenabling AI again with this change.

and handle drag and scroll in histogram in exposure sliders

There's a bug when clicking on the histogram while the exposure module sliders haven't been realized yet (i.e. they haven't been shown at least once). This causes the colour picker to be enabled (since width = 0 the click is always in the right-most area). Should be easy to fix but haven't prioritised.

Personally I would prioritise scale and format changed values in history list tooltip, in mask manager create new group for active module, shift+click snapshot to show difference and in shortcuts list change background to indicate which fields can be changed. I think those provide real useful improvements to normal users (rather than coders or compulsives). But maybe you disagree with/don't like them?

[jenshannoschwalm]

But maybe you disagree with/don't like them?

Just didn't look into that in detail.

[jenshannoschwalm]

Maybe a riddle you can solve quickly. If i stop darktable by click-on-close-button pretty soon after start i regularly observe this. We try to join pthreads in a bad situation. Any idea?

Using host libthread_db library "/lib64/libthread_db.so.1".
0x00007fe3ff713da9 in __futex_abstimed_wait_common () from /lib64/libc.so.6
warning: Currently logging to /tmp/darktable_bt_82ROW2.txt.  Turn the logging off and on to make the new setting effective.
#0  0x00007fe3ff713da9 in __futex_abstimed_wait_common () at /lib64/libc.so.6
#1  0x00007fe3ff719403 in __pthread_clockjoin_ex () at /lib64/libc.so.6
#2  0x00007fe404db4d09 in dt_control_shutdown (s=0x3a22e790) at /home/hanno/sources/darktable/src/control/control.c:308
#3  0x00007fe404cf7a86 in dt_cleanup () at /home/hanno/sources/darktable/src/common/darktable.c:2039
#4  0x000000000040117f in main (argc=<optimized out>, argv=<optimized out>) at /home/hanno/sources/darktable/src/main.c:139

[dterrahe]

There's a bug when clicking on the histogram while the exposure module sliders haven't been realized yet

Solved now. It also reversed the direction of a drag. All working now (even when right panel hidden (and histogram is shown in the left). Ready for your testing!

I've also added a toast of the resulting value/the affected slider, like you get when using shortcuts.

[dterrahe]

Any idea?

Seems you are able to interrupt dt_init before it gets to dt_control_init. Maybe you can check darktable.control->running before pthread_join(s->kick_on_workers_thread, NULL); but be careful; it gets set to FALSE early in dt_control_shutdown (and might not be correctly initialised yet either)

[jenshannoschwalm]

Seems you are able to interrupt dt_init before it gets to dt_control_init. Maybe you can check darktable.control->running before pthread_join(s->kick_on_workers_thread, NULL); but be careful; it gets set to FALSE early in dt_control_shutdown (and might not be correctly initialised yet either)

Mmh. It's set to FALSE via calloc initially.

Solved now. It also reversed the direction of a drag. All working now (even when right panel hidden (and histogram is shown in the left). Ready for your testing!

Will do the next days for sure and also the snapshot-difference thing. Just need to cleanup my git branches first due to demosaic cleanup and capture sharpening otherwise i won't be able to keep everything "safe" :-)

But there's really no point because new cases are committed into the codebase faster than anybody can keep up with.

I personally don't think it's that bad atm. Although i confess, besides pipe things i don't look into PR details...

[dterrahe]

@TurboGit not sure what you expect in 17784. This is intended as a simple tool to quickly check if there aren't any unintended large differences or unevenness in large impact areas. If there are, switch back to normal snapshot comparison to zoom in and see if they are good or bad; you can't really judge that in the diff. If there aren't any large differences, then indeed the diff will be dark. You could scale that, but you'd have to be able to adjust it, so large difference areas don't get blown out. Now you are creating a complex tool that people use infrequently which is a ui and bikeshed nightmare causing a lot of wasted time down the line. Then it would be better not to include it at all and spend the time perfecting the clock in the splash screen.

For in depth analysis it might be better to use blend mode "difference" to highlight impact of a module. Then you should be able to easily apply a curve module after to zoom into specific ranges of subtle differences.

[TurboGit]

I took the image from the integration test and just enable the "local contrast" module. This is quite visible on the image, but the display is:

I would have expected something like this:

I just took dt and have added 3EV exposure and curve with contrast in shadows.

EDIT: Done quickly and so probably too much here.

[dterrahe]

If I compare the snapshot diff with a blend mode=difference (on the local contrast module) then it is indeed a little bit dimmer, but not as extreme as your example. So cairo may be using a slightly different blending algorithm or it may be a color space effect. But if you increase the "detail" slider you quickly see a much more pronounced diff. So I'm not sure there's anything wrong. It behaves more or less as I expect.

Again, the intend is to quickly check if there are no unexpected large differences anywhere (much easier to see against a mostly black background than in a possibly very detailed image). I have no intend to build a complex analysis tool (that isn't needed because there are ways to do such analysis with much more control already). If you think that makes the option useless, then I suggest @jenshannoschwalm drops that commit from the PR or closes the PR completely. As I have stated a few times now, I have no intention to have my name linked to anything contentious anymore (and anything where different people might have different ideas of what is "correct" quickly becomes contentious).

[TurboGit]

Yes, my point is that if you need this shift-click to see the diff it is probably because it is somehow subtle and difficult to see otherwise on the image. So I think we need to have some stronger contrast in this case.

[dterrahe]

If you make it brighter for small subtle differences then you blow out big differences. You'd have to make that configurable. Now you need to add shortcuts or modifiers and make them discoverable and not clash with anything else. If you maintain state between invocations, people won't remember they changed something sometime so you need to remind them. @jenshannoschwalm please drop the commit from the PR because l don't see a path forward for it that doesn't involve much more waste of time (now and in future maintenance) than this gimmick is worth. Thank you!

In fact, the other commit has a similar "subtlety" issue, and the people that I'm dreaming might notice it to realize the flexibility of the shortcut system are probably exactly the kind of people who don't pay attention to such details, so probably not worth keeping that either (because it will trigger requests to make it configurable via CSS or whatever).

[TurboGit]

If you make it brighter for small subtle differences then you blow out big differences.

But big differences are already visible on the standard view. My point is really that this "diff" view is more for subtle changes. No?

[dterrahe]

No?

No. It is meant to spot unintended larger changes in areas of much detail where they would be sometimes hard to spot if there are many intended large changes everywhere too. I wonder how you intend to judge whether small changes are good or bad in a diff rather than the full image? The diff is certainly not meant to encourage pixel peeping there.

or unevenness in large impact areas.

Edit: I'm not saying that it wouldn't be nice to improve contrast if there are only small changes. I was asking if you thought through what that means for large changes and the user interface. I'm not hearing what exactly you would want to happen. Like, what would the end state of the tool be like, in your opinion.

But I'd rather stop discussing it because in any case, I'm not going to spend time on "improvements".

[jenshannoschwalm]

Will close the PR.

I'm fully aware btw that all improvements would not work for me, if there is more UI complexity. Also work would be on my side. And no room for User-Tuning, i'm also not interested in that.

[jenshannoschwalm]

Just mentioning, a slightly modified version of your accelarator work has gone into dt master. I chose using bold instead of your background visualizing, think that's good to see but less "offensive".

[jenshannoschwalm]

Solved now. It also reversed the direction of a drag. All working now (even when right panel hidden (and histogram is shown in the left). Ready for your testing!

Yes. Seems all good now :-)