-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Add dynamic launcher portal #696
Add dynamic launcher portal #696
Conversation
0ffeefc
to
83b245e
Compare
Do we really still care about Ubuntu 16.04 support? It is EOL except for their ESM program. |
79723aa
to
9584207
Compare
I made the new portal only compile when we have GLib 2.66, so we can still maintain compatibility for the existing code with old GLib for the sake of security patches. |
I realized that the implementation currently doesn't allow the |
I also notice that the notification portal takes an icon as a GVariant (made with |
The corresponding x-d-p PR is here: flatpak/xdg-desktop-portal#696
309f304
to
22f294c
Compare
The corresponding x-d-p PR is here: flatpak/xdg-desktop-portal#696
The corresponding x-d-p PR is here: flatpak/xdg-desktop-portal#696
I'm not sure it's worth it...? My recollection is that a D-Bus message can be 10MB in size, and the icons are expected to be significantly smaller. Requiring a fd means that if you are composing / sizing / etc the icon dynamically you are forced to place it on disk or set up a memory-backed fd (/dev/shm I guess?) which is a bit of a pain. @matthiasclasen may have an opinion on this but for a relatively rare process of adding an app, requiring an fd might be overkill. If we were streaming video, sure... :) |
Yeah the way it's currently implemented:
So if message size limits aren't an issue it might make sense to pass a GVariant. |
Separately, there is the question of how to handle installation of a launcher that would overwrite an existing one. Currently the portal just does the overwrite without confirmation, but Epiphany checks if the app already exists before calling the install method and presents a confirmation dialog if so. It could make sense to have some sort of overwrite confirmation in the portal implementation, but having it controlled by the application has the advantage that it can clear any other state associated with the launcher being overwritten. However it does mean that the confirmation dialog is separate rather than a child of the main portal dialog. |
I checked 18 PWAs and found one icon that was 275 kB but all the rest were an order of magnitude smaller. |
Message size would be an issue for photos. I don't see it being a problem for icons |
c00e0a2
to
ecd95d1
Compare
This is based on the "Create Web Application" dialog from Epiphany, adjusted to be more generalizable. The x-d-p side of this is here: flatpak/xdg-desktop-portal#696
Okay, I can change the implementation to use a GVariant instead for the icon. In other news, I just was able to successfully test this with unsandboxed Epiphany. Still need to test sandboxed Epiphany but it seems to be working so far. |
The corresponding x-d-p PR is here: flatpak/xdg-desktop-portal#696
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.
One common pattern in portals that can provide a variety of options is to expose the availability of these options using D-Bus properties. In this case, I think it needs a SupportedLauncherTypes
property (uint32):
SupportedLauncherTypes:
A bitmask of available launcher types. Currently defined types are:
<simplelist>
<member>1: Application. A launcher that represents an application.</member>
<member>2: Link. A launcher that represents a webapp.</member>
</simplelist>
PrepareInstall()
would then receive a launcher_type
(uint32) as part of the options
vardict. Knowing ahead of type what kind of launcher it is will allow portal implementations to provider more specialized interfaces. Despite being technically a bitmask, PrepareInstall()
must only accept one type. ¹
By providing a launcher_type
, the naming of the url
option doesn't feel natural to me - perhaps rename to target
? Ideas welcomed.
¹ - This can be validated with __builtin_popcount(launcher_types) == 1
.
That makes sense to make the launcher types more explicit. But I guess application launchers wouldn't have a target so it would be NULL in that case. Still seems fine to name it |
Currently we only add a
For Epiphany I couldn't find an easy way to get the absolute path to the binary so I had it add |
Looks like there's no easy way to tell the file format of a serialized |
a1cfb04
to
06f8c05
Compare
This pulls in the version of validate-icon.c on the Flatpak main branch commit 8b3728addb, which prints the information we need using GKeyFile rather than a string separated by a ':', as discussed in flatpak/flatpak#4803
This should fix the Ubuntu 16.04 CI build.
This matches what happens in Flatpak and in xdg-desktop-portal-gnome. First parse the argv so it's unquoted then quote any arguments that need it. Don't do so in the background portal since the backend handles it. Also don't allow Flatpaks to use the --file-forwarding argument for "flatpak run".
900e3d6
to
de13890
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.
Looks good. We shall land this now. Thanks!
@mwleeds let's focus on the -gnome and -gtk portal implementations now. I've filed https://bugs.kde.org/show_bug.cgi?id=451510 for KDE. |
Sounds good thanks for filing that. Of course there are other back-ends as well but I don't think we have an obliglation to file a ticket for each one. |
The corresponding x-d-p PR is here: flatpak/xdg-desktop-portal#696
This is based on the "Create Web Application" dialog from Epiphany, adjusted to be more generalizable. The x-d-p side of this is here: flatpak/xdg-desktop-portal#696
This makes use of the new dynamic launcher portal (flatpak/xdg-desktop-portal#696) to install and uninstall web apps. This means we are able to support web apps now in Flatpak'd Epiphany. Unfortunately it's hard to test though so for now remove the unit tests. Disable the app manager feature under Flatpak since it's hard to implement (e.g. the portal doesn't provide access to installed apps' icons) but we don't need it since Software will be able to install/uninstall these web apps.
This is based on the "Create Web Application" dialog from Epiphany, adjusted to be more generalizable. The x-d-p side of this is here: flatpak/xdg-desktop-portal#696
Is it possible to add the docs for the new portal? |
Yup, thanks for the reminder: flatpak/flatpak-docs#325 |
The corresponding x-d-p PR is here: flatpak/xdg-desktop-portal#696
The corresponding x-d-p PR is here: flatpak/xdg-desktop-portal#696
This makes use of the new dynamic launcher portal (flatpak/xdg-desktop-portal#696) to install and uninstall web apps. This means we are able to support web apps now in Flatpak'd Epiphany. Unfortunately it's hard to test though so for now remove the unit tests. Disable the app manager feature under Flatpak for now; a later commit will re-enable it.
Since flatpak#696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
Since flatpak#696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
Since flatpak#696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
Since flatpak#696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
Since flatpak#696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
Since #696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
Since #696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
Since flatpak#696, the arguments sent to background portal implementations contained the app-id in quoted form. Implementations, in turn, created invalid autostart files.
This is not ready to be merged since I haven't implemented the gnome backend or the libportal side of it, and I haven't tested it. But it can be reviewed.
For context see #681 and #326