[Applets/Battery] Don't use toolTipMainText to show info, rather use the second line
ClosedPublic

Authored by ngraham on Sep 19 2019, 7:50 AM.

Details

Summary

Currently the applet uses the main line in tooltip to display all info. Instead it should show always the same first line like other widgets and use the second line to show info.

BUG: 411973
FIXED-IN: 5.19.0

Test Plan

The point of this patch is to make the tooltip more like the style used in the Audio Volume applet:

Other "after" images:

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
mthw updated this revision to Diff 69854.Nov 16 2019, 6:57 PM

Secondly if you wait long enough, the description will change, meaning the remaining time isn't calculated imediately after plug/un-plug which looks like a bug somewhere else. This change only adds a message that remaining time isn't available yet.

ngraham accepted this revision.Nov 16 2019, 11:27 PM

Perfect.

mthw updated this revision to Diff 69869.Nov 17 2019, 8:18 AM

Added i18n()

broulik added inline comments.Nov 18 2019, 8:22 AM
applets/batterymonitor/package/contents/ui/batterymonitor.qml
69

This early return still hasn't been addressed.
When I keep the screen on I still want to see my remaining battery time.

mthw added a comment.Dec 10 2019, 9:22 AM

Is everything fine now?

broulik requested changes to this revision.Dec 10 2019, 9:23 AM
broulik added inline comments.
applets/batterymonitor/package/contents/ui/batterymonitor.qml
53

You still haven't addressed this. Don't just return when powermanagementDisabled, I would still want to see my battery state when this is the case?

This revision now requires changes to proceed.Dec 10 2019, 9:23 AM
mthw added inline comments.Dec 10 2019, 10:03 AM
applets/batterymonitor/package/contents/ui/batterymonitor.qml
53

OK, would you like to see:
MainText: Power management disabled
SubText: Battery at X%, HH:MM left
Or:
MainText: Battery at X%
SubText: HH:MM left
Or something else?

Probably:

MainText: Battery at X%
SubText: Powermanagement disabled\n
HH:MM left

mthw updated this revision to Diff 71175.Dec 10 2019, 11:26 AM

The second line is everywhere now. I guessed it would make more sense. But if you dont't like it that way, I can remove it.

ngraham accepted this revision.Dec 10 2019, 3:59 PM
broulik requested changes to this revision.Dec 10 2019, 3:59 PM
This revision now requires changes to proceed.Dec 10 2019, 3:59 PM

You can just do something like

var parts = [];
if (powerManagementDisabled) {
    parts.push(i18n("Powermanagement disabled"));
}
if (pmSource.data["AC Adapter"] && pmSource.data["AC Adapter"]["Plugged in"]) {
    parts.push(blabla);
} else {
    parts.push(blablabla);
}
parts.push(whatever other texts);
return parts.join("\n");
mthw updated this revision to Diff 71222.Dec 10 2019, 6:43 PM

Tried to implement your suggestion, hopefully correctly, works correctly so far.

broulik resigned from this revision.Dec 11 2019, 9:55 AM
mthw added a comment.Dec 12 2019, 11:49 AM

What does it mean? Can this patch still go forward?

Yeah, I'll take over the review. Working through some other stuff first, will resume later today or tomorrow.

GB_2 added a subscriber: GB_2.Dec 17 2019, 1:01 PM

Ping :-)

mthw added a comment.EditedDec 19 2019, 3:02 PM

@ngraham could you please look at this? I would say it's finished and there isn't much time left until the next freeze. I also don't expect you to do this during the holidays, so it would be nice to have it finished before them. I am sorry for being impatient.

Oh you know me, I'll be doing KDE stuff on Christmas eve. :)

mthw added a comment.Jan 1 2020, 9:13 AM

@ngraham Sorry to bother you again, but tomorrow is repo freeze and it would be unfortunate if this doesn't make it in. I mean it's finished and it works correctly, right? If needed we can still do small polishing during the Beta period, correct?

