Skip to content

Conversation

victoryforce
Copy link
Collaborator

This PR is one of the steps towards completing the transition to new boxing/layouting functions, which was started by @dterrahe in #18273. I hope to find time to continue this conversion in our code, although I don't plan to complete it myself without help from others (these are simple changes but there are just a lot of them).

Code reformatting in this source file was also piggybacked into the PR.

The last commit adds specific formatting for the expression in the "if" for better readability. That formatting doesn't follow our established style, but it does make it much clearer to me in that expression. So this commit can be removed if such style is controversial.

@TurboGit TurboGit added this to the 5.4 milestone Jul 23, 2025
@dterrahe
Copy link
Member

At the moment of course this change is obviously correct (since it just refactors the call to gtk_container_add to be embedded in dt_gui_box_add.

However, the intention of #18273 was to eventually allow replacing that gtk_container_add in dt_gui_box_add with gtk_box_append during the switch to gtk4, which doesn't have containers or generic widget "add" functions anymore. That won't work if dt_gui_box_add is also used for non-GtkBox type widgets, like GtkScrolledWindow here which would require gtk_scrolled_window_set_child. So the code would need to be fixed up during the transition again anyway. You could use gtk_scrolled_window_add_with_viewport for now, just to be able to get rid of this gtk_container_add, but that would still need to be replaced again later.

I would suggest using dt_gui_box_add only to replace gtk_box_pack_... calls (of which there are many) and the rare gtk_container_add where the first parameter is a cast GtkBox. A GTK_IS_BOX check could have been added in dt_gui_box_add to catch unintended uses, but at the time I didn't think imposing a runtime cost was needed. Feel free to revisit.

@dterrahe
Copy link
Member

#19118 now adds such a check. gtk.h already contains a cast to GTK_BOX, but unfortunately these only get tested in debug builds; this PR would generate a runtime assert then.

@dterrahe
Copy link
Member

dterrahe commented Aug 4, 2025

@victoryforce #19118 includes the two cases of container_add adressed here. Are you OK with the approach taken there? If so, maybe you could remove the first commit from this PR and leave just the code reformatting (which is the largest part of the work done here anyway).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants