Bug 406295 - State of checkboxes is not immediately updated after I enable/disable a firmwares source in "Settings" page
Summary: State of checkboxes is not immediately updated after I enable/disable a firmw...
Status: RESOLVED FIXED
Alias: None
Product: Discover
Classification: Applications
Component: fwupd Backend (show other bugs)
Version: 5.22.4
Platform: Arch Linux Linux
: NOR normal
Target Milestone: ---
Assignee: Abhijeet Sharma
URL:
Keywords:
: 428386 443456 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-04-07 11:58 UTC by Patrick Silva
Modified: 2023-02-02 22:03 UTC (History)
7 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Silva 2019-04-07 11:58:48 UTC
STEPS TO REPRODUCE
1. open discover
2. click "Sources" in the side bar
3. check/uncheck some "Firmware updates" source (discover asks for your password)
4. type your password and press enter

OBSERVED RESULT
status of the checked/uncheched checkbox remains the same.
Restart Discover and now the checkbox status is correct.

EXPECTED RESULT
checkbox status is updated immediately after you type your password
and press enter.


SOFTWARE/OS VERSIONS
Operating System: Arch Linux 
KDE Plasma Version: 5.15.4
KDE Frameworks Version: 5.56.0
Qt Version: 5.12.2
Comment 1 Patrick Silva 2020-10-28 23:42:28 UTC
*** Bug 428386 has been marked as a duplicate of this bug. ***
Comment 2 Patrick Silva 2021-10-08 03:53:38 UTC
*** Bug 443456 has been marked as a duplicate of this bug. ***
Comment 3 Adam Williamson 2021-10-14 23:57:46 UTC
So I've been beating my head against this all day and not getting *that* far, but I can at least say the problem is caused by our handling of clicks on the checkbox in discover/qml/SourcesPage.qml:

                    onClicked: {
                        sourcesView.model.setData(idx, checkState, Qt.CheckStateRole)
                        checked = Qt.binding(function() { return modelChecked !== Qt.Unchecked; })
                    }

this is the bit that makes clicking the checkbox actually do something. What it's *supposed* to do is tell the source backend to actually enable or disable the source and then update its record as to whether the checkbox should be checked or not - this all happens when we call sourcesView.model.setData - and then reset the 'checked' state to whatever the backend now thinks it should be (so the checkbox should change state if it did, or stay the same if it didn't).

If you take the onClicked block out entirely, the checkboxes will respond perfectly to being clicked whichever state they're in. Of course, they won't *do* anything. If that block is there, clicking them does what it is supposed to so long as the backend supports it, but the checkbox gets sort of 'stuck' as described here. Note that a *second* click while it's stuck does not change the state again.

Interestingly, neither taking the `checked = Qt.binding(function() { return modelChecked !== Qt.Unchecked; })` line out nor exactly inverting its logic (to === instead of !==) changes the observed behaviour *at all*.

The bug is not apparent on flatpak sources because of https://bugs.kde.org/show_bug.cgi?id=443455 (the flatpak source backend is just flat out missing the code to enable or disable sources), and it's not apparent on PackageKit sources because the way that backend works is that it entirely rebuilds the source list any time the repo configuration changes at all, which sort of hides the bug.
Comment 4 Adam Williamson 2021-10-15 00:03:52 UTC
Note I think this code, which was essentially identical at the time, must have been working back in late 2018, or else it's hard to see how this bug could have been reported and fixed:

https://bugs.kde.org/show_bug.cgi?id=401663

so it almost seems like something in Qt or even Kirigami may have broken this, somehow, between then and when this bug was reported in April 2019?
Comment 5 Adam Williamson 2021-10-15 00:06:41 UTC
oh, sorry, one more note: if you apply my partial fix for https://bugs.kde.org/show_bug.cgi?id=443455 so that the checkboxes for flatpak sources are also clickable, you'll find that they behave exactly the same as fwupd ones, *even though setData is not overridden for the fwupd backend at all*. So the bug reproduces with *just* the qml change from 'upstream' behaviour - it seems we don't need to be concerned about it being affected by anything in the flatpak backend's setData implementation.
Comment 6 Adam Williamson 2021-10-15 00:12:05 UTC
huh, another finding which kinda surprised me: if you take out the call to `sourcesView.model.setData` so that the block just looks like this:

                    onClicked: {
                        checked = Qt.binding(function() { return modelChecked !== Qt.Unchecked; })
                    }

you get the same behaviour as if you remove the block entirely. The checkboxes change state correctly every time you click them, but do nothing. That's not what I was expecting, actually - I was expecting them to do nothing and have buggy behaviour (probably stay in the same state no matter how often you clicked them). Not sure of the significance of that, though.
Comment 7 Bug Janitor Service 2021-10-17 22:46:48 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/qqc2-desktop-style/-/merge_requests/102
Comment 8 Bug Janitor Service 2021-10-18 01:34:05 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/discover/-/merge_requests/186
Comment 9 Nate Graham 2021-10-18 15:40:44 UTC
Git commit 4c7c40360ecc639e140629e4adf55b4b014e6460 by Nate Graham, on behalf of Aleix Pol.
Committed on 18/10/2021 at 15:40.
Pushed by ngraham into branch 'master'.

