-
Notifications
You must be signed in to change notification settings - Fork 926
core, gtk: implement host resources dir for Flatpak #6661
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
Conversation
4d88103
to
53e5efc
Compare
53e5efc
to
f76d334
Compare
Changes in v2:
|
f76d334
to
4982c0d
Compare
Changes in v3:
|
e1e708a
to
9aa7f5d
Compare
Changes in v4:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its better, but I still don't love this approach...
Calling resourcesDir
twice once for guest once for host is not great, especially since 99% of users (today, but a significant number) are not running in a sandboxed mode. Its just waste.
A perhaps better approach would be to return a struct instead so we can get all the info in one call. But stepping back its not clear to me if we ever need both, would it be more useful to redocument resourcesDir
as always being host accessible?
For platforms like Flatpak the host-accessible directory is usually non-accessible in the sandbox, but the sandbox path is not accessible outside. We need sandbox-accessible paths for things like reading bundled themes, and only termio needs the host-accessible one since that's where host processes are launched. So on Flatpak you'd want both to implement all the features. |
9aa7f5d
to
de54443
Compare
Changes in v5:
|
de54443
to
696d3b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks sound, just one small nitpick
696d3b2
to
255aa59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better. Thanks!
IMO there isn't much that's blocking this PR right now and I'd like to get this merged for the Flatpak build to become more usable, but as this is a core change I would have to hear from someone else first. @mitchellh @jcollie you both have previously requested changes - any thoughts at the moment? |
255aa59
to
f54cce3
Compare
Rebased to solve merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time to re-review right now but I'll trust @pluiedev 's judgement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty solid. I'll also leave it to Leah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a couple reallyyyyy small things that would be good and quick to address.
I'm going to approve this so that we can merge once they're addressed and I don't block anyone.
Introduces host resources directory as a new concept: A directory containing application resources that can only be accessed from the host operating system. This is significant for sandboxed application runtimes like Flatpak where shells spawned on the host should have access to application resources to enable integrations. Alongside this, apprt is now allowed to override the resources lookup logic.
f54cce3
to
faf9d59
Compare
Actually I just quickly addressed them myself. CI running now, auto merge on! |
Introduces host resources directory as a new concept: A directory containing application resources that can only be accessed from the host operating system. This is significant for sandboxed application runtimes like Flatpak where shells spawned on the host should have access to application resources to enable integrations.
Alongside this, apprt is now allowed to override the resources lookup logic.