b4n commented on this pull request.

Overall, why not.

@@ -2440,20 +2441,32 @@ void ui_init_builder(void)

gtk_builder_set_translation_domain(builder, GETTEXT_PACKAGE);

error = NULL; - interface_file = g_build_filename(app->datadir, "geany.glade", NULL); - if (! gtk_builder_add_from_file(builder, interface_file, &error)) + ui_data = g_resource_lookup_data(geany_get_resource(), + "/org/geany/Geany/geany.glade", G_RESOURCE_LOOKUP_FLAGS_NONE, &error); + if (ui_data == NULL)

what about combining this with the check below with a `||`, as the body is almost totally the same (but for the `ui_data` unrefing part)

@@ -2440,20 +2441,32 @@ void ui_init_builder(void)

gtk_builder_set_translation_domain(builder, GETTEXT_PACKAGE);

error = NULL; - interface_file = g_build_filename(app->datadir, "geany.glade", NULL); - if (! gtk_builder_add_from_file(builder, interface_file, &error)) + ui_data = g_resource_lookup_data(geany_get_resource(), + "/org/geany/Geany/geany.glade", G_RESOURCE_LOOKUP_FLAGS_NONE, &error);

Too bad we can't use [`gtk_builder_add_from_resource()`](https://developer.gnome.org/gtk3/stable/GtkBuilder.html#gtk-builder-add-from... maybe we should simply add compatibility for it on GTK < 3.4?

```c #if !GTK_CHECK_VERSION(3, 4) guint gtkcompat_builder_add_from_resource(GtkBuilder *builder, const gchar resource_path, GError **error) { guint success = 0; GBytes *bytes = g_resources_lookup_data(resource_path, G_RESOURCE_LOOKUP_FLAGS_NONE, error); gsize bytes_size;

if (bytes) success = gtk_builder_add_from_string(builder, g_bytes_get_data(bytes, &bytes_size), bytes_size, error);

return success; } #define gtk_builder_add_from_resource gtkcompat_builder_add_from_resource #endif ```

codebrainz commented on this pull request.

@@ -2440,20 +2441,32 @@ void ui_init_builder(void)

gtk_builder_set_translation_domain(builder, GETTEXT_PACKAGE);

error = NULL; - interface_file = g_build_filename(app->datadir, "geany.glade", NULL); - if (! gtk_builder_add_from_file(builder, interface_file, &error)) + ui_data = g_resource_lookup_data(geany_get_resource(), + "/org/geany/Geany/geany.glade", G_RESOURCE_LOOKUP_FLAGS_NONE, &error);

Too bad we can't use gtk_builder_add_from_resource()… maybe we should simply add compatibility for it on GTK < 3.4?

Yeah. Alternatively we could just add a comment there saying to change the code when the needed version is minimum supported.

I think this specific (example) code is missing an `#else`, and also I think the `gtk_build_add_from_string` call is not OK because of the undefined ordering of parameter evaluation in C (which is why I did it with two calls in the patch).

b4n commented on this pull request.

@@ -2440,20 +2441,32 @@ void ui_init_builder(void)

gtk_builder_set_translation_domain(builder, GETTEXT_PACKAGE);

error = NULL; - interface_file = g_build_filename(app->datadir, "geany.glade", NULL); - if (! gtk_builder_add_from_file(builder, interface_file, &error)) + ui_data = g_resource_lookup_data(geany_get_resource(), + "/org/geany/Geany/geany.glade", G_RESOURCE_LOOKUP_FLAGS_NONE, &error);

Yeah. Alternatively we could just add a comment there saying to change the code when the needed version is minimum supported.

What I like with the gtkcompat thing is that we just have to drop the compat code when it's not needed anymore, we don't need to touch caller code.

I think this specific (example) code is missing an `#else`

It's not, as it's replacing the GTK API with the `#define`, so the else if just the normal GTK 3.4 call.

`gtk_build_add_from_string` call is not OK because of the undefined ordering of parameter evaluation in C (which is why I did it with two calls in the patch).

Good point

b4n commented on this pull request.

@@ -2440,20 +2441,32 @@ void ui_init_builder(void)

gtk_builder_set_translation_domain(builder, GETTEXT_PACKAGE);

error = NULL; - interface_file = g_build_filename(app->datadir, "geany.glade", NULL); - if (! gtk_builder_add_from_file(builder, interface_file, &error)) + ui_data = g_resource_lookup_data(geany_get_resource(), + "/org/geany/Geany/geany.glade", G_RESOURCE_LOOKUP_FLAGS_NONE, &error);

I'm on it, give me a sec and you won't have to bother ;)

b4n commented on this pull request.

@@ -2440,20 +2441,32 @@ void ui_init_builder(void)

gtk_builder_set_translation_domain(builder, GETTEXT_PACKAGE);

error = NULL; - interface_file = g_build_filename(app->datadir, "geany.glade", NULL); - if (! gtk_builder_add_from_file(builder, interface_file, &error)) + ui_data = g_resource_lookup_data(geany_get_resource(), + "/org/geany/Geany/geany.glade", G_RESOURCE_LOOKUP_FLAGS_NONE, &error);

@codebrainz https://github.com/b4n/geany/tree/codebrainz/gresource

kugel- requested changes on this pull request.

@@ -98,6 +98,26 @@ G_BEGIN_DECLS

#endif

+#if !GTK_CHECK_VERSION(3, 4, 0) +static inline guint gtkcompat_builder_add_from_resource(GtkBuilder *builder, + const gchar *resource_path, GError **error) +{ + GBytes *bytes = g_resources_lookup_data(resource_path, + G_RESOURCE_LOOKUP_FLAGS_NONE, error); + if (bytes == NULL)

This is worth a a g_error(), as it means Geany wasn't compiled correctly.

• --target=$@ \
  

+ --sourcedir=$(srcdir) \ + --generate-source \ + --c-name="geany" \ + --manual-register \ + --internal \ + $(srcdir)/geany.gresource.xml +resources.h: $(resource_dependencies) + $(AM_V_GEN)$(GLIB_COMPILE_RESOURCES) \ + --target=$@ \ + --sourcedir=$(srcdir) \ + --generate-header \ + --c-name="geany" \ + --manual-register \ + --internal \ + $(srcdir)/geany.gresource.xml

This runs glib-compile-resources twice. It's usually better to a construct pattern rule, where (GNU) make recognizes that one rule produces multiple output files. That's assuming it's even possible with, I haven't checked.

``` %.gresource.c %.gresource.h: %.gresource.xml $(top_srcdir)/data/geany.glade […] ```

b4n requested changes on this pull request.

Apart from the extra include, looks good and seems to work well.

@@ -42,6 +42,7 @@

#include "msgwindow.h" #include "prefs.h" #include "project.h" +#include "resources.h"

Still not needed :) Could you remove this extra include please?