Bug 443455 - Disabled Flatpak repos can't be successfully removed
Summary: Disabled Flatpak repos can't be successfully removed
Status: RESOLVED DUPLICATE of bug 426565
Alias: None
Product: Discover
Classification: Applications
Component: Flatpak Backend (show other bugs)
Version: 5.23.0
Platform: Other Linux
: NOR major
Target Milestone: ---
Assignee: Dan Leinir Turthra Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-08 00:09 UTC by Adam Williamson
Modified: 2021-10-18 21:54 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.23.1
Sentry Crash Report:


Attachments
Checkboxes look odd (84.78 KB, image/png)
2021-10-09 00:53 UTC, Aleix Pol
Details
screenshot from Fedora 35 KDE (1.21 MB, image/png)
2021-10-09 06:41 UTC, Adam Williamson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Williamson 2021-10-08 00:09:09 UTC
This is an upstream report of Fedora downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=2011291 .

As described there, configured Flatpak remotes are always shown as enabled but greyed out - whether they are actually disabled or not. This is misleading.

The 'remove' button is available, and works for enabled repos, but for disabled repos it fails with a "Can't fetch summary from disabled remote" error.

This bug is still present in 5.23.0, I just tested that today.
Comment 1 Nate Graham 2021-10-08 16:39:11 UTC
Hmm, cannot fully reproduce in Fedora 34 with all KDE stuff built from source. I can remove Flatpak remotes just fine.

However I cannot disable any remotes. For me the checkboxes are visually enabled, but clicking on them does nothing.

This seems likely to be related. Probably for you, the checkbox visually reflects its immutable status, and then removing the remote is disabled too.

The real question is why the checkboxes are trying to be immutable in the first place.
Comment 2 Adam Williamson 2021-10-08 17:23:06 UTC
I'm testing on F35, as is Kamil. Note that removing an *enabled* remote works fine, it's only removing *disabled* remotes that doesn't work.
Comment 3 Aleix Pol 2021-10-09 00:53:41 UTC
Created attachment 142270 [details]
Checkboxes look odd

Maybe OP is referring to how the check-boxes look enabled while they're not?
Comment 4 Adam Williamson 2021-10-09 06:37:27 UTC
That's not how it looks here. I'll attach a screenshot...
Comment 5 Adam Williamson 2021-10-09 06:41:51 UTC
Created attachment 142273 [details]
screenshot from Fedora 35 KDE

note how the flatpak remote 'checkboxes' are grey, while the 'Firmware Updates' ones are blue. The 'Firmware Updates' ones change when you mouse over them, indicating they can be changed, and indeed they can be (you can enable and disable them). The Flatpak ones do not change when you mouse over them, and nothing happens when you click them.
Comment 6 Adam Williamson 2021-10-14 00:38:11 UTC
So I looked into this a bit today. It appears to be basically just flat-out missing functionality; if you compare the similar FlatpakSourcesBackend.cpp, FwupdSourcesBackend.cpp and PackageKitSourcesBackend.cpp , you can see that the fwupd and pk backends have both code to set the initial check state of the QStandardItems that are used for source rows here based on the source state and also code to enable or disable a source when the box is clicked, but the flatpak backend has neither of those.

