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

Implement the settings portal #38

Merged
merged 8 commits into from Oct 6, 2021

Conversation

alice-mkh
Copy link
Contributor

Thankfully, we don't have to implement everything - settings portal
can have multiple implementations and will combine settings from all
of them.

Fixes #37

Screenshot from 2021-09-20 02-39-06

Screenshot from 2021-09-20 02-39-08

Copy link
Contributor Author

@alice-mkh alice-mkh left a comment

Choose a reason for hiding this comment

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

A self-review for things that should probably be changed before merging.

settings-portal/Settings.vala Outdated Show resolved Hide resolved
settings-portal/Settings.vala Outdated Show resolved Hide resolved
public abstract string find_user_by_name (string username) throws GLib.Error;
}

private class AccountsServiceMonitor : GLib.Object {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are multiple options for this:

  • Use Granite instead (and allow Granite to use both portal and accountsservice directly + a way to disable portal there)
  • Have this in the same process as the settings daemon itself, then it can be accessed directly from there instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is still an open question. We can also leave it as is, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OTOH if it's in the same process Granite will need an override to not try to access portal from portal itself too, having it separate doesn't require it as long as the portal binary doesn't link to granite.

settings-portal/Main.vala Outdated Show resolved Hide resolved
@alice-mkh alice-mkh force-pushed the wip/exalm/portal branch 2 times, most recently from 0929da6 to e132254 Compare September 19, 2021 21:57
Thankfully, we don't have to implement everything - settings portal
can have multiple implementations and will combine settings from all
of them.

Fixes elementary#37
@danirabbit
Copy link
Member

Just wanted to say thank you so much for doing all of this work! It's massively appreciated ❤️

Copy link
Contributor

@Marukesu Marukesu left a comment

Choose a reason for hiding this comment

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

Some comments, thanks for working in this.

settings-portal/Error.vala Outdated Show resolved Hide resolved
settings-portal/Settings.vala Show resolved Hide resolved
settings-portal/Settings.vala Outdated Show resolved Hide resolved
settings-portal/Settings.vala Outdated Show resolved Hide resolved
settings-portal/Settings.vala Outdated Show resolved Hide resolved
Copy link
Contributor

@Marukesu Marukesu left a comment

Choose a reason for hiding this comment

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

Works as expected.

However, one thing i noticed is, the portal frontend sort settings implementations by name not by the backend UseIn key, so the GTK portal will have priority.

@alice-mkh
Copy link
Contributor Author

Ugh, and in that case -gtk will be prioritized over -gnome as well. Maybe it should have a separate priority key.

@GeorgesStavracas
Copy link

Ugh, and in that case -gtk will be prioritized over -gnome as well. Maybe it should have a separate priority key.

To the best of my knowledge, "gnome" is sorted before "gtk". Here's some evidence!

>>> a = ['gtk','gnome','pantheon','kde']
>>> a.sort()
>>> print(a)
['gnome', 'gtk', 'kde', 'pantheon']

@alice-mkh
Copy link
Contributor Author

gnome.portal
gtk.portal
io.elementary.settings-daemon.portal

Ah indeed, I assumed it was org.gnome.Portal or sth without checking.

@GeorgesStavracas
Copy link

For what is worth, I remember talking to Matthias about a more sophisticated sorting in the past, and the conclusion was that we would cross this bridge when it comes to it. Seems like the time to to cross that bridge has come.

@danirabbit danirabbit added this to In progress in OS 6.1 via automation Sep 22, 2021
@danirabbit danirabbit removed this from In progress in OS 6.1 Sep 22, 2021
Copy link
Member

@davidmhewitt davidmhewitt left a comment

Choose a reason for hiding this comment

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

Just one super minor tweak and then this is good, thanks for all your hard work! ❤️

I have a couple of questions that would be good to have clarified here for posterity:

From reading above, it sounds like the GTK and GNOME portals take precedence over this elementary portal based on the current code in xdg-desktop-portal. What I understand from that is that at the point where we get a version of the GNOME (or GTK) portal that starts returning a value for the org.freedesktop.appearance.color-scheme key, then that portal implementation will be considered the source of truth for that value? Meaning that this won't work right because our settings pane will be writing to AccountsService and the "main" portal will be reading from whatever the GNOME backend implementation is? So it's not a blocker to merging this right now, but there's a piece of work still outstanding here to get more complicated prioritisation in xdg-portal-gtk, or we need to work around this somehow in elementary in the future?

Secondly, am I correct that the work required in Granite to have it read from here is essentially a D-Bus call to org.freedesktop.portal.Settings Read ? There's not a "nice" GSettings-esque way of doing it essentially because of this feature request? https://gitlab.gnome.org/GNOME/glib/-/issues/2213

settings-portal/Main.vala Outdated Show resolved Hide resolved
@alice-mkh
Copy link
Contributor Author

but there's a piece of work still outstanding here to get more complicated prioritisation in xdg-portal-gtk, or we need to work around this somehow in elementary in the future?

It will need prioritization, yes. I don't know of any work in this area, one thing that could be done is renaming the portal file to something different, e.g. elementary-settings-daemon.portal.

Secondly, am I correct that the work required in Granite to have it read from here is essentially a D-Bus call to org.freedesktop.portal.Settings Read?

Correct - GTK does the same for GtkSettings. There's already a PR though: elementary/granite#529

@danirabbit danirabbit merged commit b364954 into elementary:master Oct 6, 2021
@WhyNotHugo
Copy link

Hi all.

From reading above, it sounds like the GTK and GNOME portals take precedence over this elementary portal based on the current code in xdg-desktop-portal.

This is being a problem for me too on a separate situation (flatpak/xdg-desktop-portal#710). AFAIK, it's a bug on xdg-desktop-portal, right? Do you have any further insight onto it, or is it simple a non-issue for your usage?

@alice-mkh
Copy link
Contributor Author

The priority is decided based on the name (and portals are filtered based on your session, GTK one just loads everywhere), so just name your portal so it comes before others.

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.

Provide the settings portal
6 participants