SourcesPage: Address how we show the checked state

Do not use data() as it won't refresh as the model changes, we never
actually set the checked property anyway. Just call set and let the
checked property be updated by the model.

Alternatively we had 2 actors updating the value and it didn't work
well.

M  +1    -1    discover/qml/SourcesPage.qml
M  +1    -0    libdiscover/resources/SourcesModel.cpp

https://invent.kde.org/plasma/discover/commit/4c7c40360ecc639e140629e4adf55b4b014e6460
Comment 10 Nate Graham 2021-10-18 15:43:16 UTC
Git commit e9f3a33feb24093f5f40e565a16b20e81334b0ee by Nate Graham, on behalf of Aleix Pol.
Committed on 18/10/2021 at 15:43.
Pushed by ngraham into branch 'Plasma/5.23'.

SourcesPage: Address how we show the checked state

Do not use data() as it won't refresh as the model changes, we never
actually set the checked property anyway. Just call set and let the
checked property be updated by the model.

Alternatively we had 2 actors updating the value and it didn't work
well.


(cherry picked from commit 4c7c40360ecc639e140629e4adf55b4b014e6460)

M  +1    -1    discover/qml/SourcesPage.qml
M  +1    -0    libdiscover/resources/SourcesModel.cpp

https://invent.kde.org/plasma/discover/commit/e9f3a33feb24093f5f40e565a16b20e81334b0ee
Comment 11 Aleix Pol 2021-10-19 14:27:39 UTC
Git commit 939176f4a556ed38570db6e6735ba44a1015ea67 by Aleix Pol.
Committed on 17/10/2021 at 22:57.
Pushed by ngraham into branch 'master'.

Do not set the palette for every component

This makes it properly display disabled checkboxes as grey.

M  +0    -1    org.kde.desktop/BusyIndicator.qml
M  +0    -1    org.kde.desktop/Button.qml
M  +0    -1    org.kde.desktop/CheckBox.qml
M  +0    -1    org.kde.desktop/CheckDelegate.qml
M  +0    -1    org.kde.desktop/ComboBox.qml
M  +0    -1    org.kde.desktop/Container.qml
M  +0    -1    org.kde.desktop/Control.qml
M  +0    -1    org.kde.desktop/DelayButton.qml
M  +0    -1    org.kde.desktop/Dial.qml
M  +0    -1    org.kde.desktop/Dialog.qml
M  +0    -1    org.kde.desktop/Drawer.qml
M  +0    -1    org.kde.desktop/Frame.qml
M  +0    -1    org.kde.desktop/GroupBox.qml
M  +0    -1    org.kde.desktop/ItemDelegate.qml
M  +0    -1    org.kde.desktop/Menu.qml
M  +0    -1    org.kde.desktop/MenuBar.qml
M  +0    -1    org.kde.desktop/MenuBarItem.qml
M  +0    -1    org.kde.desktop/MenuItem.qml
M  +0    -1    org.kde.desktop/Popup.qml
M  +0    -1    org.kde.desktop/ProgressBar.qml
M  +0    -1    org.kde.desktop/RadioButton.qml
M  +0    -1    org.kde.desktop/RadioDelegate.qml
M  +0    -1    org.kde.desktop/RangeSlider.qml
M  +0    -1    org.kde.desktop/ScrollBar.qml
M  +0    -1    org.kde.desktop/ScrollView.qml
M  +0    -1    org.kde.desktop/Slider.qml
M  +0    -1    org.kde.desktop/SpinBox.qml
M  +0    -1    org.kde.desktop/Switch.qml
M  +0    -1    org.kde.desktop/SwitchDelegate.qml
M  +0    -1    org.kde.desktop/TabBar.qml
M  +0    -1    org.kde.desktop/TabButton.qml
M  +0    -1    org.kde.desktop/TextArea.qml
M  +0    -1    org.kde.desktop/TextField.qml
M  +0    -2    org.kde.desktop/ToolBar.qml
M  +0    -1    org.kde.desktop/ToolButton.qml
M  +0    -1    org.kde.desktop/ToolTip.qml
M  +4    -0    tests/CheckBox.qml

https://invent.kde.org/frameworks/qqc2-desktop-style/commit/939176f4a556ed38570db6e6735ba44a1015ea67
Comment 12 Marco Martin 2023-02-01 12:43:07 UTC
This causes https://bugs.kde.org/show_bug.cgi?id=465054
Comment 13 Nate Graham 2023-02-02 22:03:48 UTC
Wrong bug? This is for an issue in Discover and Bug 465054 is about an issue in Kirigami.