This prevents Geany from failing to run when it can't find the Glade/GtkBuilder UI for whatever reason.
Fixes #1702 indirectly. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1703
-- Commit Summary --
* Use GResource to load GtkBuilder XML UI file
-- File Changes --
M .gitignore (1) M configure.ac (1) M data/Makefile.am (5) A m4/geany-gresource.m4 (7) M src/Makefile.am (27) A src/geany.gresource.xml (6) M src/libmain.c (3) M src/ui_utils.c (27)
-- Patch Links --
https://github.com/geany/geany/pull/1703.patch https://github.com/geany/geany/pull/1703.diff
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 ```
Looks good from cursory inspection, and one less file is good. Isn't there an option on the resource compiler to compress it? We only access the builder once IIUC, so decompression won't slow normal usage down.
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).
Isn't there an option on the resource compiler to compress it?
https://github.com/geany/geany/pull/1703/files#diff-4542cd4ff6b80a759a745621...
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
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); + if (ui_data == NULL)
Could do, I just wanted to show two discrete messages (ie. "load" vs "create"). I don't mind merging them, both are super-fatal.
Isn't there an option on the resource compiler to compress it?
https://github.com/geany/geany/pull/1703/files#diff-4542cd4ff6b80a759a745621...
Oh, its in the xml, well, you should have known I wouldn't have read that :)
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);
What I like with the gtkcompat thing is that we just have to drop the compat code when it's not needed anymore
Seems OK to me. I'll try and get to this.
It's not, ...
Ah, I didn't notice the differing prefixes on the names.
There is also `preprocess="xml-stripblanks"` attribute, but I'm not sure it's good since a) it seems to require an additional non-default-installed tool, and b) the compression ought to clobber all the redundant whitespace anyway.
Yeah, saw on docs it uses xmllint, so didn't mention it.
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); + if (ui_data == NULL)
Oh, I didn't notice that. I don't think it matters much though.
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
b4n commented on this pull request.
- 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) + { + dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, + _("Geany cannot start!"), error->message); + g_error("Cannot load user-interface: %s", error->message); + g_error_free(error); + g_object_unref(builder); + return; + } + + error = NULL; + if (gtk_builder_add_from_string(builder, g_bytes_get_data(ui_data, NULL), + g_bytes_get_size(ui_data), &error) == 0)
You could keep the boolean style condition (`if (gtk_builder_...())`) instead of checking for `0`; it's the working the same yet is a little more straightforward in a boolean-filled world, and the use of `guint` really seem like an API glitch in the GTK side, as it's just documented to return "A positive value on success, 0 if an error occurred".
@codebrainz pushed 1 commit.
6e2a51f Add gtkcompat wrapper for gtk_builder_add_from_resource
@b4n I collided with your change, sorry.
Either:
https://github.com/b4n/geany/commit/0afab963c365d30dff97fb3c59bfa1929a4792db
Or:
https://github.com/geany/geany/pull/1703/commits/6e2a51ff5608d188387c856c9d3...
I don't care which, need to diff them.
codebrainz commented on this pull request.
- 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) + { + dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, + _("Geany cannot start!"), error->message); + g_error("Cannot load user-interface: %s", error->message); + g_error_free(error); + g_object_unref(builder); + return; + } + + error = NULL; + if (gtk_builder_add_from_string(builder, g_bytes_get_data(ui_data, NULL), + g_bytes_get_size(ui_data), &error) == 0)
I like the explicit `== 0` since it shows that the result is not semantically a boolean.
b4n commented on this pull request.
- 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) + { + dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, + _("Geany cannot start!"), error->message); + g_error("Cannot load user-interface: %s", error->message); + g_error_free(error); + g_object_unref(builder); + return; + } + + error = NULL; + if (gtk_builder_add_from_string(builder, g_bytes_get_data(ui_data, NULL), + g_bytes_get_size(ui_data), &error) == 0)
Well, the thing is that from what I understand from the API, it actually has the semantics of a boolean, it seems like the actual value has literally *no* meaning apart from success/failure. I didn't check the code yet, but AFAICT the documentation doesn't say anything about any use for it, and there's no API that would use the returned value. But anyway, it doesn't really matter to me either way.
codebrainz commented on this pull request.
- 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) + { + dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, + _("Geany cannot start!"), error->message); + g_error("Cannot load user-interface: %s", error->message); + g_error_free(error); + g_object_unref(builder); + return; + } + + error = NULL; + if (gtk_builder_add_from_string(builder, g_bytes_get_data(ui_data, NULL), + g_bytes_get_size(ui_data), &error) == 0)
Without any actual research, I assumed it's related to GtkUIManager "merge id" return value, but I have no good evidence that is the case.
elextr commented on this pull request.
- 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) + { + dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, + _("Geany cannot start!"), error->message); + g_error("Cannot load user-interface: %s", error->message); + g_error_free(error); + g_object_unref(builder); + return; + } + + error = NULL; + if (gtk_builder_add_from_string(builder, g_bytes_get_data(ui_data, NULL), + g_bytes_get_size(ui_data), &error) == 0)
Compiler will almost certainly generate the same code either way, but maybe we should check for 666 in case GTK is possessed :)
b4n commented on this pull request.
- 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) + { + dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, + _("Geany cannot start!"), error->message); + g_error("Cannot load user-interface: %s", error->message); + g_error_free(error); + g_object_unref(builder); + return; + } + + error = NULL; + if (gtk_builder_add_from_string(builder, g_bytes_get_data(ui_data, NULL), + g_bytes_get_size(ui_data), &error) == 0)
@codebrainz interesting idea, but no: https://git.gnome.org/browse/gtk+/tree/gtk/gtkbuilder.c#n1299
codebrainz commented on this pull request.
- 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) + { + dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, + _("Geany cannot start!"), error->message); + g_error("Cannot load user-interface: %s", error->message); + g_error_free(error); + g_object_unref(builder); + return; + } + + error = NULL; + if (gtk_builder_add_from_string(builder, g_bytes_get_data(ui_data, NULL), + g_bytes_get_size(ui_data), &error) == 0)
So why isn't the return type `gboolean`? It's the same width and conveys the semantics better.
I think we should go with https://github.com/geany/geany/commit/6e2a51ff5608d188387c856c9d35f25ce7d20d... since it's the same but more paranoid :)
@codebrainz I prefer my implementation for the compat function, but either is fine. Maybe your checks for emptiness are nice though (as `add_from_string()` doesn't accept it for some reason, although `from_resource()` does), although it really shouldn't ever happen.
Otherwise, I think the changes in `ui_utils.c` are slightly better in mine, and closer to master (less unnecessary changes): ```diff diff --git a/src/ui_utils.c b/src/ui_utils.c index a0093174a..5b544bff4 100644 --- a/src/ui_utils.c +++ b/src/ui_utils.c @@ -42,7 +42,6 @@ #include "msgwindow.h" #include "prefs.h" #include "project.h" -#include "resources.h" #include "sciwrappers.h" #include "sidebar.h" #include "stash.h" @@ -2427,7 +2426,6 @@ static GtkWidget *ui_get_top_parent(GtkWidget *widget) void ui_init_builder(void) { const gchar *name; - GBytes *ui_data; GError *error; GSList *iter, *all_objects; GtkWidget *widget, *toplevel; @@ -2445,7 +2443,7 @@ void ui_init_builder(void) { dialogs_show_msgbox_with_secondary(GTK_MESSAGE_ERROR, _("Geany cannot start!"), error->message); - g_error(_("Cannot load user-interface: %s"), error->message); + g_error("Cannot create user-interface: %s", error->message); g_error_free(error); g_object_unref(builder); return; ```
@b4n you should go on IRC :)
I will try to make these changes.
@codebrainz pushed 1 commit.
5b70b0f Some minor fixups
I squished it all together, please double-check.
b4n commented on this pull request.
@@ -42,6 +42,7 @@
#include "msgwindow.h" #include "prefs.h" #include "project.h" +#include "resources.h"
not needed
--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
Hum, maybe you need to add something like `|| rm -f $@` (but you need to still return in error) so that if the generation fails for some reason it will be tried again next time. Another common solution is to generate a temporary file and move it over on success, so it's mostly atomic.
codebrainz commented on this pull request.
--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
Are you sure this tool is too stupid to not properly update the file (or not) depending on the outcome?
b4n commented on this pull request.
--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
I'm not sure, but many tools aren't smart enough so I'd at least check.
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 […] ```
codebrainz commented 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)
That should be done [in the calling code](https://github.com/geany/geany/pull/1703/files#diff-4bbd8d1b91370dfb89a7e350...); this function is just a compat wrapper around `gtk_builder_add_from_resource` and so is meant to match its semantics.
codebrainz commented on this pull request.
--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
@kugel- It's supposed to run it twice, once for the source file (`--generate-source`) and once for the header file (`--generate-header`).
kugel- commented 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)
Right, makes sense.
kugel- approved this pull request.
Looks fine to me. I wouldn't think we need the compat wrapper since 3.4 is really old by now, but anyway.
We still [support back to 2.24](https://github.com/geany/geany/blob/1.32.0/configure.ac#L70).
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?
I should update this for merging early next release. @b4n is it only the superfluous include that needs to be fixed?
github-comments@lists.geany.org