[Applets/Power Manager] Update layout based on T10470
AbandonedPublic

Authored by gvgeo on Jan 21 2020, 1:14 PM.

Details

Summary

Decrease icons size and place in row with their labels.
Main area can scroll now (only batteries could scroll).
For 'Enable power management' checkbox, fix tooltip and switch
area (tooltip was only on text, and switch was on full width).
Add a separation line under PM checkbox.
Don't show suppression messages if PM is disabled.
Remove battery details tooltip (had no extra info).
Align battery details to the right, and use opacity for label.

Test Plan

Diff Detail

Repository
R120 Plasma Workspace
Branch
b3 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21651
Build 21669: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
gvgeo added a comment.EditedJan 21 2020, 2:01 PM

I will not touch Tooltip in BatteryItem.qml, has a bug with Grid and uses Flow instead. That made very difficult to fix the tooltip and details.
R120:d12521b149fa7766fbbeb9957bde14b4a6278928

gvgeo updated this revision to Diff 74017.Jan 21 2020, 2:08 PM

Remove translating of new lines

gvgeo marked an inline comment as done.EditedJan 21 2020, 2:20 PM

I believe scroolbars should look good, but didn't test the new scrollbars. I use stable builds, where it touches the scrollbar.


Can someone, confirm if it looks okay with the new scrollbars changes?

gvgeo edited the summary of this revision. (Show Details)Jan 21 2020, 2:26 PM
gvgeo planned changes to this revision.Jan 21 2020, 4:47 PM
gvgeo updated this revision to Diff 74044.Jan 21 2020, 7:34 PM

Fix Layout issues. No changes of the apperance.

gvgeo updated this revision to Diff 74045.Jan 21 2020, 7:45 PM

Correct previous wrong update

gvgeo marked an inline comment as done.Jan 21 2020, 8:06 PM
gvgeo updated this revision to Diff 74069.Jan 22 2020, 7:19 AM

Revert 2 lines (unintentional changes)

Nice!

Couple of UI comments:

  • The icons are now blurry, presumably because the plasma theme doesn't supply versions for the size you're using. I would recommend increasing the size until you find a size that the Plasma theme has icons for
  • This is a pre-existing issue, but the alignment and consistency for text subtitles is poor:
    We might want to consider right-aligning everything and using the same font size and opacity. Thoughts, @manueljlin?
gvgeo updated this revision to Diff 74158.Jan 22 2020, 6:14 PM

SmallMedium icons
remove margin from inhibitor message

gvgeo added a comment.EditedJan 22 2020, 6:19 PM

About the text subtitles

I will not touch Tooltip in BatteryItem.qml, has a bug with Grid and uses Flow instead. That made very difficult to fix the tooltip and details.
R120:d12521b149fa7766fbbeb9957bde14b4a6278928

If this bug has been fixed from then. I could probably made the changes.

Edit: Maybe there is an other hack that I haven't thought about, but I just can't change the alignment with the flow layout.


Opacity and size test.

gvgeo added a comment.Jan 22 2020, 8:15 PM

An other interesting idea from apol D26735#597158 was to use checkbox, when a keyboard has only 1 setting for brightness like mine, as an on/off switch.
But haven't found a way to fit it visually.

Is there anything stopping a port away from the FlowLayout and using a GridLayout, or even just using one Rowlayout per row, all stuck in a ColumnLayout?

gvgeo added a comment.Jan 22 2020, 8:44 PM

First line in the commit message above Workaround a crash in GridLayout when using a Repeater. But was 5 years ago, don't know if this has been fixed, or how to reproduce for testing.
Rowlayout in ColumnLayout should be fine, instead of repeater. I will look into it.

gvgeo planned changes to this revision.Jan 23 2020, 6:02 AM
gvgeo updated this revision to Diff 74222.EditedJan 23 2020, 12:26 PM

Removed battery tooltip (had no extra info).
Changed details view.

Would be better without colon and without grid arrangement?

opacity: 0.6

opacity:1

Current patch: split opacity
(warning message was forced for screenshot only)

gvgeo updated this revision to Diff 74228.Jan 23 2020, 1:20 PM

Remove detailsLoader visibility

davidedmundson requested changes to this revision.Jan 23 2020, 1:32 PM

Few of my initial comments do not seem to be addressed yet.

applets/batterymonitor/package/contents/ui/BatteryItem.qml
57–58

You can't do that.

That'll change the column size, which will change our text bounding box which will change our paintedWidth. A loop.

This revision now requires changes to proceed.Jan 23 2020, 1:32 PM
broulik added inline comments.
applets/batterymonitor/package/contents/ui/BatteryItem.qml
57–58

