[WIP] Port kcm energy info to kirigami 2, fix colors issues
Needs ReviewPublic

Authored by meven on Apr 16 2019, 9:22 AM.

Details

Summary

Just a beginning.

It still miss alignment between repeating sections.

I haven't found a way to align Kirigami.FormLayout together in a repeater.
There is still room for improvement.

Relates to D20533

Test Plan

Before:

After:

Diff Detail

Repository
R102 KInfoCenter
Branch
arcpatch-D20598
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10934
Build 10952: arc lint + arc unit
meven created this revision.Apr 16 2019, 9:22 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 16 2019, 9:22 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Apr 16 2019, 9:22 AM

way to align Kirigami.FormLayout together in a repeater.

Check out the twinFormLayouts property where you can have it manage multiple different FormLayout instances as a single layout

Modules/energy/package/contents/ui/main.qml
19–20

and also getting rid of QQC1

29

A goal is to get rid of these plasma imports, too

447

Shouldn't be neccessary as it's QQC2.Label

meven edited the summary of this revision. (Show Details)Apr 16 2019, 9:26 AM
meven edited the test plan for this revision. (Show Details)
meven added reviewers: ngraham, broulik.

I haven't found a way to align Kirigami.FormLayout together in a repeater.

Maybe you could use twinFormLayouts like in D19873 and D19932?

meven added inline comments.Apr 16 2019, 9:31 AM
Modules/energy/package/contents/ui/main.qml
447

For breeze dark this makes the label, dark gray on a dark background. See before screenshot.

Could it be a breeze dark bug ?

filipf added a subscriber: filipf.EditedApr 16 2019, 11:27 AM

Looks like you'll need to use twinFormLayouts. One unfortunate thing is you can't go "ha, I'll have all the layouts adjust to this specific one (with the longest label)". You'll need to have them all interlinked with one another because label length can change based on translation.

I can have a better look at it later in the day I hope.

filipf requested changes to this revision.EditedApr 16 2019, 1:53 PM

Here's what works:

