GNOME Bugzilla – Bug 744981
[PATCHes] Use GResource.
Last modified: 2015-03-05 10:30:43 UTC
Created attachment 297591 [details] [review] Use GResource for loading the app-menu. GResource offers a great way to load UI and CSS files with less code. The first patch does it with the app-menu only, using GtkApplication’s facilities; the second changes a little this app-menu, because there’s no reason to put the “New Game” and “Change Size” entries in two different sections; the third loads the main UI file and the CSS from GResource.
Created attachment 297592 [details] [review] 'New Game' and 'Change Size' in the same menu section.
Created attachment 297593 [details] [review] Use GResource for the UI and CSS files.
Review of attachment 297591 [details] [review]: ::: Makefile.am @@ +166,3 @@ po/quot.sed \ po/remove-potcdate.sin \ + src/hitori-resources.c \ This should be in CLEANFILES, not MAINTAINERCLEANFILES. ::: configure.ac @@ +21,3 @@ +# GResources +AC_PATH_PROG(GLIB_COMPILE_RESOURCES, glib-compile-resources) To be properly quoted, this should be: AC_PATH_PROG([GLIB_COMPILE_RESOURCES],[glib-compile-resources]) ::: data/hitori.ui @@ +2,3 @@ <!--*- mode: xml -*--> <interface> + <requires lib="gtk+" version="3.12"/> This change to add the <requires/> element is unrelated. Please split it out into a separate commit. ::: src/hitori.gresource.xml @@ +4,3 @@ + <file preprocess="xml-stripblanks" alias="hitori.ui">./data/hitori.ui</file> + <file alias="hitori.css">./data/hitori.css</file> + </gresource> --> The commented out stuff should be removed from this commit and added in the third one. ::: src/main.c @@ +91,3 @@ { /* Set various properties up */ + g_application_set_application_id (G_APPLICATION (object), "org.gnome.hitori"); What’s the reason for this change?
Review of attachment 297592 [details] [review]: Looks good to me.
Review of attachment 297593 [details] [review]: ::: src/hitori.gresource.xml @@ +4,3 @@ <file preprocess="xml-stripblanks" alias="hitori.ui">./data/hitori.ui</file> <file alias="hitori.css">./data/hitori.css</file> + </gresource> Shouldn’t hitori.ui and hitori.css now also be dist_noinst_DATA in the Makefile.am?
(In reply to Philip Withnall from comment #3) > ::: Makefile.am > @@ +166,3 @@ > po/quot.sed \ > po/remove-potcdate.sin \ > + src/hitori-resources.c \ > > This should be in CLEANFILES, not MAINTAINERCLEANFILES. I advised Arnaud to put this in MAINTAINERCLEANFILES because it gets distributed and requires "special maintainer tools" to rebuild. The general rule I've been following is: * If it's distributed -> MAINTAINERCLEANFILES * Otherwise, if it's generated by configure -> DISTCLEANFILES * Otherwise, CLEANFILES So, please satisfy my intellectual curiosity: do you disagree, and if so why?
(In reply to Michael Catanzaro from comment #6) > (In reply to Philip Withnall from comment #3) > > ::: Makefile.am > > @@ +166,3 @@ > > po/quot.sed \ > > po/remove-potcdate.sin \ > > + src/hitori-resources.c \ > > > > This should be in CLEANFILES, not MAINTAINERCLEANFILES. > > I advised Arnaud to put this in MAINTAINERCLEANFILES because it gets > distributed and requires "special maintainer tools" to rebuild. The general > rule I've been following is: glib-compile-resources comes with libglib-devel — I wouldn’t consider it a special maintainer tool. > * If it's distributed -> MAINTAINERCLEANFILES > * Otherwise, if it's generated by configure -> DISTCLEANFILES > * Otherwise, CLEANFILES > > So, please satisfy my intellectual curiosity: do you disagree, and if so why? I go by these rules: http://www.gnu.org/software/automake/manual/html_node/Clean.html, and would consider hitori-resources.c something that make built.
(In reply to Philip Withnall from comment #7) > I go by these rules: > http://www.gnu.org/software/automake/manual/html_node/Clean.html, and would > consider hitori-resources.c something that make built. Hm, well make could build it, but normally if you're building from a tarball it was already built by the maintainer and won't be rebuilt if it already exists, correct? In the same way that .c files generated by valac automatically get put into MAINTAINERCLEANFILES rather than CLEANFILES. In fact, simply listing it under BUILT_SOURCES seems to be enough to get it added to MAINTAINERCLEANFILES automatically (at least it is definitely cleaned by 'make maintainer-clean' even if not listed), so I think it doesn't need to be listed at all. Doesn't really matter much at all, but it'd still be nice to have clearer guidelines for this....
(In reply to Michael Catanzaro from comment #8) > (In reply to Philip Withnall from comment #7) > > I go by these rules: > > http://www.gnu.org/software/automake/manual/html_node/Clean.html, and would > > consider hitori-resources.c something that make built. > > Hm, well make could build it, but normally if you're building from a tarball > it was already built by the maintainer and won't be rebuilt if it already > exists, correct? In the same way that .c files generated by valac > automatically get put into MAINTAINERCLEANFILES rather than CLEANFILES. Yes. Generally I dislike distributing this kind of generated file because it could depend on properties of the build system instead of the host system. For example, if glib-compile-resources ended up generating something which used a specific GLib version X feature on the build machine, then the code was compiled against a GLib version Y on the host machine which didn’t have that feature. > In fact, simply listing it under BUILT_SOURCES seems to be enough to get it > added to MAINTAINERCLEANFILES automatically (at least it is definitely > cleaned by 'make maintainer-clean' even if not listed), so I think it > doesn't need to be listed at all. Huh, that’s not in the documentation for BUILT_SOURCES, but does seem to be the case. > Doesn't really matter much at all, but it'd still be nice to have clearer > guidelines for this.... It would indeed. See bug #744101.
Created attachment 297779 [details] [review] Use GResource for loading the app-menu. > ::: Makefile.am > @@ +166,3 @@ > po/quot.sed \ > po/remove-potcdate.sin \ > + src/hitori-resources.c \ > > This should be in CLEANFILES, not MAINTAINERCLEANFILES. Ok, let’s go with that for now. > ::: src/main.c > @@ +91,3 @@ > { > /* Set various properties up */ > + g_application_set_application_id (G_APPLICATION (object), "org.gnome.hitori"); > > What’s the reason for this change? The GResource file /org/gnome/hitori/gtk/menus.ui is automatically loaded by the app’ org.gnome.hitori, not by the app’ org.gnome.Hitori. As I love magic, I hope that one time the GSettings at org.gnome.hitori will also be automatically used. So I prefer to align from now all to org.gnome.hitori than to have half at org.gnome.Hitori.
Created attachment 297780 [details] [review] Use GResource for the UI and CSS files. > Shouldn’t hitori.ui and hitori.css now also be dist_noinst_DATA in the Makefile.am? Right, the should.
Created attachment 297781 [details] [review] Use 'requires' tag in UI files. > ::: data/hitori.ui > @@ +2,3 @@ > <!--*- mode: xml -*--> > <interface> > + <requires lib="gtk+" version="3.12"/> > > This change to add the <requires/> element is unrelated. Please > split it out into a separate commit. Ok.
Review of attachment 297779 [details] [review]: ::: Makefile.am @@ +137,3 @@ MAINTAINERS \ hitori.doap \ + src/hitori-resources.c \ No, the whole point from my last review was that the generated hitori-resources.c should *not* be distributed! ::: configure.ac @@ +40,2 @@ # Dependencies +PKG_CHECK_MODULES([GENERAL],[glib-2.0 gio-2.0 >= 2.32 gtk+-3.0 >= 3.15.0 gmodule-2.0 cairo >= 1.4]) You should mention in the commit message that this bumps Hitori’s GTK+ dependency to 3.15. ::: src/main.c @@ +91,3 @@ { /* Set various properties up */ + g_application_set_application_id (G_APPLICATION (object), "org.gnome.hitori"); > The GResource file /org/gnome/hitori/gtk/menus.ui is automatically loaded by > the app’ org.gnome.hitori, not by the app’ org.gnome.Hitori. As I love > magic, I hope that one time the GSettings at org.gnome.hitori will also be > automatically used. So I prefer to align from now all to org.gnome.hitori > than to have half at org.gnome.Hitori. The convention seems to be to use ‘org.gnome.Foo’ rather than ‘org.gnome.foo’ (with a few notable exceptions, such as gedit). I would prefer to keep the app ID as org.gnome.Hitori and change the GResource paths to be /org/gnome/Hitori/… if that’s possible. The GSettings stuff is entirely hypothetical, isn’t it? If so, I’m not sure it would be a problem, since GSettings schemas are in a different namespace to app IDs and app resources. Please correct me if I’m wrong, since you’ve looked at this more recently than me.
Review of attachment 297780 [details] [review]: ++
Review of attachment 297781 [details] [review]: ++
(In reply to Philip Withnall from comment #13) > Review of attachment 297779 [details] [review] [review]: > > ::: Makefile.am > @@ +137,3 @@ > MAINTAINERS \ > hitori.doap \ > + src/hitori-resources.c \ > > No, the whole point from my last review was that the generated > hitori-resources.c should *not* be distributed! Well it should not be listed in EXTRA_DIST, for sure, but surely it will be distributed by virtue of being listed in BUILT_SOURCES listed in src_hitori_SOURCES! I'm not sure if it's possible to avoid distributing it?
Created attachment 298290 [details] [review] Gtk+ >= 3.15, GResource for loading the app-menu. (In reply to Philip Withnall from comment #13) > Review of attachment 297779 [details] [review] [review]: > > ::: Makefile.am > @@ +137,3 @@ > MAINTAINERS \ > hitori.doap \ > + src/hitori-resources.c \ > > No, the whole point from my last review was that the generated > hitori-resources.c should *not* be distributed! Sorry, didn’t put this line where I wanted, not enough coffee. /o\ But reading the whole discussion, I finally don’t know if you want it to be somewhere or not. As the file is distributed in every case I know about, I suppressed the line. > The GSettings stuff is entirely hypothetical, isn’t it? If so, I’m not sure > it would be a problem, since GSettings schemas are in a different namespace > to app IDs and app resources. Please correct me if I’m wrong, since you’ve > looked at this more recently than me. The GSettings stuff is hypothetical. But there will be some big things attached to the application’s ID in the next cycle –DBus support–, so it won’t be easy after that to change it.
Created attachment 298294 [details] [review] 'New Game' and 'Change Size' in the same menu section.
Created attachment 298295 [details] [review] Use GResource for the UI and CSS files.
Created attachment 298296 [details] [review] Use 'requires' tag in UI files.
Pushed with some changes to the Makefile to: • Fix builddir ≠ srcdir issues by using $(top_srcdir) appropriately • List hitori-resources.c in nodist_src_hitori_SOURCES instead of BUILT_SOURCES* • List hitori-resources.c in CLEANFILES *I don’t see the need to use BUILT_SOURCES — nodist_src_hitori_SOURCES makes the dependency management work fine for me. Thanks a lot for your work on this! The following fixes have been pushed: 6860892 Use GResource for the UI and CSS files. b8a8695 Use 'requires' tag in UI files. 030f6ef 'New Game' and 'Change Size' in the same menu section. 72315c8 Gtk+ >= 3.15, GResource for loading the app-menu.
Created attachment 298613 [details] [review] Use GResource for the UI and CSS files.
Created attachment 298614 [details] [review] Use 'requires' tag in UI files.
Created attachment 298615 [details] [review] 'New Game' and 'Change Size' in the same menu section.
Created attachment 298616 [details] [review] Gtk+ >= 3.15, GResource for loading the app-menu.