-
Notifications
You must be signed in to change notification settings - Fork 355
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
Url handlers #5059
Url handlers #5059
Conversation
(note this is still work in progress. it's not ready to go in yet) |
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 to me, thanks for this. Just a note - the desktop file will end up in the installed system the same way as Anaconda. I guess it's not ideal but works.
@martinpitt do you think the cockpit.spawn could be enhanced to solve this? I guess not but I would like to ask you first. |
Not in future proof way without firefox changes. Right now firefox uses X11, and the "context" i mentioned is just a timestamp which I believe can be accessed by javascript. So in theory, i think, it could be made to work today by passing the timestamp through cockpit.spawn. With wayland, the "context" is a token that has to be requested from the compositor. This isnt available from javascript, and would need some api worked out and implemented in firefox. |
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.
Thanks @halfline for improving this!
the desktop file will end up in the installed system the same way as Anaconda
That seems fine, though -- anaconda isn't installed on the final system anyway.
do you think the cockpit.spawn could be enhanced to solve this
I don't think so, no -- as @halfline said this is not a stable/public API in the general case between compositors, and it may not even work with Chromium or webkit.
Also, this is the only case of a graphical app being launched through the cockpit API -- this hack only works at all due to the "special nature" of webui-desktop
. So this doesn't generalize well.
Do you have an integration test that this button works? By the sound of it, it needs to be continuously tested with changing browsers, GNOME, etc.
<Button variant="link" icon={<WrenchIcon />} onClick={() => cockpit.spawn(["blivet-gui"])}> | ||
<Button variant="link" icon={<WrenchIcon />} | ||
onClick={() => { | ||
var onBeforeUnload = window.onbeforeunload; |
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.
Use const
instead of var
.
[Desktop Entry] | ||
Name=Anaconda Storage |
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.
Does this appear anywhere visible to the user? I.e. does this need i18n?
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.
nope, it's NoDisplay=true, shouldn't show up anywhere in the ui
Um, anaconda is definitely installed. It just so happens that when you have a live environment with anaconda, that is installed verbatim, so you get anaconda. |
Anaconda is installed on the final system - everything that is installed on the Live image when its created ends up on the installed system. That's how Live image installation works - its is just an rsync of the Live image rootfs to the target system. That's also why Blivet GUI does not have a desktop icon on the Live image - so that it won't visibly show on the installed system (this was requested by the Workstation SIG long ago). There have been some attempts to uninstall Anaconda and possibly other things from the image after the installation (either by Anaconda or some first-run systemd unit) but this is tricky, as there might not always be network connectivity and even uninstalls apparently can require new packages to be installed, which will fail without network connectivity. This is the latest effort, that was eventually abandoned: https://fedoraproject.org/wiki/Changes/AutoFirstBootServices
Right now we need to be able to launch Blivet GUI and another Firefox window for error reporting to satisfy core requirement.
Looks like this could be tested by OpenQA - @AdamWill will know more. Still, there is the unresolved matter of how will launching of Anaconda from the Live session work - another run in a separate environment or somehow switching back to the minimal session as suggested by Alan ? |
openQA could test this; as things stand it will not at first, because we don't do any custom installs from the Workstation live (all the custom install testing happens on the Server DVD). I guess we will need to move or duplicate some such tests on the Workstation live, if we're going forward with the webUI change for F39, as we'll need to actually check that the custom workflow works on webUI :/ |
@@ -130,6 +130,7 @@ const InstallationDestination = ({ deviceData, diskSelection, dispatch, idPrefix | |||
}); | |||
|
|||
const rescanDisksButton = ( | |||
|
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.
odd extra space here
Couldn't anaconda and related packages be installed a in separate payload on the live cd and just get mounted to /var/lib/extensions/anaconda by the livesys scripts? systemd should automatically overlay it into the live image I think, and the rsync shouldn't see it since you're just rsyncing from the base layer if i understand things right. to be clear i mean something like this in lorax (untested):
before packing up the iso. Then at boot systemd should put anaconda in /usr automatically but the rsync won't see it. |
okay so one issue i'm running into is firefox wants to get confirmation from the user that the external url handler is okay. That's less than optimal... I've found I can stop this confirmation request by doing this: sqlite3 ${FIREFOX_PROFILE_PATH}/permissions.sqlite "INSERT INTO moz_perms VALUES(2,'http://127.0.0.1','open-protocol-handler^anaconda-storage',1,0,0,strftime('%s','now')); but it's a little ugly... Also, we don't ship sqlite3 on the live image. We should maybe be pre-generating the firefox profile at build time |
I suppose Anaconda has a lot of control over how it starts firefox. It ought to start it with a temporary However, we can assume Python in the live system, no? That can talk to sqlite3 natively. |
@bcl @ondrejbudai what do you think about #5059 (comment) as future improvement? |
@supakeen can you take a look? |
We don't currently do overlay mounts in osbuild, nor do we use dnf to install the packages (we use RPM directly). It'd be a fair bit of work to support such a scheme but possible and well worth it to not have anaconda on the installed system. |
so I chatted with @stransky and he has a much better solution: We just need to make sure the url handler is listed in mimeapps.list and firefox won't do any warning! I tested it and it seems to work great. So I'll rebase this branch to master and drop draft status shortly. |
cockpit.spawn isn't a good command to run UI tools because it doesn't know the user event that lead to it getting called. UI applications need to be run with that sort of context in their environment or compositors won't start them in the foreground. This is so if a user is typing, a slow starting program doesn't steal focus inadvertently when it finally starts. Firefox has a mechanism for launching applications with the appropriate context, however: Url Handlers. This commit registers a url handler "anaconda-storage:" and changes the Web UI to use it instead of cockpit.spawn.
08a344c
to
bea7d30
Compare
Okay this is pretty much ready to go in. Some caveats:
|
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.
LGTM, I think...
onClick={() => { | ||
const onBeforeUnload = window.onbeforeunload; | ||
window.onbeforeunload = null; | ||
window.location.href = "anaconda-storage:"; |
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.
Will it have the correct address afterwards?
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.
it's not redirecting away from anaconda if that's what you mean. It's akin to having a "mailto:" link in a web page. it just fires up the email client^W^Wblivet
@halfline do you mind if I rebase you PR and get it in asap? I don't see why it was delayed merging - thanks for improving this. |
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.
@halfline I just realized that this will soon become obsolete, as we are planning to replace in f40 the blivet-gui with the cockpit-storage.
The later has not need of this hack, so I advice to not include this, as we would need anyhow to revert it soon.
I will close this, please don't spend extra time on this. Thanks and sorry for the delayed action.
okay you might want to use this as the basis for bug reporting (unless that's getting done in process too) |
@halfline for bug reporting we open the URL with window.open(reportURL, "_blank", "noopener,noreferer") From my understanding of this PR, we will not get any gain from switching that, since it's not a UI application spawned. Am I maybe missing something? So the blivet-gui was the only case where we would launch another UI application from the anaconda Web UI. |
at some point someone (@jkonecny12 ? @M4rtinK ? not sure ) mentioned to me that the bug reporting ui was going to be turned into separate program. if thats not happening, nevermind! |
@KKoukiou the other case I'm aware of where oldUI at least spawns another app is nm-connection-editor. is that not going to be a thing in webUI? |
Right now @AdamWill it seems that we will be able to avoid that. However, networking is not yet resolved so the path is unclear there. |
cockpit.spawn isn't a good command to run UI tools because it doesn't know the user event that lead to it getting called. UI applications need to be run with that sort of context in their environment or compositors won't start them in the foreground. This is so if a user is typing, a slow starting program doesn't steal focus inadvertently when it finally starts.
Firefox has a mechanism for launching applications with the appropriate context, however: Url Handlers.
This commit registers a url handler "anaconda-storage:" and changes the Web UI to use it instead of cockpit.spawn.