Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to GSettings #470

Merged
merged 21 commits into from
Dec 24, 2019
Merged

Port to GSettings #470

merged 21 commits into from
Dec 24, 2019

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Nov 2, 2019

This refactors the configuration infrastructure away from the deprecated
GConf library and towards GSettings.

This refactors the configuration infrastructure away from the deprecated
GConf library and towards GSettings.
@bgamari bgamari changed the title WIP: Port to GSettings Port to GSettings Nov 2, 2019
Copy link
Collaborator

@ederag ederag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this migration.
Looks like you know what you are doing, good !

waf has a buit-in functionality to install schemas (#64 (comment)).
Let's use that standard if possible ?

About that, from https://developer.gnome.org/GSettings/

There are several examples in the wild of projects that use .gschema.xml.in files.
This is almost always wrong.
You should absolutely never use intltool or itstool-based XML "translation merging" functionality with schema files.

So that will need to be changed as well, to be standard,
but let's wait for @GeraldJansen to review and test this first.

Another link https://developer.gnome.org/gio/stable/GSettings.html

the default values in GSettings schemas can be localized

But maybe are you also already familiar with this internationalization stuff ?

client = gconf.Client.get_default()
self.timeout_minutes = client.get_int(delay_key)
settings = gio.Settings()
self.timeout_minutes = client.get_value(delay_key)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A guess

Suggested change
self.timeout_minutes = client.get_value(delay_key)
self.timeout_minutes = settings.get_value(delay_key)

@GeraldJansen
Copy link
Contributor

Not sure about the importance of this build message:

./waf configure build
...
[126/126] Compiling data/hamster-time-tracker.desktop.in.in
feature 'glib2' does not exist - bind at least one method to it?     <===
[127/254] Compiling data/apps.hamster-time-tracker.gschema.xml.in
...
'build' finished successfully (1.708s)

and also

sudo ./waf install
...
+ install /usr/share/hamster-time-tracker/stats.ui (from data/stats.ui)
feature 'glib2' does not exist - bind at least one method to it?      <===
+ install /usr/share/applications/hamster-time-tracker.desktop (from build/data/hamster-time-tracker.desktop)
...
'install' finished successfully (0.274s)

Then

 /usr/bin/hamster

(hamster:5992): GLib-GIO-ERROR **: 11:43:23.021: Settings schema 'apps.HamsterTimeTracker' is not installed
Trace/breakpoint trap (core dumped)

@ederag
Copy link
Collaborator

ederag commented Nov 3, 2019

glib2 has to be loaded first.
Probably add

conf.load('glib2')  # for gsettings schemas installation`

in the wscript file, configure method,
maybe replacing those lines

hamster/wscript

Lines 26 to 27 in 2d58bf6

# gsettings_schema_dir is defined in options
conf.env.schemas_destination = '${DATADIR}/glib-2.0/schemas'

@GeraldJansen
Copy link
Contributor

Works for me.

@bgamari
Copy link
Contributor Author

bgamari commented Nov 3, 2019 via email

Copy link
Collaborator

@ederag ederag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small changes found while looking for the schema name.

src/hamster/lib/configuration.py Outdated Show resolved Hide resolved
src/hamster/lib/configuration.py Outdated Show resolved Hide resolved
@ederag
Copy link
Collaborator

ederag commented Nov 3, 2019

@bgamari The DBus interface should remain stable (org.gnome.Hamster)

bus_name = dbus.service.BusName("org.gnome.Hamster", bus=self.bus)

So we should keep this name.
Maybe one day this application will get back under the gnome organization umbrella;
that's not planned, but who knows.

@GeraldJansen Does it mean that we should rename the database directory to hamster as well ?

@GeraldJansen
Copy link
Contributor

Does it mean that we should rename the database directory to hamster as well ?

For me, .local/share/hamster-time-tracker is consistent with /usr/share/hamster-time-tracker and other installation directories and file names. Either rename everything to just hamster or use hamster-time-tracker consistently, I'd say.

@ederag
Copy link
Collaborator

ederag commented Nov 4, 2019

Either rename everything to just hamster or use hamster-time-tracker consistently

Agreed.
There might be good reasons to keep the more expressive hamster-time-tracker directories.
I'll file an issue to discuss that.

@ederag ederag added this to the v3.0 milestone Nov 23, 2019
@ederag
Copy link
Collaborator

ederag commented Nov 23, 2019

@GeraldJansen would you have time to implement the directory names changes (#472) and
thoroughly review this PR ?
This is very important indeed, and will get into v3.0,
but the parser update currently takes all my focus.

@ederag ederag mentioned this pull request Nov 23, 2019
@GeraldJansen
Copy link
Contributor

@ederag I'm willing to try.

@GeraldJansen
Copy link
Contributor

@bgamari In #485 hamster-time-tracker has been renamed to hamster everywhere. This has caused a few conflicts with your PR. Would you mind rebasing on current master?

@ederag
Copy link
Collaborator

ederag commented Nov 26, 2019

@GeraldJansen If @bgamari is not available at this time
(really hoping we did not lose such a valuable contributor),
another workflow might suit you:
it is possible to branch from this PR,
merge (not rebase) upstream/master,
make additional changes,
and push them to your fork.
Then either
make a PR on the fork, taking care to
select the bgamari/hamster fork gsettings branch at the "compare across forks" step
(that would allow both of you to discuss and converge there)
or
tell me here to cherry-pick your branch and push it for you to this PR
(you would remain the author of your commits IIUC),
so that all the discussion stays there.

@GeraldJansen
Copy link
Contributor

@ederag Well, I'm out of my depth here, both concerning advanced git techniques and gsettings. I've created a replacement PR (take 2) with a single additional commit (1e08ebe) which merges in current master and adds a couple minimal changes. If you prefer to add the last commit to this PR and close the other, that would be fine by me too.

@ederag
Copy link
Collaborator

ederag commented Nov 26, 2019

That went well, thanks !
Next time you may just push, without creating any PR, and just give me your branch name
(or its last commit).

@GeraldJansen
Copy link
Contributor

Tried to contact @bgamari also by private email but got no response.
From my side I'd say this is good to go.

@ederag
Copy link
Collaborator

ederag commented Dec 15, 2019

external, desktop and idle have to be removed first (#493),
to start with a clean schema.

@GeraldJansen
Copy link
Contributor

@ederag I am currently traveling and will not be able to work on this until January. Please feel free to proceed.

@ederag
Copy link
Collaborator

ederag commented Dec 15, 2019

Sure, already started (that's why it was added to maintenance).
It was obvious you were unavailable, but it's good to know you are fine. Happy traveling !

@ederag
Copy link
Collaborator

ederag commented Dec 22, 2019

@bgamari Your PR was great, only slightly polished.
Sliced in small commits to allow easy reversal,
but the overall change with respect to your initial PR is very small.
You might want to try the parser-updates-with-gsettings branch that merges your PR into #482 .

Note: the schema localization seems to follow the recommendations, but has not been tested yet.

Is it OK to merge ?

@ederag
Copy link
Collaborator

ederag commented Dec 24, 2019

The schema localization works on openSUSE Leap-15.1, after an update of the french po/fr.po
and installation of hamster.

> LANGUAGE=fr gsettings describe org.gnome.Hamster last-report-folder
Le dernier répertoire utilisé pour enregistrer un rapport

@bgamari
Merging so the snap package (created by @extraymond, initially in #418) can follow the master branch.
Please file an issue if anything is wrong.
Thanks again for the great contribution !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants