portal-impl: fix config ordering #1240
Conversation
GeorgesStavracas
added
bug
config
alyssais
force-pushed
the
order
branch
2 times, most recently
from
27d2100 to
2321ed8
Compare
There was a problem hiding this comment.
LGTM with the final comment above, but I'd like to have @ebassi's review before landing
Quoting portals.conf(5): > Each key in the group contains a semi-colon separated list of portal backend > implementation, to be searched for an implementation of the requested interface, > in the same order as specified in the configuration file. But this wasn't actually true. If the portals were set to z;y and z and y both implemented the same portal, y would be used. Fixing this requires reworking how portals are selected — going through the config file and selecting the first available configured portal, rather than going through each known portal and checking whether it's allowed in the config file. find_all_portal_implementations() is unchanged. The portal-first approach is fine for it, and it would be difficult for it to share as much code with find_portal_implementation() now that it's written to stop as soon as a configured portal is found. Fixes: flatpak#1111
|
ping @ebassi |
|
(I'm not sure to what extent 1394bb2 affected this — there's definitely some overlap, because I also fixed that bug. But given it's caused complicated merge conflicts, and it was merged instead of this despite my PR being open way longer, I want to feel confident that's not going to happen again before I put any more time in.) |
Hi @alyssais, I'm the author of 1394bb2. I was aware of this PR when I wrote that patch, and I tried it out see if the bug was already fixed (it didn't). I think the only reason that my PR was merged faster than yours was that it was bumped up by several users from the Arch community. I think this is a very important PR that I think should get merged since it has been the cause of bugs like this in the distribution that I contribute to. We currently use a very hacky workaround, which this PR would remove the need for. |
|
LMK if you need any help with testing or resolving conflicts. |
|
@SoumyaRanjanPatnaik if you want to resolve the conflicts that'd be great! I had a go but I found it difficult because it's been so long I've forgotten a lot of the nuances I was trying to get right here. |
|
@SoumyaRanjanPatnaik, if you can provide a separate PR that supersedes this one, using |
This is the 'new' way of declaring portal support, but the order of precedence that this was supposed to fix is currently broken, and the Gtk portal services some requests that our implementation (xdg-desktop-portal-xapp) should be handling. The xdg-destop-portal-xapp package still installs the 'deprecated' xapp.portal file, with which everything still works as it should. We can re-add this file once we confirm upstream has fixed the ordering issue. Ref: flatpak/xdg-desktop-portal#1240
@smcv I've created a PR that supersedes this one. |
|
#1378 was merged. Apologies for this mess. |
Quoting portals.conf(5):
But this wasn't actually true. If the portals were set to z;y and z and y both implemented the same portal, y would be used.
Fixing this requires reworking how portals are selected — going through the config file and selecting the first available configured portal, rather than going through each known portal and checking whether it's allowed in the config file.
find_all_portal_implementations() is unchanged. The portal-first approach is fine for it, and it would be difficult for it to share as much code with find_portal_implementation() now that it's written to stop as soon as a configured portal is found.
Fixes: #1111