mgalgs / gnome-shell-system-monitor-applet

Display system informations in gnome shell status bar, such as memory usage, cpu usage, network rates…
https://extensions.gnome.org/extension/3010/system-monitor-next/
GNU General Public License v3.0
220 stars 19 forks source link

Rename gschema file to avoid conflict with other extensions #65

Closed NVieville closed 1 week ago

NVieville commented 8 months ago

Signed-off-by: NVieville nicolas.vieville@uphf.fr


This change is Reviewable

NVieville commented 8 months ago

Hello,

This PR allows to deal with global installation of the extension, where the gschema XML file has to be copied in global directory in order to be available or processed (typically Fedora RPM installation). This is maybe the case too on user's home directory local installations. See https://bugzilla.redhat.com/show_bug.cgi?id=2268400 for an example of the conflict.

Any comment are welcome.

Cordially,

-- NVieville

mgalgs commented 8 months ago

Interesting... nice find... This looks like a good change but I'm concerned about breaking existing users' installations. It would be great if we could copy the old schema to the new location if we detect the old filename (and verify that it is indeed for this extension, not the other one), but I'm not sure if that's possible during the extension lifecycle.

I'll have to look into this later unless someone more familiar with extension architecture can comment.

NVieville commented 8 months ago

Hello,

Thanks for your response.

According to https://gjs.guide/extensions/development/preferences.html the schemas directory containing the extension settings is located in the user's local directory for the extension (for example ~/.local/share/gnome-shell/extensions/example@gjs.guide/schemas) in case of a local installation. In case of a global installation, the schemas file is located in the /usr/share/glib-2.0/schemas/ global directory.

If this is really the way things are done, there should not have problems when renaming the schemas file and modifying the other files accordingly for a newer version of the extension in case of user's local installation. The only caveat I can see about it, is that the old file could remain in the schemas directory of the extension regarding the way the upgrade is done through the extension manager in the browser (I don't know what is done or not, for example is the zip file just deflated in the directory or the old files or directory are erased before).

The problem remains with global installation of the extension, but as this should be operated through a package manager or manually, we should consider that renaming the schemas file and modifying the other files accordingly will not be a problem (removing old files before installing new ones is done by the package manager).

My 2 cents about this. Maybe I missed some other point, feel free to make any comment.

Cordially,

-- NVieville

ZimbiX commented 8 months ago

What's the conflict with? The upstream/abandoned extension? Perhaps the new name should match the rest of this fork, using '-next'

NVieville commented 8 months ago

Hello,

@ZimbiX if you look at the bug report cited in my first message you'll see that the conflict is with the standard gnome shell extension: gnome-shell-extension-system-monitor. There seems that this extension named gnome-shell-system-monitor-applet, renamed gnome-shell-extension-system-monitor-applet to comply with Fedora packaging guidelines uses the same name for its schemas file as the original one. I think we should rename the schemas file of this one in order to not conflict with the standard gnome shell extension. I cannot comment on a broader name change.

Cordially,

-- NVieville

mgalgs commented 8 months ago

As long as we're renaming I agree that we should insert the -next somewhere.

I still want to figure out if we can carry over user settings from the old schema ID for existing users during extension startup or something. If I'm not mistaken the schema ID rename will cause existing users to lose their settings (since their customizations would be saved under the old schema ID).

Speaking of schema IDs, don't we also need to make this change here:

https://github.com/mgalgs/gnome-shell-system-monitor-applet/blob/2a581046085eab40dd20367ba424e04b7ec434a5/system-monitor-next%40paradoxxx.zero.gmail.com/schemas/org.gnome.shell.extensions.system-monitor.gschema.xml#L2-L12

glerroo commented 8 months ago

This extension ArchMenu use in settings window dconf dump & dconf load to save and load preferences.

NVieville commented 8 months ago

As long as we're renaming I agree that we should insert the -next somewhere.

I still want to figure out if we can carry over user settings from the old schema ID for existing users during extension startup or something. If I'm not mistaken the schema ID rename will cause existing users to lose their settings (since their customizations would be saved under the old schema ID).

Speaking of schema IDs, don't we also need to make this change here:

https://github.com/mgalgs/gnome-shell-system-monitor-applet/blob/2a581046085eab40dd20367ba424e04b7ec434a5/system-monitor-next%40paradoxxx.zero.gmail.com/schemas/org.gnome.shell.extensions.system-monitor.gschema.xml#L2-L12

Yes, indeed.

https://gitlab.gnome.org/GNOME/gnome-shell-extensions/-/blob/main/extensions/system-monitor/schemas/org.gnome.shell.extensions.system-monitor.gschema.xml?ref_type=heads

This file shows that the collision with the official gnome shell system monitor extension is possible. So it seems mandatory to rename the root name of this extension in the schemas file (adding the word applet or next at the end).

Concerning user's settings, they are stored in a local binary database located in ~/.config/dconf/user file. These settings are stored using the schemas described in the file provided with each extension. So, it seems that conflicts can also arrive in the database in case of co-installation of the two extensions. It should be reasonable to consider modifying the actual schemas file of the extension to avoid such a case.

For my part since the use of the gnome-shell-extension-system-monitor-applet, nearly 12 years, I never had any problem, because I never used the official gnome extension.

Cordially,

-- NVieville

ZimbiX commented 8 months ago

if you look at the bug report cited in my first message

Sorry, I was sleep-deprived and missed that! Thanks for the context

mgalgs commented 8 months ago

Regarding the settings migration, you might be able to do something like this:

const oldSettings = new Gio.Settings({ schema_id: OLD_SCHEMA_ID, path: OLD_SCHEMA_PATH });
const newSettings = new Gio.Settings({ schema_id: NEW_SCHEMA_ID, path: NEW_SCHEMA_PATH });
for (const settingKey of oldSettings.list_keys()) {
    const value = oldSettings.get_value(settingKey);
    newSettings.set_value(settingKey, value);
}

And then we'll need some kind of flag to indicate that the settings were already migrated (so that we don't do this every time)... We could wipe out the old settings completely but that's always risky (user might downgrade after migration, for example). Maybe we stash a "migrated=true" in the old settings database or something (and then just leave the old db there)...

Still not exactly sure where to shove this... Possibly in the SMGeneralPrefsPage constructor??

mgalgs commented 3 months ago

Fixes #89

mgalgs commented 1 month ago

Just wanted to check in and say I haven't forgotten about this. I'm hoping to get around to implementing the settings migration hook soon (unless someone else beats me to it) so that we can merge this.

NVieville commented 1 month ago

Thank you very much for the heads-up. Unfortunately, I don't think I could beat you on this: heavy load at work.

mgalgs commented 2 weeks ago

Superseded-by: #98