GNOME Bugzilla – Bug 733411
Follow the Gjs application conventions
Last modified: 2015-01-05 09:27:55 UTC
Recently [1], new conventions for Gjs apps were proposed [2]. It would be great to follow this conventions in Maps. I propose to start this migration for 3.16 since some cooking patch-set coming from this GSoC doesn't follow the convetions, so starting the migration now can needlessly delay the upcoming features. [1] https://mail.gnome.org/archives/desktop-devel-list/2014-July/msg00018.html [2] https://wiki.gnome.org/Projects/Gjs/Package
Sounds like a good idea and a good plan, Damián. Thanks for creating this bug. If some one feels like hacking a patch for this it would be nice. I will mark it as gnome-love. And be willing to act as a mentor and answer question if someone want to try this out to get their feet wet in gnome development.
If you want, I prepared a patch, just to see how difficult it is to take an existing app and turn into a package.js app. It's not huge, but there are subtleties here and there. I can attach it, or not, if you want to use this bug to mentor newcomers.
A patch against Maps? You could attach it, the task then will be to massage that patch to work against the post 3.14 tree.
Created attachment 281284 [details] [review] Convert to the new Gjs "package.js" application conventions Unify resource names and GSettings schemas as org.gnome.Maps. Move all data files to data/, leaving only importable JS sources in src/ Remove the C shim which is not needed. Copy the Makefiles from gtk-js-app, and replace the names appropriately Remove the config.js and path.js modules, as their functionality is provided by package.js Remove C compiler dependency. Add helper to obtain the settings that works if the app is not yet installed.
Created attachment 287550 [details] [review] Convert to Gjs "package.js" application conventions Unify resource names and GSettings schemas as org.gnome.Maps. Move all data files to data/, leaving only importable JS sources in src/ Remove the C shim which is not needed. Copy the Makefiles from gtk-js-app, and replace the names appropriately Remove the config.js and path.js modules, as their functionality is provided by package.js Remove C compiler dependency. Add helper to obtain the settings that works if the app is not yet installed. ------ - Rebased on master. - Fixed translation of gschemas. - Removed Makefile-js.am. - Fixed Makefile.am tab alignment. Just to be sure, I am not seeing the correct app name and icon in shell's topbar after these changes. This is because I am running gnome-shell in a non-developer environment (different than Maps one) and org.gnome.Maps.desktop in shell's environment, am I right?
s/and org.gnome.Maps.desktop in shell's environment/and org.gnome.Maps.desktop is not present in shell's environment
Review of attachment 287550 [details] [review]: Thx for doing this. I am bit confused by whether the const _ = imports.gettext.gettext is needed or not. Since it is removed in application.js but nowhere(?) else. ::: src/application.js @@ -26,3 @@ const Signals = imports.signals; -const Gettext = imports.gettext; -const _ = imports.gettext.gettext; Oh so this is not needed anymore? :o @@ +116,3 @@ return; + Gtk.IconTheme.get_default().append_search_path(pkg.pkgdatadir + '/icons'); Should we use GLib.build_filanamev? ::: src/config.js.in @@ -2,3 @@ -const PACKAGE_VERSION = '@PACKAGE_VERSION@'; -const GETTEXT_PACKAGE = '@GETTEXT_PACKAGE@'; -const USER_AGENT = [PACKAGE_NAME, PACKAGE_VERSION].join(' '); (o/ ::: src/geoclue.js @@ -34,2 @@ const Signals = imports.signals; const _ = imports.gettext.gettext; Why was the _ import removed from application.js but not here? ::: src/mainWindow.js @@ -42,3 @@ const ZoomControl = imports.zoomControl; const _ = imports.gettext.gettext; Same here... ::: src/path.js.in @@ -2,3 @@ -let STYLE_DIR = "@pkgdatadir@/style/"; -let ICONS_DIR = "@pkgdatadir@/icons/"; -let RESOURCE_DIR = "@pkgdatadir@"; (o/ ::: src/routeService.js @@ +35,3 @@ const Utils = imports.utils; +const USER_AGENT = 'gnome-maps/' + pkg.version; GLib.build_filenamev ::: src/userLocationMarker.js @@ -33,3 @@ const UserLocationBubble = imports.userLocationBubble; const Utils = imports.utils; const _ = imports.gettext.gettext; And here.
Review of attachment 287550 [details] [review]: ::: src/application.js @@ -26,3 @@ const Signals = imports.signals; -const Gettext = imports.gettext; -const _ = imports.gettext.gettext; Ah... it looks like package.js declares _ as a global in initGettext function. Yes, then let's remove everywhere since is a package.js related stuff. @@ +116,3 @@ return; + Gtk.IconTheme.get_default().append_search_path(pkg.pkgdatadir + '/icons'); +1 ::: src/config.js.in @@ -2,3 @@ -const PACKAGE_VERSION = '@PACKAGE_VERSION@'; -const GETTEXT_PACKAGE = '@GETTEXT_PACKAGE@'; -const USER_AGENT = [PACKAGE_NAME, PACKAGE_VERSION].join(' '); \o) ::: src/path.js.in @@ -2,3 @@ -let STYLE_DIR = "@pkgdatadir@/style/"; -let ICONS_DIR = "@pkgdatadir@/icons/"; -let RESOURCE_DIR = "@pkgdatadir@"; \o/ ::: src/routeService.js @@ +35,3 @@ const Utils = imports.utils; +const USER_AGENT = 'gnome-maps/' + pkg.version; +1
Review of attachment 287550 [details] [review]: ::: src/routeService.js @@ +35,3 @@ const Utils = imports.utils; +const USER_AGENT = 'gnome-maps/' + pkg.version; I mean -1, it's a HTTP user agent, not a filename :)
Created attachment 287614 [details] [review] Convert to Gjs "package.js" application conventions Unify resource names and GSettings schemas as org.gnome.Maps. Move all data files to data/, leaving only importable JS sources in src/ Remove the C shim which is not needed. Copy the Makefiles from gtk-js-app, and replace the names appropriately Remove the config.js and path.js modules, as their functionality is provided by package.js Remove C compiler dependency. Add helper to obtain the settings that works if the app is not yet installed. ---- - Remove all explicit imports of gettext's _ function. - Use pkg.initFormat instead of manually initialize String.prototype.format. - Use GLib.build_filenamev when appropriated
Review of attachment 287614 [details] [review]: I had my gjs stuck on an old branch. So I got an error while running this. Maybe we should bump the gjs min version in configure.ac to 1.42 while doing this? Other than that it seems swell. And distcheck passed. But I have one concern. We made maps a binary for a reason, in https://bugzilla.gnome.org/show_bug.cgi?id=722338, Zeeshan wrote: "Currently our executable '${bindir}/gnome-maps' is a shall script that launches gjs-console with scripts etc passed to it. Trouble is that geoclue2 will need to identify apps with /proc/${PID}/exe (as opposed to /proc/${PID}/cmdline, which can be easily overwritten by apps themselves) and that points to the path of gjs-console binary rather. So geoclue2 will have no means of differentiating btwe gnome-weather and gnome-maps for example." Zeeshan, is this still valid?
Review of attachment 287614 [details] [review]: Zeeshan says that is is not valid anymore, so seems like we are fine,
(In reply to comment #11) > Review of attachment 287614 [details] [review]: > > I had my gjs stuck on an old branch. So I got an error while running this. > Maybe we should bump the gjs min version in configure.ac to 1.42 while doing > this? Yes, I think that's fine.
Created attachment 293002 [details] [review] Convert to Gjs "package.js" application conventions - Rebased on master - Move .ui files to data/ui (I hope you like it, it looks more comfortable to me but I think is not the standard convention) - Use imports.package.start instead of imports.package.run
Review of attachment 293002 [details] [review]: Oh boy, these are a lot of changes :) Thanks for doing that work Damián. So, ok. Could you tell me again exactly what we gain from this? I got a general idea but it would b nice. Maybe you could update the commit message to be more why than what? Also: Maybe we could do the moving of the ui files in a separate commit? It is not strictly related to this and it would make it easier to get an overview and review this monster :) I am fine with that move, makes our structure a bit more like other GNOME apps. ::: src/routeService.js @@ +33,3 @@ const Utils = imports.utils; +const USER_AGENT = 'gnome-maps/' + pkg.version; Will this be the same USER_AGENT as the one we hade in Config? And there is no way to make this pkg.user_agent?
Created attachment 293203 [details] [review] Convert to Gjs "package.js" application conventions Unify resource names and GSettings schemas as org.gnome.Maps. Remove the C shim which is not needed. Copy the Makefiles from gtk-js-app, and replace the names appropriately Remove the config.js and path.js modules, as their functionality is provided by package.js Remove C compiler dependency.
Created attachment 293204 [details] [review] Unify resource namespace and move ui files to data/ui
Created attachment 293205 [details] [review] settings: Load settings if the app is not installed
Created attachment 293206 [details] [review] Use pkg.initGettext This initializes gettext for us and declares "_", "C_" and "N_" functions implicitly.
Created attachment 293207 [details] [review] Use pkg.initFormat
Review of attachment 293203 [details] [review]: I love you for diving into this! Regarding review: mostly simple code style issues. ::: data/Makefile.am @@ +1,3 @@ SUBDIRS = icons +app_resource_files = $(shell $(GLIB_COMPILE_RESOURCES) --sourcedir=$(srcdir) --generate-dependencies $(srcdir)/org.gnome.Maps.data.gresource.xml) Split this up in multiple lines. One line per flag. @@ +52,3 @@ + $(gsettings_SCHEMAS) + $(NULL) + Align these '\' at column 72 (like in data/icons/Makefile.am and src/Makefile.am) ::: src/Makefile.am @@ +4,3 @@ +nodist_app_SCRIPTS = org.gnome.Maps + +app_resource_files = $(shell $(GLIB_COMPILE_RESOURCES) --sourcedir=$(srcdir) --generate-dependencies $(srcdir)/org.gnome.Maps.src.gresource.xml) Split this up in multiple lines. One flag per line. @@ +25,3 @@ + org.gnome.Maps.src.gresource.xml \ + $(app_resource_files) \ + $(service_resource_files) \ Align at column 72 ::: src/main.js @@ +25,3 @@ +function main(args) { + pkg.useragent = 'gnome-maps/' + pkg.version; Hm, in general I don't feel comfortable adding properties to objects owned by other code. I think we can just skip this line, and later in routeService.js... ::: src/routeService.js @@ +47,2 @@ _init: function() { + this._session = new Soup.Session({ user_agent : pkg.useragent }); ...just use 'gnome-maps/' + pkg.version here.
Review of attachment 293203 [details] [review]: ::: src/main.js @@ +25,3 @@ +function main(args) { + pkg.useragent = 'gnome-maps/' + pkg.version; Just to chirp in a bit, I was the one who asked if there was any possibility of getting pkg.useragent. I am fine with going the route Mattias spells out here if there is no "native" pkg.useragent.
Review of attachment 293204 [details] [review]: Other than comment it looks good. ::: data/org.gnome.Maps.data.gresource.xml @@ +24,1 @@ <file alias="application.css">gnome-maps.css</file> Shouldn't the css file be part of "ui" ?
Review of attachment 293206 [details] [review]: Looks fine!
Review of attachment 293207 [details] [review]: Ok, so this gives us String.format? Maybe the commit message body can say so? Other than that: thanks!
Review of attachment 293204 [details] [review]: I think you might have forgotten to move the ui-files here no? Otherwise looks great! ::: data/org.gnome.Maps.data.gresource.xml @@ +2,3 @@ <gresources> + <gresource prefix="/org/gnome/Maps"> + <file preprocess="xml-stripblanks">ui/app-menu.ui</file> If you change these paths here, don't you need to also move the ui-files as well? The patches doesn't apply on master for me so I can't test.
Review of attachment 293203 [details] [review]: Thanks! ::: data/org.gnome.Maps.data.gresource.xml @@ +26,3 @@ + <file alias="maptype-street.png">media/maptype-street.png</file> + </gresource> +</gresources> No newline at end of file Maybe we can alphabetize this list when we are recreating it? ::: data/org.gnome.maps.gschema.xml.in @@ +1,2 @@ <schemalist gettext-domain="gnome-maps"> + <schema id="org.gnome.Maps" path="/org/gnome/maps/"> Does this fix anything?
Review of attachment 293205 [details] [review]: So I need this to be more explained. Both for me, here and now and in the commit message. Why is this needed? Why was it not needed before? Or was it? What does this fix? ::: src/settings.js @@ +69,3 @@ + } + + if (path === undefined) What is path? We do not supply any path atm, when do we need to?
Review of attachment 293205 [details] [review]: ACK
Review of attachment 293203 [details] [review]: ::: data/org.gnome.Maps.data.gresource.xml @@ +26,3 @@ + <file alias="maptype-street.png">media/maptype-street.png</file> + </gresource> +</gresources> No newline at end of file No newline at end of file Oh yes please! :) ::: data/org.gnome.maps.gschema.xml.in @@ +1,2 @@ <schemalist gettext-domain="gnome-maps"> + <schema id="org.gnome.Maps" path="/org/gnome/maps/"> From the gsettings docs: » The convention for schema ids is to use a dotted name, similar in style to a D-Bus bus name, e.g. "org.gnome.SessionManager". In particular, if the settings are for a specific service that owns a D-Bus bus name, the D-Bus bus name and schema id should match. « Not sure if it gives us anything but lets be consistent.
Review of attachment 293204 [details] [review]: ::: data/org.gnome.Maps.data.gresource.xml @@ +2,3 @@ <gresources> + <gresource prefix="/org/gnome/Maps"> + <file preprocess="xml-stripblanks">ui/app-menu.ui</file> WTF, I can see the renames in my log, let me see what happened. @@ +24,1 @@ <file alias="application.css">gnome-maps.css</file> Hmmm... why? you mean moving it to ui directory?
(In reply to comment #31) > Review of attachment 293204 [details] [review]: > > ::: data/org.gnome.Maps.data.gresource.xml > @@ +2,3 @@ > <gresources> > + <gresource prefix="/org/gnome/Maps"> > + <file preprocess="xml-stripblanks">ui/app-menu.ui</file> > > WTF, I can see the renames in my log, let me see what happened. > > @@ +24,1 @@ > <file alias="application.css">gnome-maps.css</file> > > Hmmm... why? you mean moving it to ui directory? Yes, Ive seen other apps have the css filé in ui/
Review of attachment 293204 [details] [review]: ::: data/org.gnome.Maps.data.gresource.xml @@ +2,3 @@ <gresources> + <gresource prefix="/org/gnome/Maps"> + <file preprocess="xml-stripblanks">ui/app-menu.ui</file> :) @@ +24,1 @@ <file alias="application.css">gnome-maps.css</file> Nah I agree with you here Damián. Let ui/ be just ui-files.
Review of attachment 293205 [details] [review]: Let me be honest, I fully trusted in Giovanni for that part of the code :P (it comes from gnome-weather and gtk-js-app I think) Maybe this is really not necessary and we could skip it since executing the app without installing it is not part of our workflow (maybe because jhbuild always do make install). Originally, I think Giovanni made it in that way because package.js always take into account that the app can be executed directly from source (I mean, be able to execute the app from source seems to be part of the convention) and this is needed to load the GSettings from source. We could skip it, personally, I would never use this.
Review of attachment 293204 [details] [review]: ::: data/org.gnome.Maps.data.gresource.xml @@ +2,3 @@ <gresources> + <gresource prefix="/org/gnome/Maps"> + <file preprocess="xml-stripblanks">ui/app-menu.ui</file> So, the patch is submitted right (see the raw version of it), it seems that splinter doesn't manage renames well. Also, I checked out master and reapplied the patchset from bz, and it applies right. Maybe you need to pull master?
Review of attachment 293205 [details] [review]: Being able to run without an install step is actually something I'd really like. It would potentially speed up development a little bit more.
Review of attachment 293204 [details] [review]: ::: data/org.gnome.Maps.data.gresource.xml @@ +2,3 @@ <gresources> + <gresource prefix="/org/gnome/Maps"> + <file preprocess="xml-stripblanks">ui/app-menu.ui</file> Ah great! I'll try to apply the patches once more! :)
Created attachment 293263 [details] [review] Convert to Gjs "package.js" application conventions Unify resource names and GSettings schemas as org.gnome.Maps. Remove the C shim which is not needed. Copy the Makefiles from gtk-js-app, and replace the names appropriately Remove the config.js and path.js modules, as their functionality is provided by package.js Remove C compiler dependency.
Created attachment 293264 [details] [review] Unify resource namespace and move ui files to data/ui
Created attachment 293265 [details] [review] settings: Load settings if the app is not installed
Created attachment 293266 [details] [review] Use pkg.initGettext This initializes gettext for us and declares "_", "C_" and "N_" functions implicitly.
Created attachment 293267 [details] [review] Use pkg.initFormat pkg.initFormat is a gjs's helper that declares the String.format method, let's use this helper instead of declare the method manually.
Created attachment 293745 [details] [review] Convert to Gjs "package.js" application conventions Unify resource names and GSettings schemas as org.gnome.Maps. Remove the C shim which is not needed. Copy the Makefiles from gtk-js-app, and replace the names appropriately Remove the config.js and path.js modules, as their functionality is provided by package.js Remove C compiler dependency.
Created attachment 293746 [details] [review] Unify resource namespace and move ui files to data/ui
Created attachment 293747 [details] [review] settings: Load settings if the app is not installed
Created attachment 293748 [details] [review] Use pkg.initGettext This initializes gettext for us and declares "_", "C_" and "N_" functions implicitly.
Created attachment 293749 [details] [review] Use pkg.initFormat pkg.initFormat is a gjs's helper that declares the String.format method, let's use this helper instead of declare the method manually.
Review of attachment 293745 [details] [review]: Thanks!
Review of attachment 293746 [details] [review]: Great!
Review of attachment 293747 [details] [review]: Still unsure of this, but if you guys want it, sure. ::: src/settings.js @@ +51,3 @@ }); + +function getSettings(schemaId, path) { path is never used? Why include it? @@ +67,3 @@ + if (!schemaObj) { + log('Missing GSettings schema ' + schemaId); + System.exit(1); Can't we return NULL instead? And let the application deal with what to do?
Review of attachment 293748 [details] [review]: Thanks!
Review of attachment 293749 [details] [review]: Thanks!