GNOME Bugzilla – Bug 750861
port to package.js
Last modified: 2015-08-10 19:05:16 UTC
The port to package.js is necessary to be able to use Templates.
Created attachment 305176 [details] [review] port to package.js
Created attachment 305180 [details] [review] port to package.js
Created attachment 305184 [details] [review] port to package.js
Created attachment 308591 [details] [review] port to package.js
Created attachment 308656 [details] [review] port to package.js
Review of attachment 308656 [details] [review]: Thanks for doing this, Alessandro. One quick comment: ::: src/mainBooks.js @@ +26,3 @@ + 'Goa': '1.0', + 'GObject': '2.0', + 'Gtk': '3.0' }); Aren't we missing a few things here? Tracker, TrackerControl, EvinceDocument, WebKit2 come to mind. You can take a look at the top of src/application.js, and that bit can now be removed.
Created attachment 308973 [details] [review] port to package.js
Review of attachment 308973 [details] [review]: Thanks, Alessandro. A few more things that I noticed: ::: src/Makefile.am @@ +95,3 @@ + $(MKDIR_P) $(DESTDIR)$(bindir) + -rm -f $(DESTDIR)$(bindir)/gnome-documents + -rm -f $(DESTDIR)$(bindir)/gnome-books We should also delete the gresource symlinks. ::: src/application.js @@ -493,3 @@ - let resource = Gio.Resource.load(Path.RESOURCE_DIR + '/gnome-documents.gresource'); - resource._register(); We should also remove the set_prgname calls because package.js does it for us. ::: src/main.js @@ +29,3 @@ + 'Goa': '1.0', + 'Gtk': '3.0', + 'Goa': '1.0', 'Goa' has been mentioned twice. ::: src/mainBooks.js @@ +29,3 @@ + 'Goa': '1.0', + 'Gtk': '3.0', + 'Goa': '1.0', Ditto.
Created attachment 308977 [details] [review] port to package.js I took the liberty to fix these up.
Created attachment 308986 [details] [review] porting to package.js
Review of attachment 308986 [details] [review]: Thanks, Alessandro. This looks really good. A few comments: ::: data/Makefile.am @@ +7,3 @@ + --sourcedir=$(srcdir) \ + --generate-dependencies \ + $(srcdir)/org.gnome.Documents.data.gresource.xml \ We should add app_resource_files and the XML to EXTRA_DIST. Otherwise they will be missing from the release tarballs. ::: po/POTFILES.in @@ +26,3 @@ +[type: gettext/glade]data/ui/preview-context-menu.ui +[type: gettext/glade]data/ui/preview-menu.ui +[type: gettext/glade]data/ui/selection-menu.ui Better to retain the alphabetical order. ::: src/Makefile-js.am @@ -34,3 @@ BUILT_SOURCES += \ path.js \ config.js We can completely remove path.js.in and config.js.in, and along with them this entire file, because package.js already exports all the information provided by these files. As a nice side effect of that we won't have to fix the distcheck failures that I am seeing now due to build-generated JavaScript sources. ::: src/Makefile.am @@ +50,3 @@ + --generate-dependencies \ + $(srcdir)/org.gnome.Documents.src.gresource.xml \ + ) We should put documents_app_resource_files and the XML file in EXTRA_DIST. @@ +62,3 @@ + --generate-dependencies \ + $(srcdir)/org.gnome.Books.src.gresource.xml \ + ) Ditto. @@ +74,1 @@ +CLEANFILES += org.gnome.Documents.src.gresource org.gnome.Books.src.gresource We could just use the resource_DATA, as we do elsewhere, instead of repeating the names.
Review of attachment 308986 [details] [review]: Thanks, Alessandro. This looks really good. A few comments: ::: data/Makefile.am @@ +7,3 @@ + --sourcedir=$(srcdir) \ + --generate-dependencies \ + $(srcdir)/org.gnome.Documents.data.gresource.xml \ We should add app_resource_files and the XML to EXTRA_DIST. Otherwise they will be missing from the release tarballs. @@ +60,3 @@ +install-exec-hook: + -rm -f $(appdir)/org.gnome.Books.data.gresource + $(LN_S) $(appdir)/org.gnome.Documents.data.gresource $(appdir)/org.gnome.Books.data.gresource The destination (ie. the second argument) is missing DESTDIR. ::: po/POTFILES.in @@ +26,3 @@ +[type: gettext/glade]data/ui/preview-context-menu.ui +[type: gettext/glade]data/ui/preview-menu.ui +[type: gettext/glade]data/ui/selection-menu.ui Better to retain the alphabetical order. ::: src/Makefile-js.am @@ -34,3 @@ BUILT_SOURCES += \ path.js \ config.js We can completely remove path.js.in and config.js.in, and along with them this entire file, because package.js already exports all the information provided by these files. As a nice side effect of that we won't have to fix the distcheck failures that I am seeing now due to build-generated JavaScript sources. ::: src/Makefile.am @@ +50,3 @@ + --generate-dependencies \ + $(srcdir)/org.gnome.Documents.src.gresource.xml \ + ) We should put documents_app_resource_files and the XML file in EXTRA_DIST. @@ +62,3 @@ + --generate-dependencies \ + $(srcdir)/org.gnome.Books.src.gresource.xml \ + ) Ditto. @@ +74,1 @@ +CLEANFILES += org.gnome.Documents.src.gresource org.gnome.Books.src.gresource We could just use the resource_DATA, as we do elsewhere, instead of repeating the names.
Created attachment 309024 [details] [review] Port to package.js
Comment on attachment 309024 [details] [review] Port to package.js I fixed the above issues and pushed to master.