Closed jaimeMF closed 1 month ago
@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
@jaimeMF, as stated, I am willing to merge this, because all requests to the original authors to review this (rsp. to finish / provide feedback on their review) have been futile.
But lastly now I have a (maybe silly) question, I would like to have clarified, first.
@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. :disappointed:
i believe it's not not caring, it's about resources being spread thin. Certainly in my case, way too many irons in the forge as they say.
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.
At the least it's fragile should the sailjail parameters change in some way. I also believe this will probably fail in some esoteric cases.
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
busctl --user call org.nemomobile.lipstick / org.sailfishos.fileservice openUrl s file:///usr/share/applications/foo.desktop
buut this launching an application may just be coincidence, not too sure.
The Lipstick Launcher does it by calling launchApplication()
on a member of LauncherModel. Maybe this can be used but I'd have to find out more about it.
On the topic of design: I'm not convinced parsing the desktop Exec line is the ideal way to do this. At the least it's fragile should the sailjail parameters change in some way. I also believe this will probably fail in some esoteric cases.
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
busctl --user call org.nemomobile.lipstick / org.sailfishos.fileservice openUrl s file:///usr/share/applications/foo.desktop
buut this launching sn application may just be coincidence, not too sure.
... So something like this:
https://github.com/nephros/sailfishos-chum-gui/commit/689a26fad9884be4f80941e7e0057252bbe67351
On the topic of design: I'm not convinced parsing the desktop Exec line is the ideal way to do this. At the least it's fragile should the sailjail parameters change in some way. I also believe this will probably fail in some esoteric cases. 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
busctl --user call org.nemomobile.lipstick / org.sailfishos.fileservice openUrl s file:///usr/share/applications/foo.desktop
buut this launching sn application may just be coincidence, not too sure.
... So something like this:
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 applaunch
good to be merged, too?
i believe it's not not caring, it's about resources being spread thin.
Actually IMO this is basically the same: A matter of prioritising. More on this below.
Certainly in my case, way too many irons in the forge as they say.
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).
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.
LOL! :wink: Your code is well designed and well readable even for me, and it does not just compile but also work well in practice, as I can tell from years of experience and collaboration. :smiley:
I shall do my best though.
You always do, from what I have observed. :smile:
On the topic of design: I'm not convinced parsing the desktop Exec line is the ideal way to do this. At the least it's fragile should the sailjail parameters change in some way. I also believe this will probably fail in some esoteric cases. 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
busctl --user call org.nemomobile.lipstick / org.sailfishos.fileservice openUrl s file:///usr/share/applications/foo.desktop
buut this launching sn application may just be coincidence, not too sure.
... So something like this: nephros@689a26f
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.
Nice! So, will you add this change to the PR branch, or shall I do it and put a PR onto your branch?
On the topic of design: I'm not convinced parsing the desktop Exec line is the ideal way to do this. At the least it's fragile should the sailjail parameters change in some way. I also believe this will probably fail in some esoteric cases. 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
busctl --user call org.nemomobile.lipstick / org.sailfishos.fileservice openUrl s file:///usr/share/applications/foo.desktop
buut this launching sn application may just be coincidence, not too sure.
... So something like this: nephros@689a26f
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.
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.
@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 main
branch meanwhile).
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.
I have rebased on top of main and cherry picked @nephros 's commit.
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 exec=
line for starting the app, but to use the launcher), I tend to my second suggestion or simply visible: (pkg.installed && pkg.desktopFile.length > 0)
.
But as noted before, that may be nonsense (as anything I write), then I would like to understand the reason.
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
exec=
line for starting the app, but to use the launcher), I tend to my second suggestion or simplyvisible: (pkg.installed && pkg.desktopFile.length > 0)
. But as noted before, that may be nonsense (as anything I write), then I would like to understand the reason.
The pkg.type == ChumPackage.PackageApplicationDesktop
check seems unnecessary, I have removed it. Thank you.
SailfishOS-Chum GUI application v0.6.7 is available at the sailfishos:chum:testing
repository and GitHub's releases page for testing.
I will submit it to the sailfishos:chum
repository proper in a couple of days, if no negative feedback is provided.
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 lupdate
and updated the translations at Transifex.
Maybe this weekend.
Second thing is, and I'm sorry I didn't catch that earlier:
Launched applications show up as not-sailjailed! They seem to inherit the out-of-jail status of Chum GUI.
Second thing is, and I'm sorry I didn't catch that earlier:
Launched applications show up as not-sailjailed! They seem to inherit the out-of-jail status of Chum GUI.
Where do they show up as not-sailjailed? When launching a newly installed application, it asks for permissions.
Second thing is, and I'm sorry I didn't catch that earlier: Launched applications show up as not-sailjailed! They seem to inherit the out-of-jail status of Chum GUI.
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.
Oh, I missed that there is a new translatable string, hence did not run
lupdate
and updated the translations at Transifex.
Done by commit f54ae3a.
Edit: I also updated all translations at Transifex and merged the resulting PRs #276, #277, #278, #279, #280, #281, #282, #283 and #284.
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):
[nemo@sailfish ~]$ sailfishos-chum-gui
[D] unknown:0 - Using Wayland-EGL
[W] unknown:0 - Failed to refresh Chum repository "Failed to obtain authentication."
[D] unknown:0 - GitLab support added for "gitlab.com"
[D] unknown:0 - Forgejo support added for "codeberg.org"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "flypig/harbour-fosdem23"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/harbour-privoxy"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/nextcloud-client"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/harbour-uptimed"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/openrepos-keychain"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - Forgejo: Failed to fetch repository data for Forgejo "sailfish-moreutils"
[W] unknown:0 - Forgejo: Error: "Error transferring https://codeberg.org/api/v1/repos/sailfish-moreutils - server replied: Not Found"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "project/nephros/coderus-screencast"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/harbour-privoxy"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/openrepos-nethack"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/nextcloud-client"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/harbour-privoxy"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/qtkeychain"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/harbour-fbjail"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/harbour-mlocate"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "dracks/quicklaunchslx"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/youtube-dl/-/tree/sfos-open-helper"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/harbour-privoxy"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "nephros/openrepos-nethack"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - Forgejo: Failed to fetch repository data for Forgejo "sailfish-moreutils"
[W] unknown:0 - Forgejo: Error: "Error transferring https://codeberg.org/api/v1/repos/sailfish-moreutils - server replied: Not Found"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "sailfishos-chum/smem"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
[W] unknown:0 - GitLab: Failed to fetch repository data for GitLab "dracks/flight-tracker-slx"
[W] unknown:0 - GitLab: Error: "Host requires authentication"
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:
[W] unknown:17 - file:///usr/share/sailfishos-chum-gui/qml/components/PackagesListItem.qml:17:5: QML Image: Error transferring https://github.com/ilpianista/harbour-Base64/-/raw/master/icons/harbour-base64.svg - server replied: Not Found
[W] unknown:17 - file:///usr/share/sailfishos-chum-gui/qml/components/PackagesListItem.qml:17:5: QML Image: Error transferring https://github.com/black-sheep-dev/harbour-kasa/raw/master/icons/harbour-audiothek.svg - server replied: Not Found
O.K., well plausible so far.
Then I scrolled to Bar Code and opened it:
[W] unknown:157 - file:///usr/lib/qt5/qml/Sailfish/Silica/private/RemorseBase.qml:157:5: QML Column: Cannot anchor to an item that isn't a parent or sibling.
[W] unknown:157 - file:///usr/lib/qt5/qml/Sailfish/Silica/private/RemorseBase.qml:157:5: QML Column: Cannot anchor to an item that isn't a parent or sibling.
[W] unknown:157 - file:///usr/lib/qt5/qml/Sailfish/Silica/private/RemorseBase.qml:157:5: QML Column: Cannot anchor to an item that isn't a parent or sibling.
[W] unknown:157 - file:///usr/lib/qt5/qml/Sailfish/Silica/private/RemorseBase.qml:157:5: QML Column: Cannot anchor to an item that isn't a parent or sibling.
[W] unknown:157 - file:///usr/lib/qt5/qml/Sailfish/Silica/private/RemorseBase.qml:157:5: QML Column: Cannot anchor to an item that isn't a parent or sibling.
[D] unknown:0 - bool PatchManagerTranslator::installTranslator(const QString&) "return-old-remorse-animation" QLocale(C, Default, Default)
[D] unknown:0 - bool PatchManagerTranslator::installTranslator(const QString&) fail
[D] unknown:0 - bool PatchManagerTranslator::installTranslator(const QString&) "return-old-remorse-animation" QLocale(C, Default, Default)
[D] unknown:0 - bool PatchManagerTranslator::installTranslator(const QString&) fail
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 /usr/lib/qt5/qml/Sailfish/Silica/RemorseItem.qml:204&236
(i.e. not /usr/lib/qt5/qml/Sailfish/Silica/private/RemorseBase.qml:157
)!?! Ah, but Return old remorse popup animation patches /usr/lib/qt5/qml/Sailfish/Silica/private/RemorseBase.qml
: Disabled and deactivated it.
[W] unknown:0 - Failed to refresh Chum repository "Failed to obtain authentication."
being emitted again in the terminal window, the GUI output suggests that refreshing the SailfishOS:Chum repository works fine.QML image
errors are displayed as expected, but neither when entering Bar Code or Bugger any additional command line output appears. Well, but there still is no pulley menu entry Launch (chum-launch
), only Remove, plus Source Code, Issue tracker and Discussion forum for Bugger.No, nothing in terminal output of the SailfishOS:Chum GUI app started at the command line indicates anything WRT the pulley entry chum-launch
, but this entry (with its text Launch) is not shown on SailfishOS 3.2.1.
Second thing is, and I'm sorry I didn't catch that earlier: Launched applications show up as not-sailjailed! They seem to inherit the out-of-jail status of Chum GUI.
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.
Any news WRT this observation, @nephros?
Or WRT this one:
Do you see these errors accessing GitLab, Forgejo (codeberg.org) and Sailfish-OBS (SailfishOS:Chum repository), too?
The SailfishOS:Chum GUI app v0.6.7-2 is available at its GitHub releases page and in the sailfishos:chum:testing
repository (and is on its way to sailfishos:chum
proper): Please check that it works fine, especially WRT the aforementioned two points.
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?
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.
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 chum-launch
not being visible on older SailfishOS releases.
Any news WRT the apparent sandboxing issue? I.e. was this a false positive or is it a real issue?
The applications use the SailJail sandbox if they are configured to do so.
Resolves https://github.com/sailfishos-chum/sailfishos-chum-gui/issues/105