Yeah sorry, I need to block out some time to build a mental model of all the possible combinations currently, and what this changes them to, to make sure nothing regresses. Don't worry, this can go in after the soft feature freeze since it was already in progress, as long as it gets in before the hard feature freeze/string freeze.

ngraham commandeered this revision.Jan 5 2020, 3:05 AM
ngraham edited reviewers, added: mthw; removed: ngraham.

Yoink. :)

mthw accepted this revision.Jan 5 2020, 7:57 AM
ngraham planned changes to this revision.Jan 5 2020, 3:59 PM

I'm not done yet. ;-)

ngraham updated this revision to Diff 72811.Jan 5 2020, 5:11 PM

Update according to review comments and tweak presentation

ngraham marked 6 inline comments as done and an inline comment as not done.Jan 5 2020, 5:15 PM
ngraham marked an inline comment as done.
ngraham retitled this revision from Don't use toolTipMainText to show info, rather use the second line to [WIP] Don't use toolTipMainText to show info, rather use the second line.
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 72812.Jan 5 2020, 5:18 PM
ngraham edited the test plan for this revision. (Show Details)

--whitespace changes

mthw added a comment.Jan 5 2020, 5:25 PM

Power management disabled message is not show anymore if there is no battery present. Is that intentional?

ngraham updated this revision to Diff 72816.Jan 5 2020, 5:32 PM

Also show "Power management is disabled" text when there are no batteries

Oops, good catch.

mthw accepted this revision.Jan 6 2020, 7:53 AM

Anyway, anytime you feel like this is finished you can land it. You don't have to wait for me.

ngraham updated this revision to Diff 73088.Jan 8 2020, 7:09 PM

Fix the remaining FIXMEs

ngraham edited the test plan for this revision. (Show Details)Jan 8 2020, 7:10 PM
mthw added a comment.Jan 14 2020, 3:07 PM

@ngraham How does it look with this patch? Will it make it in?

ngraham planned changes to this revision.Jan 14 2020, 3:53 PM
ngraham updated this revision to Diff 73554.Jan 14 2020, 6:34 PM

Fix FIXME and don't show "Calculating blah..." text in subtitle when exact time isn't available

ngraham edited the test plan for this revision. (Show Details)Jan 14 2020, 8:14 PM

@broulik I'm leery of landing this for 5.18 since today is the last day for string changes. Maybe we can land it early in the 5.19 cycle?

mthw added a comment.Jan 15 2020, 3:14 PM

Maybe we can land it early in the 5.19 cycle?

Do we really have to? That would delay this by additional 5 months., which is really unfortunate since work (a very slow work but still...) on this begun about 4 moths ago. I mean this patch works correctly right? And even if we find an issue during Beta, it can still be fixed. That's what Betas are for, anyway.

Strings can't be changed during the beta.

There really isn't a rush for this, is there? It's a pretty minor thing. I want it too, but it's more important to get it right than to get it in earlier.

ngraham retitled this revision from [WIP] Don't use toolTipMainText to show info, rather use the second line to [Applets/Battery] Don't use toolTipMainText to show info, rather use the second line.Jan 29 2020, 7:06 PM
mthw added a comment.Jan 30 2020, 8:36 AM

Are you asking for my approval?

ngraham updated this revision to Diff 74702.Jan 30 2020, 3:41 PM

"Battery may be damaged" -> "Not charging" (it's more accurate)

@broulik I get the sense that you're boycotting this patch :)

broulik added inline comments.Fri, Mar 13, 4:11 PM
applets/batterymonitor/package/contents/ui/batterymonitor.qml
80

Why this early return?

broulik accepted this revision.Fri, Mar 13, 4:12 PM

With that early return addressed please

This revision is now accepted and ready to land.Fri, Mar 13, 4:12 PM
ngraham updated this revision to Diff 77592.Fri, Mar 13, 6:40 PM
ngraham marked an inline comment as done.

Don't return early

ngraham edited the summary of this revision. (Show Details)Fri, Mar 13, 6:45 PM
This revision was automatically updated to reflect the committed changes.