Differential D20533

Display a shortcut to the kcm Energy Information from the battery applet context menu
ClosedPublic
Actions

Authored by meven on Apr 14 2019, 11:45 AM.

Details

Reviewers
ngraham
broulik
filipf
Group Reviewers
Plasma
VDG
Commits
R120:a4e231e7b6c0: Display a shortcut to the kcm Energy Information from the battery applet…
Summary

Depends on D20598

Test Plan

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.
meven created this revision.Apr 14 2019, 11:45 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 14 2019, 11:45 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Apr 14 2019, 11:45 AM
meven retitled this revision from Display two shortcuts to the kcm Energy Information from the battery applet. to Display two shortcuts to the kcm Energy Information from the battery applet.Apr 14 2019, 11:45 AM
meven edited the test plan for this revision. (Show Details)
meven added reviewers: ngraham, broulik.
apol added a subscriber: apol.Apr 14 2019, 5:51 PM
apol added inline comments.
applets/batterymonitor/package/contents/ui/batterymonitor.qml
106

Typo? kcm_energyinfo vs kcms_energyinfo

meven marked an inline comment as done.Apr 14 2019, 5:55 PM
meven added inline comments.
applets/batterymonitor/package/contents/ui/batterymonitor.qml
106

Thanks

meven updated this revision to Diff 56236.Apr 14 2019, 5:56 PM
meven marked 2 inline comments as done.

Capitalization

meven added a comment.Apr 14 2019, 6:05 PM

The energy info kcm would need some attention though.

kcmshell5 kcm_energyinfo
ngraham added a reviewer: VDG.Apr 15 2019, 2:17 AM

I like having it in the context menu. Putting it in the expanded pop-up I'm less sure about, because the icon isn't as recognizable and maybe we might be cluttering the view up too much?

broulik added a comment.Apr 15 2019, 3:56 PM

-1 please let's cleanup / rewrite the energy info kcm first using Kirigami FormLayout and kcm components before making it prominently accessible from the UI

ngraham added a comment.Apr 15 2019, 5:34 PM
In D20533#450711, @broulik wrote:

-1 please let's cleanup / rewrite the energy info kcm first using Kirigami FormLayout and kcm components before making it prominently accessible from the UI

Yeah, that makes sense.

@meven, wanna take a crack at it? Here's the relevant Phab task: T7256

meven planned changes to this revision.Apr 16 2019, 2:20 PM
In D20533#450777, @ngraham wrote:
In D20533#450711, @broulik wrote:

-1 please let's cleanup / rewrite the energy info kcm first using Kirigami FormLayout and kcm components before making it prominently accessible from the UI

Yeah, that makes sense.

@meven, wanna take a crack at it? Here's the relevant Phab task: T7256

The task concerns the energy settings, not the energy information / kinfocenter.

broulik added inline comments.Apr 16 2019, 2:22 PM
applets/batterymonitor/package/contents/ui/batterymonitor.qml
146

This should be in a check for kcmEnergyInformationAuthorized

meven retitled this revision from Display two shortcuts to the kcm Energy Information from the battery applet to Display a shortcut to the kcm Energy Information from the battery applet context menu.Apr 19 2019, 11:22 AM
meven edited the test plan for this revision. (Show Details)
meven updated this revision to Diff 56587.Apr 19 2019, 11:22 AM

Remove button in popup, add check in the context menu

meven marked an inline comment as done.Apr 19 2019, 11:23 AM
meven updated this revision to Diff 56588.Apr 19 2019, 11:24 AM

Inverted checks

ngraham added inline comments.Apr 19 2019, 2:39 PM
applets/batterymonitor/package/contents/ui/batterymonitor.qml
143

Generally we start menu items with action verbs. And in this case we don't need ellipsis because no further input from the user is required as a part of the action. See https://hig.kde.org/style/writing/labels.html#using-ellipses-in-labels

So for this string, I would recommend "Show Energy Information"

meven updated this revision to Diff 56602.Apr 19 2019, 6:08 PM

Start menu action with a verb

meven marked an inline comment as done.Apr 19 2019, 6:09 PM
meven added inline comments.
applets/batterymonitor/package/contents/ui/batterymonitor.qml
143

Thanks for pointing it out.

meven marked 2 inline comments as done.Apr 19 2019, 6:09 PM
meven updated this revision to Diff 56903.Apr 24 2019, 3:56 PM

Check the energy information is available before showing a link to it, don't show a link to it from the battery applet

ngraham requested changes to this revision.Jun 21 2019, 9:52 AM
ngraham added inline comments.
applets/batterymonitor/package/contents/ui/batterymonitor.qml
143

Not fixed yet. The text needs to be "Show Energy Information..."

This revision now requires changes to proceed.Jun 21 2019, 9:52 AM
meven updated this revision to Diff 60208.Jun 21 2019, 10:23 AM

Fix text

filipf accepted this revision.Jun 21 2019, 10:34 AM
ngraham accepted this revision.Jun 21 2019, 10:51 AM
This revision is now accepted and ready to land.Jun 21 2019, 10:51 AM

Revision Contents
Changeset List

Diff 60233

View Options

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

Loading...