That's been in there ever since the initial release 2013.

davidedmundson added inline comments.Jan 23 2020, 2:10 PM
applets/batterymonitor/package/contents/ui/BatteryItem.qml
57–58

Well that explains "// GridLayout crashes with a Repeater in it somehow"

gvgeo marked 3 inline comments as done.Jan 23 2020, 2:17 PM

I marked 3 comments as done that had forgotten, as I removed the old tooltip code.
If there is something else, please be more specific. I can not see a better way to address any of your comments.

applets/batterymonitor/package/contents/ui/BatteryItem.qml
57–58

No, it does not. Those two comments were in the same commit, to avoid the GridLayout crash.
https://phabricator.kde.org/R120:d12521b149fa7766fbbeb9957bde14b4a6278928

ngraham added inline comments.Jan 23 2020, 4:33 PM
applets/batterymonitor/package/contents/ui/PopupDialog.qml
76

The icon is a bit small:

gvgeo updated this revision to Diff 74262.EditedJan 23 2020, 4:59 PM

Fix inhibition icon size.

Elisa icon will always look small. But now is the correct size.

gvgeo marked an inline comment as done.Jan 23 2020, 5:00 PM

Looking pretty good to me, UI-wise.

gvgeo updated this revision to Diff 74314.Jan 24 2020, 3:03 PM

Don't show suppression messages if PM is disabled.

davidedmundson added inline comments.Jan 24 2020, 3:21 PM
applets/batterymonitor/package/contents/ui/BatteryItem.qml
48

width used inside a layout

58

width/height used inside a layout

66

here

applets/batterymonitor/package/contents/ui/PopupDialog.qml
147

and here

applets/batterymonitor/package/contents/ui/PowerManagementItem.qml
34

and here

and you shouldn't need to be using chidrenRect.width
when your child has

anchors.fill: parent

that's a loop

gvgeo updated this revision to Diff 74322.Jan 24 2020, 5:52 PM

Remove as much as I could width and height settings.
Removed loader. Moved instead flow in place.

gvgeo marked 5 inline comments as done.Jan 24 2020, 5:58 PM

After some effort, I reached in these conclusions:
1.Is not that big of a problem to specify width in a layout. The big problem is specifying preferredWidth and height in the same item. Breaks the dimensions, this is what got me confused.
2.Item set as property, does not get managed by layout.

There are still some width set in places. That do not respond to preferredWidth, minimumWidth or maximumWidth.

applets/batterymonitor/package/contents/ui/BatteryItem.qml
112

This does not react to preferredWidth, minimumWidth or maximumWidth. Is it really managed by Layout?

davidedmundson added inline comments.Jan 24 2020, 6:45 PM
applets/batterymonitor/package/contents/ui/BatteryItem.qml
112

It's not Flow isn't a layout.

gvgeo updated this revision to Diff 74347.Jan 25 2020, 6:19 AM

Need specific width for columns in FLow. Changed maximumWidth to preferredWidth.

gvgeo marked 2 inline comments as done.Jan 25 2020, 6:20 AM

Might be better to be aligned to the left since it looks similar now to the battery percentage, but aside from that it's great! Also, sorry for replying late :P

gvgeo edited the summary of this revision. (Show Details)Feb 5 2020, 11:59 AM
gvgeo edited the summary of this revision. (Show Details)
gvgeo added a comment.Feb 5 2020, 12:05 PM

I 'll wait for response about the alignment before I update test plan too.

gvgeo updated this revision to Diff 75297.Feb 9 2020, 11:39 AM

Small improvement of Power Managment checkbox's tooltip code.

gvgeo updated this revision to Diff 75315.Feb 9 2020, 9:20 PM

Up it goes again

gvgeo edited the summary of this revision. (Show Details)Feb 9 2020, 9:23 PM
gvgeo edited the test plan for this revision. (Show Details)

Thanks, and sorry for wasting your time :x

gvgeo added a comment.EditedFeb 14 2020, 5:40 PM

No need to worry. The flip of the toolbar is was a small proportion of the patch, most of the time I try to solve other problems. And in the end of the line was a team decision.

gvgeo added a comment.Feb 18 2020, 4:52 PM

Is there any problem with this patch?

We're waiting for @davidedmundson to re-review it.

manueljlin accepted this revision.Feb 19 2020, 6:18 PM
gvgeo edited the summary of this revision. (Show Details)Feb 27 2020, 5:49 AM
gvgeo abandoned this revision.Mar 4 2020, 9:32 AM

Unfortunately I won't be able to continue with this patch about battery applet.
Feel free to use/modify if you want.