-
Notifications
You must be signed in to change notification settings - Fork 17
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 action for launching installed applications #266
Conversation
@rinigus and @piggz, to properly review and test this while still fighting the flu is not going to work at all, because reviewing this is at the border of my capabilities, any way. But if one of you reviews / tests and signals "O.K." (with or without introducing changes or requesting ones from jamieMF), I will create a release v0.6.7. Binaries to test are here: https://github.com/sailfishos-chum/sailfishos-chum-gui/actions/runs/7258613885 |
@nephros, you would do the progress of the SailfishOS:Chum GUI app a big favour, if you add concise comments to the two open review comment threads above. I did what I can, but neither @piggz or @jaimeMF respond any longer, this is why I am resorting to you; sometimes (more often lately) it appears that we are the only ones left who care about community-related infrastructure software for SailfishOS. 😞 |
i believe it's not not caring, it's about resources being spread thin. And in the light of this I don't think it's smart lashing out at contributors this way. It's certainly not helping. On the matter of reviewing, I don't think I'm a good choice for CPP. I can write things that compile, sometimes. But I don't really know Qt or cpp. I shall do my best though. |
On the topic of design: I'm not convinced parsing the desktop Exec line is the ideal way to do this. I would prefer if there was some way to let Lipstick handle all of this, i.e. just simulate a user click. One way I have found is
buut this launching an application may just be coincidence, not too sure. The Lipstick Launcher does it by calling |
Taken fom PR sailfishos-chum#266 Authored-By: Jaime Marquínez Ferrándiz <jaime.marquinez.ferrandiz@fastmail.net>
... So something like this: |
Taken fom PR sailfishos-chum#266 Authored-By: Jaime Marquínez Ferrándiz <jaime.marquinez.ferrandiz@fastmail.net>
I tested your version and it seems to behave like mine, so we could use it as it is indeed simpler and hopefully more robust. |
Thank you very much, @nephros for taking a look at this PR, then having an idea for, researching and implementing a simpler solution, and @jaimeMF for the original idea and testing @nephros' variant. Hence I created PR #274: Is that what you @nephros had in mind? Is the other commit (rsp. its changes) in that branch |
Actually IMO this is basically the same: A matter of prioritising. More on this below.
This sure applies to piggz, rinigus an me, too. Still, you did help here and I try to handle PRs and issues properly (just the handling).
LOL! 😉
You always do, from what I have observed. 😄 |
Nice! So, will you add this change to the PR branch, or shall I do it and put a PR onto your branch? |
@Olf0 already opened a PR to the main repo with your changes. If you prefer that I use my PR instead, I can add your changes. |
See PR #274, which applies cleanly in contrast to this one (likely due to changes in the But ultimately I do not care at all which route is taken, I just wanted to make it easy for both of you to look at it (~ review) and give an "O.K." so I can merge. |
The applications use the SailJail sandbox if they are configured to do so. Resolves sailfishos-chum#105
1899839
to
48d2820
Compare
I have rebased on top of main and cherry picked @nephros 's commit. |
Than you very much @jaimeMF: This is really looking good now! Still, I would really appreciate if you provide me with your opinion on my code review and comment: In the light of the recent changes (the ones suggested by nephros, i.e. not to evaluate the desktop-file-entry's |
The |
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 (AFAICT), so let's go!
SailfishOS-Chum GUI application v0.6.7 is available at the I will submit it to the |
Thanks! For me the new version shows a missing translation string for the launch menu entry ("chum-launch"). |
Oh, I missed that there is a new translatable string, hence did not run Maybe this weekend. |
Second thing is, and I'm sorry I didn't catch that earlier: Launched applications show up as not-sailjailed! |
Where do they show up as not-sailjailed? When launching a newly installed application, it asks for permissions. |
i'll have to check in detail then. I'm just using my Sandbox Indicator patch to check, it's possible that that is wrong. |
BTW, the new menu entry "Launch" does not show on SailfishOS 3.2.1. This is not tragic, as it is just not there (i.e. nothing broken is exposed), but mind that SailfishOS:Chum (the repository and all infrastructure, including the GUI app) does support all SailfishOS releases ≥ 3.1.0. I.e., it would be nice to fix that, if it can be fixed easily. |
Hm. Nothing obvious to me why it would do (or not do) that. And I don't have a 3.2 device to look at. Does launching from console say anything useful? |
Oh, ugh, launching from the console does not look good (part 1: git-hoster access):
This is how far the SailfishOS:Chum GUI app runs without any user interaction at the GUI. Do you see these errors accessing GitLab, Forgejo (codeberg.org) and Sailfish-OBS (SailfishOS:Chum repository), too? BTW, there is no mention of GitHub, so I conclude the repositories hosted there are well accessible. |
When I switch to Installed packages, it outputs:
O.K., well plausible so far. Then I scrolled to Bar Code and opened it:
I guess I better switch off the Patchmanager-Patch Return old remorse item animation. Oh, it is not enabled (because it is not applicable on SailfishOS 3.2.1) and it tries to patch Next try, this time I also watched the GUI:
TL;DRNo, nothing in terminal output of the SailfishOS:Chum GUI app started at the command line indicates anything WRT the pulley entry |
Any news WRT this observation, @nephros? Or WRT this one:
|
The SailfishOS:Chum GUI app v0.6.7-2 is available at its GitHub releases page and in the |
Sorry, haven't had time to look into these either (hacking new stuff is so much more rewarding than squishing bugs ;) ). Is it possible the github/lab/codeberg errors are because of older SSL certificates or library versions in the testing OS? |
Unlikely this reason (but there may be others), because I specifically fixed-up these things and everything else works, e.g. HTTPS for websites in Browser. But this exactly is the reason, why I asked you and @jaimeMF to test. |
On SFOS 4.5 I get the same messages about Gitlab and Forgejo, the "Launch" menu entry works fine. |
Pretty sure the forgejo/gitlab messages are unrelated to this change. The forgejo 'not found' message is because we just iterate over the package name in the integrations. Quite possible the gitlab messages are for the same reason, but GL respond with a different code. |
O.K., I filed issue #289 WRT the command line output, i.e. the warnings indicating authentication failures at the Sailfish-OBS, GitLab.com and codeberg.org. Furthermore I filed issue #290 WRT the top pulley menu entry Any news WRT the apparent sandboxing issue? |
The applications use the SailJail sandbox if they are configured to do so.
Resolves #105