Repeater {
    id: helloThere // I'll leave the naming up to you :)
    model: root.details

    Kirigami.FormLayout {
        twinFormLayouts: helloThere.itemAt(0)

EDIT: seems like it's not lining up the "Manufacturer" form heh.

Modules/energy/package/contents/ui/main.qml
29

Now that you've imported Kirigami, you can use Kirigami.Units and drop the PlasmaCore import

48

there is trailing space in this row

433

trailing space her as well, as well as in the preceding row

This revision now requires changes to proceed.Apr 16 2019, 1:53 PM
meven updated this revision to Diff 56369.Apr 16 2019, 2:25 PM

Port to QtQuickControls 2 and proper FormLayout

meven edited the test plan for this revision. (Show Details)Apr 16 2019, 2:27 PM
meven added a comment.Apr 16 2019, 2:30 PM

New State:

QtControls port is not done.
It is just a step.

meven marked 6 inline comments as done.Apr 16 2019, 4:05 PM
meven marked an inline comment as done.Apr 16 2019, 4:17 PM
meven updated this revision to Diff 56376.Apr 16 2019, 4:51 PM

Fix FormLayout alignment

meven added a comment.Apr 16 2019, 4:52 PM

After last patch :

meven edited the test plan for this revision. (Show Details)Apr 16 2019, 4:52 PM

This doesn't actually work for me. Upon opening, the following is printed to the console:

org.kde.kcoreaddons: Error loading plugin "kcm_energyinfo" "The shared library was not found." 
Plugin search paths are ("/usr/lib/x86_64-linux-gnu/qt5/plugins", "/usr/bin") 
The environment variable QT_PLUGIN_PATH might be not correctly set
<Unknown File>:3:89: Unable to assign [undefined] to double
QQmlExpression: Expression file:///usr/share/kpackage/kcms/kcm_energyinfo/contents/ui/main.qml:436:33 depends on non-NOTIFYable properties:
    Solid::Battery::serial

Works for me, nice work with aligning the layouts.

I'm a bit conflicted, but I think visually the issue is how these buttons look like with the port, as well as their icons (but that was a problem before as well):

Before they had a useless frame, but the buttons had a border on all 4 sides:

Losing the bottom maybe makes sense, but feels disconnected here IMO.

meven added a comment.Apr 25 2019, 8:20 PM

Works for me, nice work with aligning the layouts.

I'm a bit conflicted, but I think visually the issue is how these buttons look like with the port, as well as their icons (but that was a problem before as well):

Before they had a useless frame, but the buttons had a border on all 4 sides:

Losing the bottom maybe makes sense, but feels disconnected here IMO.

I am using the dark theme and those buttons are pretty much

Works for me, nice work with aligning the layouts.

I'm a bit conflicted, but I think visually the issue is how these buttons look like with the port, as well as their icons (but that was a problem before as well):

Before they had a useless frame, but the buttons had a border on all 4 sides:

Losing the bottom maybe makes sense, but feels disconnected here IMO.

I don't see those buttons, I guess you need a remote mouse or something similar to have them.

Modules/energy/package/contents/ui/main.qml
21

I might be too aggressive here, what would be the best version to set here ?

ngraham added inline comments.Apr 25 2019, 8:25 PM
Modules/energy/package/contents/ui/main.qml
21

2.5 is always a safe bet.

I don't see those buttons, I guess you need a remote mouse or something similar to have them.

Yeah you would need 2 devices I guess. I have my laptop's battery and the wireless mouse's battery listed.

This doesn't actually work for me. Upon opening, the following is printed to the console:

org.kde.kcoreaddons: Error loading plugin "kcm_energyinfo" "The shared library was not found." 
Plugin search paths are ("/usr/lib/x86_64-linux-gnu/qt5/plugins", "/usr/bin") 
The environment variable QT_PLUGIN_PATH might be not correctly set
<Unknown File>:3:89: Unable to assign [undefined] to double
QQmlExpression: Expression file:///usr/share/kpackage/kcms/kcm_energyinfo/contents/ui/main.qml:436:33 depends on non-NOTIFYable properties:
    Solid::Battery::serial

I am now getting the same error as well, kcmshell5 is eating up one cpu core in the background.

This doesn't actually work for me. Upon opening, the following is printed to the console:

org.kde.kcoreaddons: Error loading plugin "kcm_energyinfo" "The shared library was not found." 
Plugin search paths are ("/usr/lib/x86_64-linux-gnu/qt5/plugins", "/usr/bin") 
The environment variable QT_PLUGIN_PATH might be not correctly set
<Unknown File>:3:89: Unable to assign [undefined] to double
QQmlExpression: Expression file:///usr/share/kpackage/kcms/kcm_energyinfo/contents/ui/main.qml:436:33 depends on non-NOTIFYable properties:
    Solid::Battery::serial

I am now getting the same error as well, kcmshell5 is eating up one cpu core in the background.

I am not focused on this anymore and might abandon this review as I really don't enjoy Qml hacking and lack of good tooling at least in my experience.

filipf added a comment.Sat, Jun 1, 9:19 PM

This doesn't actually work for me. Upon opening, the following is printed to the console:

org.kde.kcoreaddons: Error loading plugin "kcm_energyinfo" "The shared library was not found." 
Plugin search paths are ("/usr/lib/x86_64-linux-gnu/qt5/plugins", "/usr/bin") 
The environment variable QT_PLUGIN_PATH might be not correctly set
<Unknown File>:3:89: Unable to assign [undefined] to double
QQmlExpression: Expression file:///usr/share/kpackage/kcms/kcm_energyinfo/contents/ui/main.qml:436:33 depends on non-NOTIFYable properties:
    Solid::Battery::serial

I am now getting the same error as well, kcmshell5 is eating up one cpu core in the background.

I am not focused on this anymore and might abandon this review as I really don't enjoy Qml hacking and lack of good tooling at least in my experience.

If you still have some reserves of energy left, I'd really be appreciated! Someone seems to have fixed the icons so this patch now looks pretty good in 5.15:

It's a massive improvement over the current state. The only thing to would be to figure out what's causing the error with 5.16 and this would be good to go.