GNOME Bugzilla – Bug 766196
Change app id to "org.gnome.Geary"
Last modified: 2016-12-09 09:58:01 UTC
Currently, Geary uses org.yorba.geary as its application id. This is fine, but due to Bug 766133 we need to rename the desktop and appid files to include it, so if we are going to consider renaming the app id to fit with convention a bit better (e.g. to "org.gnome.Geary"), then now might be a good time to do it. If we're going to do this, the following also will want to happen: - Advertise the change on the mailing list in case someone is using it via DBus - Update the GSettings schema name, ensure existing user prefs get migrated - Update the Libsecret username prefix, ensure existing passwords get mirgated - Check it isn't getting used elsewhere (End-user reviews in Software? Pinning the app in shell/unity/other launchers?) - Add a note in the Release Notes for the version that first includes it. On the balance, and as a hat tip to Yorba, I'm inclined to leave it as-is for the moment, but welcome discussion about it. Depending on what crops up as a result of the second to last point above, it might be a good idea to do it sooner rather than later.
To be honest, I think we shouldn't be putting ourselves and our users through the ordeal of changing the app id twice. This would be giving everyone a double headache for only a minor -almost invisible to most end users- application property. I do agree that we shouldn't erase all memories to Yorba, as we should be grateful for the project. However, I'm more inclined to leave a special note in the 'About'-box than to leave it in the app id.
Created attachment 328427 [details] [review] Preliminary work - migrating to org.gnome.geary The attached patch should contain most of the work that needs to be done. Some notes and questions I wrote down while working on it: * End-user reviews: I don't think this should be much of a problem in itself. In gnome-sofware, we do not have . - Update the GSettings schema name, ensure existing user prefs get migrated - Update the Libsecret username prefix, ensure existing passwords get mirgated - Check it isn't getting used elsewhere (End-user reviews in Software? Pinning the app in shell/unity/other launchers?) - Add a note in the Release Notes for the version that first includes it.
Whoops, accidentally pressed submit while typing. I'll just put my comment here The attached patch should contain most of the work that needs to be done. Some notes and questions I wrote down while working on it: * End-user reviews: I don't think this should be much of a problem in itself. In gnome-software, we do not have reviews and in Ubuntu software center (which Ubuntu itself will be migrating away from) the last version is 0.4.1, which isn't representative anyway imho. * Still need to look into migrating existing GSettings, although it started up fine with my GMail account already configured, so I'll have to take a look into that. * Concerning Libsecret: a lot of migration code is already in place. Should I keep all of it, or is it possible to start assuming people coming from <= 0.6 should just re-enter their password? * Changing the name of the desktop file already lead to gnome-shell thinking it's another application, leading to another icon and the pinned version being "the old geary". * While looking at our appdata xml-file, I saw we were still using a very old screenshot (hosted on yorba.org). It might be a good time to update it, since it's the preview people see in gnome-software? That should be for another bug though.
Hey, this is a good start. For consistency with other GNOME apps, the app-id should have a capitalised "G" in Geary. While many apps seem to use an all lower-case name in their GSettings schema, a few newer ones are also starting to use title-case, and the docs give examples using that, so the schema should probably also use a "G". I guess Ubuntu must be patching gnome-software then, since that does have user reviews (requires an Ubuntu SSO login) on a Xenial system. Thanks for the heads-up about the screenshot, I'll get it updated.
Oh, about the existing libsecret migrations - we should keep the existing code for the moment. It's pretty well compartmentalised, so it should be just a matter of adding another call to migrate_old_password, or whatever new method is required if that won't already do the job. Also, I wonder if gnome-shell has a DBus API for migrating its app-id data?
So it seems that Flatpack is going to need these files renamed anyway (see bug 766479), so we may as well do this for the next release. Nils, are you happy to update the "G" in the app name and libsecrent migrations in the patch per comments 4 & 5? I think the GSettings schema file has changed, so will need a refresh to cleanly apply to current master anyway.
Created attachment 330412 [details] [review] [Patch] Change app id to org.gnome.Geary (with big G) Attached patch should fix that, minus the libsecret migration. About that: maybe we should immediately start using our own schema for libsecret, which is the recommended way of doing this (see [1])? That way, the pain of the migrations could be contained in one version. Also, it would make future migrations easier (just use a different schema for newer incompatible versions). [1] - https://people.gnome.org/~stefw/libsecret-docs/libsecret-SecretSchema.html#SECRET-SCHEMA-COMPAT-NETWORK:CAPS
Review of attachment 330412 [details] [review]: Okay, aside from the white space issue this looks good. We still need to migrate settings values, and also it seems like autostart manager is probably going to need some migration code added as well - we need to make sure that users with it enabled still have Geary automatically start after upgrading, and once started it needs to check for an old autostart file and if present, rename/replace it with the current one. With these things taken care of, I'll be happy to commit this. I agree if we are going to migrate libsecret data, we may as well get away from using the compat schema as well - I'll create a separate bug for that. ::: cmake/FindIntltool.cmake @@ +18,3 @@ ${CMAKE_CURRENT_SOURCE_DIR}/${desktop_id}.desktop.in ${desktop_id}.desktop ) + install (FILES ${CMAKE_CURRENT_BINARY_DIR}/org.gnome.Geary.desktop DESTINATION ${CMAKE_INSTALL_PREFIX}/share/applications) `git am` is complaining about a whitespace error here.
Updating libsecret schema filed as Bug 768121.
Some other thing that should be done as a result of this: - Set DBusActivatable=true in the desktop file, to enable app management via D-Bus: https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#dbus - Remove resource_base_path arg from GearyApplication constructor added in commmit a3e0e47
Revisiting this again now, I'd be happy if the autostart and GSettings were migrated as part of this bug, but the libsecret id and schema (i.e. Bug 768121) were done later: People to tend to manually play around with user settings, but less so with stored passwords.
Filed bug 775507 to add the desktop file renaming to gnome-shell.
Created attachment 341311 [details] [review] [Patch 1] Use "org.gnome.Geary" for the app ID Patch done again. A few remarks: * One might have 2 desktop-entries installed now, giving 2 icons in the application launcher in the GNOME shell, but the aforementioned bug 775507 should solve this. This also means you can switch back to a previous version of Geary without too much trouble or missing desktop files. * The GSettings-schema is automatically migrated (see the next attached patch). * After some research, I decided against renaming geary-autostart. It is an extra inconvenience and other GNOME applications don't do it either (e.g. GNOME Software has a gnome-software-service.dektop file) * Geary is now DBUS-activatable, like you asked.
Created attachment 341312 [details] [review] [Patch 2] Migrate GSettings schema Complementary patch to migrate our settings to the new App ID
Review of attachment 341312 [details] [review]: Looks good, just a few comment/questions below. ::: src/client/util/util-migrate.vala @@ +115,3 @@ + public static void old_app_config(Settings newSettings, string old_app_id = OLD_APP_ID) { + SettingsSchemaSource schemaSource = SettingsSchemaSource.get_default(); + if (GearyApplication.GSETTINGS_DIR != null) { Pretty sure you can assume GSETTINGS_DIR won't be null here. @@ +122,3 @@ + } + } + SettingsSchema oldSettingsSchema = schemaSource.lookup(old_app_id, false); Do you think this will work going forward? I.e. if an old option is removed from the schema, will it be possible to retrieve its value using the current schema? @@ +126,3 @@ + bool alreadyMigrated = newSettings.get_boolean(MIGRATED_CONFIG_KEY); + if (alreadyMigrated) + return; Is it worth declaring this single-use variable? I.e. couldn't the get_boolean call just go in the if expression? It could probably just get folded into the oldSettings if expression below anyway, so there's not a dangling return in the middle of the function. I guess if you're using such a key, that it's not possible to determine if the migration is needed by simply looking to see if the new config is there? Also, does the migrated-config key need to be added to the schema?
Woah, sorry about the multi reviews. Also, don't worry about this: > Also, does the migrated-config key need to be added to the schema? I found it in the first patch, but maybe wants to get moved into the second patch?
Review of attachment 341311 [details] [review]: Looks good, ta!
(In reply to Niels De Graef from comment #13) > Patch done again. A few remarks: > * One might have 2 desktop-entries installed now, giving 2 icons in the > application launcher in the GNOME shell, but the aforementioned bug 775507 > should solve this. This also means you can switch back to a previous version > of Geary without too much trouble or missing desktop files. Cool. I guess people using packages won't have much trouble with this anyway, as the old one should get removed on upgrade? > * After some research, I decided against renaming geary-autostart. It is an > extra inconvenience and other GNOME applications don't do it either (e.g. > GNOME Software has a gnome-software-service.dektop file) Yep, sounds fine - it just launches the executable anyway, so all good. > * Geary is now DBUS-activatable, like you asked. Nice!
Created attachment 341594 [details] [review] [Patch 2] Migrate GSettings schema - Reworked Reworked patch 2 to consider your remarks: the new version will only migrate settings that are found in both the new and the old schema, and by using the GVariant-methods instead of the type-specific ones, it got much, much cleaner.
(In reply to Michael Gratton from comment #21) > (In reply to Niels De Graef from comment #13) > > Patch done again. A few remarks: > > * One might have 2 desktop-entries installed now, giving 2 icons in the > > application launcher in the GNOME shell, but the aforementioned bug 775507 > > should solve this. This also means you can switch back to a previous version > > of Geary without too much trouble or missing desktop files. > > Cool. I guess people using packages won't have much trouble with this > anyway, as the old one should get removed on upgrade? With the latest branch of gnome-shell 3.22 (or master) one shouldn't notice a thing. Maybe I should announce it on the mailing list to let the packagers know?
Review of attachment 341594 [details] [review]: Much cleaner! Would still be good to drop the GSETTINGS_DIR null check and move the migrated-config addition to the gschema.xml file in the second patch if you can be bothered, but assuming you've tested it out it's otherwise good to commit.
(In reply to Niels De Graef from comment #23) > With the latest branch of gnome-shell 3.22 (or master) one shouldn't notice > a thing. Maybe I should announce it on the mailing list to let the packagers > know? Indeed. I'm not sure how RPMs work, but for debs it should more or less get picked up automatically. Might be worth sending a quick heads-up to the list in any case though so people running master but don't have the gnome-shell updates aren't scratching their heads about why it's disappeared from their favourites.
First patch committed as ec9acc8. Second patch committed as 4b5b2ee. (also moved the migrated-config addition as you asked). The necessary changes to keep flatpak builds working (removing "rename-desktop-file" and "rename-appdata-file") have been committed to gnome-apps-nightly as commit b1211c6.
Nice! Thanks for the email, too. I've just edited your bugzilla rights to add you to the geary_developers group, want to test that out by resolving this as fixed?
(In reply to Michael Gratton from comment #27) > Nice! Thanks for the email, too. > > I've just edited your bugzilla rights to add you to the geary_developers > group, want to test that out by resolving this as fixed? Nice, I've been meaning to ask you about that. Thanks!
For reference, I also added the rename to appstream-glib [1], so we don't show up as a different entry in GNOME Software. [1] https://github.com/hughsie/appstream-glib/commit/bb3da45095aaf4ad50c689095dad0fe2f4e94fb6