GNOME Bugzilla – Bug 779975
Maps doesn't run with latest flatpak
Last modified: 2017-03-16 20:35:58 UTC
I'm trying to make sure all newcomers apps run with flatpak, and found out gnome-maps doesn't, with this output: Gjs-Message: JS WARNING: [resource:///org/gnome/Maps/js/xmldom/dom.js 21]: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create (gnome-maps:2): Gjs-WARNING **: JS ERROR: Gio.DBusError: Error calling StartServiceByName for org.gnome.OnlineAccounts: GDBus.Error:org.freedesktop.DBus.Error.Spawn.ExecFailed: Failed to execute program org.gnome.OnlineAccounts: No such file or directory CheckInManager<._init@resource:///org/gnome/Maps/js/checkIn.js:47:27 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22 Application<._initServices@resource:///org/gnome/Maps/js/application.js:256:26 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22 Application<.vfunc_startup@resource:///org/gnome/Maps/js/application.js:228:9 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22 main@resource:///org/gnome/Maps/js/main.js:57:12 run@resource:///org/gnome/gjs/modules/package.js:192:12 start@resource:///org/gnome/gjs/modules/package.js:176:5 @/app/bin/gnome-maps:2:1 Gjs-Message: JS WARNING: [resource:///org/gnome/Maps/js/mapView.js 299]: reference to undefined property this.view.map_source.id Gjs-Message: JS WARNING: [resource:///org/gnome/Maps/js/routeEntry.js 61]: reference to undefined property icon.window (gnome-maps:2): Gjs-WARNING **: JS ERROR: TypeError: Application.placeStore is null FavoritesPopover<._init@resource:///org/gnome/Maps/js/favoritesPopover.js:61:9 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22 MainWindow<._init@resource:///org/gnome/Maps/js/mainWindow.js:92:41 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22 Application<._createWindow@resource:///org/gnome/Maps/js/application.js:268:28 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22 Application<.vfunc_activate@resource:///org/gnome/Maps/js/application.js:288:9 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:22 main@resource:///org/gnome/Maps/js/main.js:57:12 run@resource:///org/gnome/gjs/modules/package.js:192:12 start@resource:///org/gnome/gjs/modules/package.js:176:5 @/app/bin/gnome-maps:2:1
Created attachment 347953 [details] [review] checkIn: Gracefully handle failure to create GOA client Don't crash if we're not able to create the GOA client. Instead gracefully disable check-in support.
Above patch made it work for me!
Review of attachment 347953 [details] [review]: Heya, this works! I took the freedom to review with my no knowledge of code in maps. Take it with a grain of salt, and feel free to commit with no changes or to ask someone else to review :) ::: src/checkIn.js @@ +45,3 @@ this.parent(); + try { The only thing that you really "try" is Goa.Client.new_sync (and then the this._goaClient.connect) @@ +47,3 @@ + try { + this._goaClient = Goa.Client.new_sync(null); + this._accounts = []; You can put the initializations outsise @@ +53,1 @@ + this._initBackends(); I didn't read the code of this, but in case in a future this could throw some error you really don't want to have it in the same try/catch handler. @@ +59,1 @@ + this._refreshGoaAccounts(); this can be outside of the try too. Instead to protect for emitting 'accounts-refreshed' and 'hasCheckIn' you could have an early return like if (accounts.length < 1) or so, so the functions "refreshAccounts" is more robust and can be used outside of the initialization. If this is the case, the initialization of this._accounts and authorizers can be omited in the _init. @@ +104,2 @@ get hasCheckIn() { + return this._accounts && this._accounts.length > 0; you can put just this._accounts.lenght > 0
Created attachment 348035 [details] [review] checkIn: Gracefully handle failure to create GOA client Don't crash if we're not able to create the GOA client. Instead gracefully disable check-in support.
(In reply to Carlos Soriano from comment #3) > Review of attachment 347953 [details] [review] [review]: > > Heya, this works! Great! > > I took the freedom to review with my no knowledge of code in maps. Take it > with a grain of salt, and feel free to commit with no changes or to ask > someone else to review :) > > ::: src/checkIn.js > @@ +45,3 @@ > this.parent(); > > + try { > > The only thing that you really "try" is Goa.Client.new_sync (and then the > this._goaClient.connect) > Yeah, makes sense. > @@ +47,3 @@ > + try { > + this._goaClient = Goa.Client.new_sync(null); > + this._accounts = []; > > You can put the initializations outsise > Rewrote so that the initialization (to the empty state) for accounts, authorizers, and backends are done after the try-catch and the signal attachments are guarded in an if. Also, made _refreshGoaAccounts() return early if this._goaClient is undefined. This was it should be more independent on running guarded from _init(). > @@ +53,1 @@ > + this._initBackends(); > > I didn't read the code of this, but in case in a future this could throw > some error you really don't want to have it in the same try/catch handler. > > @@ +59,1 @@ > + this._refreshGoaAccounts(); > > this can be outside of the try too. > Instead to protect for emitting 'accounts-refreshed' and 'hasCheckIn' you > could have an early return like if (accounts.length < 1) or so, so the > functions "refreshAccounts" is more robust and can be used outside of the > initialization. > > If this is the case, the initialization of this._accounts and authorizers > can be omited in the _init. > > @@ +104,2 @@ > get hasCheckIn() { > + return this._accounts && this._accounts.length > 0; > > you can put just this._accounts.lenght > 0 Yep! Thanks for the review! :-)
Review of attachment 348035 [details] [review]: LGTM!
Review of attachment 348035 [details] [review]: Send it off, godspeed.
Attachment 348035 [details] pushed as 9df6884 - checkIn: Gracefully handle failure to create GOA client