Differential D17796

Show accurate checked state for system monitor configuration pages
ClosedPublic
Actions

Authored by pavelmos on Dec 25 2018, 12:43 PM.

Details

Reviewers
jriddell
ngraham
broulik
davidedmundson
Group Reviewers
Plasma
Commits
R120:83045375ec2b: Show accurate checked state for system monitor configuration pages
Summary

Fixed general settings of the widgets "Memory Status", "Hard Disk Space Usage", "Network Monitor", "Hard Disk Monitor".

  • correctly set checked/unchecked state on load.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pavelmos created this revision.Dec 25 2018, 12:43 PM
Restricted Application edited projects, added Plasma; removed Plasma (Plasma 5.14), Plasma: Workspaces. · View Herald TranscriptDec 25 2018, 12:43 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pavelmos requested review of this revision.Dec 25 2018, 12:43 PM
pavelmos added a reviewer: jriddell.
ngraham requested changes to this revision.Dec 26 2018, 6:05 PM
ngraham added a subscriber: ngraham.

As far as I can tell, this doesn't work: the listed widgets still have all their configuration pages' checkboxes unchecked after checking some of them, closing the window, and re-opening it. Am I missing something?

Also please change the title to something more descriptive, such as "Show accurate checked state for system monitor configuration pages"

This revision now requires changes to proceed.Dec 26 2018, 6:05 PM
pavelmos updated this revision to Diff 48278.Dec 28 2018, 9:42 AM
ngraham accepted this revision.Dec 28 2018, 3:04 PM

There we go, it works now! Code change looks sane to me.

Please change the title to something more descriptive, such as "Show accurate checked state for system monitor configuration pages". And then we'll wait for a review from a Plasma developer.

This revision is now accepted and ready to land.Dec 28 2018, 3:04 PM
davidedmundson added a subscriber: davidedmundson.Dec 28 2018, 6:55 PM

The call on line 141 looks superfluous - if there's any change to checked we modify cfg_sources which should trigger the reload.

Can you confirm?

pavelmos retitled this revision from Plasma Workspace. Fixed general settings of the widgets. to Show accurate checked state for system monitor configuration pages.Dec 29 2018, 8:58 AM
pavelmos added a comment.Dec 29 2018, 9:04 AM
In D17796#382974, @ngraham wrote:

There we go, it works now! Code change looks sane to me.

Please change the title to something more descriptive, such as "Show accurate checked state for system monitor configuration pages". And then we'll wait for a review from a Plasma developer.

Okay. Thank you!

pavelmos added a comment.Feb 8 2019, 9:02 AM

I have no commit access.

broulik added a subscriber: broulik.Feb 8 2019, 9:04 AM

The call on line 141 looks superfluous - if there's any change to checked we modify cfg_sources which should trigger the reload.

QML cannot detect changes within a JS structure, if you push() into an Array or modify members of an Object it doesn't see the change and the automatism for onCfg_...Changed doesn't trigger

pavelmos added a comment.Apr 3 2019, 12:34 PM

It seems to me that everything is fine.
Are there any other comments or questions?

ngraham added a comment.Apr 3 2019, 1:54 PM

We need a Plasma review first. Calling all Plasma devs! :)

pavelmos added a reviewer: broulik.Apr 9 2019, 8:07 AM
pavelmos set the repository for this revision to R120 Plasma Workspace.
davidedmundson accepted this revision.Apr 9 2019, 1:45 PM
Closed by commit R120:83045375ec2b: Show accurate checked state for system monitor configuration pages (authored by pavelmos, committed by ngraham). · Explain WhyApr 9 2019, 2:46 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 55825

View Options

applets/systemmonitor/common/contents/ui/ConfigGeneral.qml

Loading...