I am no kind of C++ coder at all. I could successfully make the default state of the box for flatpak sources match the actual state of the source with a simple patch:

    --- a/libdiscover/backends/FlatpakBackend/FlatpakSourcesBackend.cpp
    +++ b/libdiscover/backends/FlatpakBackend/FlatpakSourcesBackend.cpp
    @@ -338,6 +361,8 @@ void FlatpakSourcesBackend::addRemote(FlatpakRemote *remote, FlatpakInstallation
         it->setData(remoteUrl.isLocalFile() ? remoteUrl.toLocalFile() : remoteUrl.host(), Qt::ToolTipRole);
         it->setData(remoteUrl, Qt::StatusTipRole);
         it->setData(id, IdRole);
    +    it->setCheckable(true);
    +    it->setCheckState(flatpak_remote_get_disabled(remote) ? Qt::Unchecked : Qt::Checked);
     #if FLATPAK_CHECK_VERSION(1, 4, 0)
         it->setData(QString::fromUtf8(flatpak_remote_get_icon(remote)), IconUrlRole);
     #endif

...but this also makes the boxes active, you can click on them, but doing so of course doesn't actually change the source's real state as clicking on them isn't hooked up to anything. (Note that when you first click on the box the state won't appear to change, but if you leave the page and come back it will be in the other state; this is https://bugs.kde.org/show_bug.cgi?id=406295 ).

I did try and implement changing the state in response to a click too, but didn't make that compile yet, let alone work, if nobody else wants to work on it, I'll keep trying.
Comment 7 Nate Graham 2021-10-18 15:40:53 UTC
Git commit 41cef10e065d09a6bbb6bdd4a468fdf6a916ec62 by Nate Graham, on behalf of Aleix Pol.
Committed on 18/10/2021 at 15:40.
Pushed by ngraham into branch 'master'.

flatpak: Set the real value of the remotes

Default was set true, so we were showing something wrong in case it was
disabled.

M  +1    -0    libdiscover/backends/FlatpakBackend/FlatpakSourcesBackend.cpp

https://invent.kde.org/plasma/discover/commit/41cef10e065d09a6bbb6bdd4a468fdf6a916ec62
Comment 8 Nate Graham 2021-10-18 15:44:01 UTC
Git commit 2836f6373acbfbca666f08bbe725e01a8b0a7ca2 by Nate Graham, on behalf of Aleix Pol.
Committed on 18/10/2021 at 15:43.
Pushed by ngraham into branch 'Plasma/5.23'.

flatpak: Set the real value of the remotes

Default was set true, so we were showing something wrong in case it was
disabled.


(cherry picked from commit 41cef10e065d09a6bbb6bdd4a468fdf6a916ec62)

M  +1    -0    libdiscover/backends/FlatpakBackend/FlatpakSourcesBackend.cpp

https://invent.kde.org/plasma/discover/commit/2836f6373acbfbca666f08bbe725e01a8b0a7ca2
Comment 9 Adam Williamson 2021-10-18 16:31:32 UTC
So the fix fixed the "always shown as enabled but greyed out" part, but not the "can't delete disabled remotes" part. That is still true - you can successfully delete an enabled remote, but trying to delete a disabled remote fails.
Comment 10 Nate Graham 2021-10-18 16:35:17 UTC
Yeah I guess that part will be done in https://invent.kde.org/plasma/discover/-/merge_requests/187.
Comment 11 Adam Williamson 2021-10-18 16:50:01 UTC
I don't think so. It's just a bug in
Comment 12 Adam Williamson 2021-10-18 16:53:23 UTC
I'm not sure that would change it at all, actually. This seems to be a bug in FlatpakSourcesBackend::removeSource(), but I'm not totally sure what. oddly, the error we get on the console - "qml: Failed to remove the source XXX" - doesn't quite match the log line you'd think we'd be htting in the backend - "Failed to remove %1 remote repository: %2".
Comment 13 Adam Williamson 2021-10-18 16:55:53 UTC
oh, that error is from the qml, indicating backend.removeSource returned false:

                        if (!backend.removeSource(sourceId)) {
                            console.warn("Failed to remove the source", model.display)
                        }

but I'm still not sure why. I'll look at it a bit harder.
Comment 14 Adam Williamson 2021-10-18 16:59:47 UTC
ohhh, wait, I see the problem. We're reusing the `error` pointer from the `flatpak_installation_list_remote_refs_sync()` call for the `flatpak_installation_remove_remote()`, but that's not safe if the `remote_refs_sync()` call actually resulted in an error. We need to re-initialize (is that the word? not a C guy) the error pointer between the calls.
Comment 15 Adam Williamson 2021-10-18 19:55:49 UTC
https://invent.kde.org/plasma/discover/-/merge_requests/191 should resolve this.
Comment 16 Aleix Pol 2021-10-18 21:54:12 UTC

*** This bug has been marked as a duplicate of bug 426565 ***