Port kcm energy info to kirigami 2, fix colors issues
ClosedPublic

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

Details

Diff Detail

Repository
R102 KInfoCenter
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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

443

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
443

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
28

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

48–49

there is trailing space in this row

441

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.Jun 1 2019, 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.

What I've got right now is:

The most notable UI change is probably the fact that I added an inline message to waste less space.

So we still need to get rid of the top gap, have the window actually have correct minimum width, and look into that crash bug.

Hey that looks pretty good!

To fix the top gap, I wonder if it would suffice to add anchors.fill: parent or some other positioning mechanism to the top-most ColumnLayout inside the Kirigami.ScrollablePage.

That's what I thought, but it doesn't work :/

What we know is the gap was there before. Since we've now got a Kirigami.ScrollablePage, we can scroll all the content up by holding the mouse. And we can also just make the ScrollablePage the top item. Regardless of that, it shows a gap at the top. I don't see where in the QML code this is coming from.

BTW I think the errors were due to using a non-existent QQC2 import (2.12), so we're good on that front.

meven updated this revision to Diff 60193.Jun 21 2019, 9:21 AM

Rebase code, apply Filip's patch, fix right padding of graph

meven updated this revision to Diff 60205.EditedJun 21 2019, 10:12 AM

Use KCM.SimpleKCM as root item, fix padding issues, increment kcm version, clean up old now unnecssary workaround

meven edited the test plan for this revision. (Show Details)Jun 21 2019, 10:13 AM
meven updated this revision to Diff 60206.Jun 21 2019, 10:15 AM

Add some padding around the inline message when history is not available

filipf accepted this revision.Jun 21 2019, 10:16 AM
This revision is now accepted and ready to land.Jun 21 2019, 10:16 AM
meven retitled this revision from [WIP] Port kcm energy info to kirigami 2, fix colors issues to Port kcm energy info to kirigami 2, fix colors issues.Jun 21 2019, 10:48 AM
meven edited the summary of this revision. (Show Details)
ngraham accepted this revision.Jun 21 2019, 10:54 AM

Re-add explicit "QQC2" name in the import, per T10862, then shipit!

meven updated this revision to Diff 60231.Jun 21 2019, 1:19 PM

import QtQuick.Controls 2.5 as QQC2

This revision was automatically updated to reflect the committed changes.