D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham updated this revision to Diff 72811. ngraham added a comment. Update according to review comments and tweak presentation REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=71222=72811 BRANCH arcpatch-D24070 REVISION DETAIL

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham planned changes to this revision. ngraham added a comment. I'm not done yet. ;-) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: ngraham, #vdg, #plasma, ndavis, mthw Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev,

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham commandeered this revision. ngraham edited reviewers, added: mthw; removed: ngraham. ngraham added a comment. Yoink. :) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: ngraham, #vdg, #plasma, ndavis, mthw Cc: GB_2, ndavis, broulik,

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. 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,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. @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? REPOSITORY R120

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. Oh you know me, I'll be doing KDE stuff on Christmas eve. :) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, ndavis Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. @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.

D24070: Don't use toolTipMainText to show info, rather use the second line

GB_2 added a comment. Ping :-) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, ndavis Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham,

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. Yeah, I'll take over the review. Working through some other stuff first, will resume later today or tomorrow. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, ndavis Cc: ndavis, broulik,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. What does it mean? Can this patch still go forward? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 71222. mthw added a comment. Tried to implement your suggestion, hopefully correctly, works correctly so far. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=71175=71222 BRANCH master REVISION DETAIL

D24070: Don't use toolTipMainText to show info, rather use the second line

broulik added a comment. 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); }

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 71175. mthw added a comment. 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. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D24070: Don't use toolTipMainText to show info, rather use the second line

broulik added a comment. Probably: > MainText: Battery at X% > SubText: Powermanagement disabled\n > HH:MM left REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added inline comments. INLINE COMMENTS > broulik wrote in batterymonitor.qml:56 > 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? OK, would you like to see: MainText: Power

D24070: Don't use toolTipMainText to show info, rather use the second line

broulik requested changes to this revision. broulik added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > broulik wrote in batterymonitor.qml:56 > You can probably just return `plasmoid.title` in the default case. > Also, what's the reason for not showing

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. Is everything fine now? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 69910. mthw added a comment. Done REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=69869=69910 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24070 AFFECTED FILES

D24070: Don't use toolTipMainText to show info, rather use the second line

broulik added inline comments. INLINE COMMENTS > batterymonitor.qml:72 > +if (powermanagementDisabled) { > +return i18n("Power management is disabled"); > +} This early return still hasn't been addressed. When I keep the screen on I still want to see my remaining

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. All good now, @broulik? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 69869. mthw added a comment. Added i18n() REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=69854=69869 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24070 AFFECTED FILES

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham accepted this revision. ngraham added a comment. Perfect. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 69854. mthw added a comment. 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

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. In D24070#563390 , @ndavis wrote: > Here's what I see when the power cable is plugged in: F7766890: Screenshot_20191116_125846.png > > Here's when I unplug it: F7766893:

D24070: Don't use toolTipMainText to show info, rather use the second line

ndavis added a comment. Here's what I see when the power cable is plugged in: F7766890: Screenshot_20191116_125846.png Here's when I unplug it: F7766893: Screenshot_20191116_125912.png REPOSITORY R120

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. From previous comments: In D24070#534832 , @ngraham wrote: > The logic looks sane to me and the UI is good, but the time-to-empty/time-to-full values still don't always update correctly for me after a state change. I

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. Yes, when the battery is being charged, right after I unplug the cord. I have a feeling that there is something odd going on with the power subsystem on my computer, since sometimes it doesn't show the time remaining. Most of the time it looks correct, like this:

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. @ngraham When does that happen? I am guessing, when batery is being charged, right? This is how it looks for me: https://imgur.com/a/vSkdDmo REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma,

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. Erm, actually, I just saw this... F7763635: Screenshot_20191114_094001.png REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc:

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham accepted this revision. ngraham added a comment. LGTM now! REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 69737. mthw added a comment. Issue above fixed and some stuff removed. I do have a question though, what info should be shown when "Power management" is disabled? With this patch it shows: - Battery level + managemet disabled (with battery) - Widget

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. Hmm F7761926: Screenshot_20191113_151950.png Looks like the condition where `remainingTime` is 0 or undefined needs better

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 69693. mthw added a comment. I think I made all the changes you wanted. Hopefully I didn't break anything. It works correctly on first glance. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66959=69693

D24070: Don't use toolTipMainText to show info, rather use the second line

broulik added a comment. There's also a lot of code duplication making the diff hard to read. Please group if statements together where it makes sense, e.g. avpod things like if (foo) { return plasmoid.title; } else if (foo) { return plasmoid.title; or

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. @ndavis Unless this needs a rebase, all I need is review/approval. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev,

D24070: Don't use toolTipMainText to show info, rather use the second line

ndavis added a comment. @broulik @mthw ping :) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. @broulik Any other changes needed? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66959. mthw marked an inline comment as done. mthw added a comment. > Also, what's the reason for not showing percentage when power management is disabled? This isn't about the service not running or being a desktop PC, this is true when screens are forced

D24070: Don't use toolTipMainText to show info, rather use the second line

broulik added inline comments. INLINE COMMENTS > batterymonitor.qml:56 > +if (powermanagementDisabled) { > +return i18n("Battery and Brightness") > +} You can probably just return `plasmoid.title` in the default case. Also, what's the reason for not showing

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. In D24070#538806 , @mthw wrote: > Like this? Don't we need to return something when "remainingTime <= 0"? No, because in this case it just doesn't show a subtitle, which I think is fine. REPOSITORY R120 Plasma

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66958. mthw added a comment. Like this? Don't we need to return something when "remainingTime <= 0"? REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66487=66958 BRANCH master REVISION DETAIL

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. From what I can tell, the problem with `remainingTime` being 0 sometimes is a pre-existing bug that the current code is hiding fairly well by not showing the string that uses

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. Is there still something that needs to be fixed, or can this go in? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev,

D24070: Don't use toolTipMainText to show info, rather use the second line

ndavis added a comment. Works just as well as it did before the patch for me. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma, broulik, ndavis Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham accepted this revision. ngraham added a reviewer: broulik. ngraham added a comment. The logic looks sane to me and the UI is good, but the time-to-empty/time-to-full values still don't always update correctly for me after a state change. I suspect this is something that @broulik will

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. @ngraham Do you still have issues with this? @ndavis could you try this too? Or anyone else? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma Cc: ndavis, broulik, plasma-devel, LeGast00n,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66487. mthw added a comment. Re-order again REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66485=66487 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24070 AFFECTED FILES

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. In D24070#534771 , @ngraham wrote: > Heh, how I get this every time my battery is discharging: F7387261: Screenshot_20190919_102717.png Not here: F7387375:

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66485. mthw added a comment. Remove ";", reorder and simplify if clauses REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66482=66485 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24070

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. Heh, how I get this every time my battery is discharging: F7387261: Screenshot_20190919_102717.png REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma Cc:

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw marked 6 inline comments as done. mthw added a comment. Is the formatting OK? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66482. mthw added a comment. Switch lines REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66480=66482 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24070 AFFECTED FILES

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. In D24070#534721 , @mthw wrote: > I can do that if @ngraham agrees. Yeah alright, I don't have any strong negative preference. Consistency with the volume applet's tooltip is also another argument in favor.

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66480. mthw added a comment. Use battery at X% REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66476=66480 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24070 AFFECTED FILES

D24070: Don't use toolTipMainText to show info, rather use the second line

ndavis added a comment. I think it's redundant to say "change level" since the percentage is always the charge level. I also think keeping the language consistent is a good idea. The volume tooltip says "Volume at %", which is why I originally suggested "Battery at %". REPOSITORY R120

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. I can do that if @ngraham agrees. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot,

D24070: Don't use toolTipMainText to show info, rather use the second line

ndavis added a comment. In D24070#534719 , @mthw wrote: > @ndavis So, generally, you would like to switch the two lines? Yes REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham,

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. @ndavis So, generally, you would like to switch the two lines? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24070 To: mthw, ngraham, #vdg, #plasma Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,

D24070: Don't use toolTipMainText to show info, rather use the second line

ndavis added a comment. In D24070#534585 , @ngraham wrote: > In D24070#534447 , @ndavis wrote: > > > In D24070#534378 , @broulik wrote: > > > > >

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66476. mthw added a comment. un-capitalize REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66474=66476 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24070 AFFECTED FILES

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added inline comments. INLINE COMMENTS > batterymonitor.qml:70 > +if (remainingTime > 0) { > +return i18n("%1 Remaining", > KCoreAddons.Format.formatDuration(remainingTime, > KCoreAddons.FormatTypes.HideSeconds)) > +} un-capitalize the R >

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. In D24070#534687 , @mthw wrote: > In D24070#534676 , @ngraham wrote: > > > Uh-oh, looks like there's a binding that isn't getting updated properly; the "time to charge/time

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw added a comment. In D24070#534676 , @ngraham wrote: > Uh-oh, looks like there's a binding that isn't getting updated properly; the "time to charge/time to empty" number always reads as 0:00 for me: F7386917: Screenshot_20190919_092713.png

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. Uh-oh, looks like there's a binding that isn't getting updated properly; the "time to charge/time to empty" number always reads as 0:00 for me: F7386917:

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66474. mthw added a comment. re-added powermanagementDisabled and fixed white spaces REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66470=66474 BRANCH master REVISION DETAIL

D24070: Don't use toolTipMainText to show info, rather use the second line

mthw updated this revision to Diff 66470. mthw added a comment. Made changes, according to instructions, tested, should work correctly REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24070?vs=66431=66470 BRANCH master REVISION DETAIL

D24070: Don't use toolTipMainText to show info, rather use the second line

ngraham added a comment. In D24070#534447 , @ndavis wrote: > In D24070#534378 , @broulik wrote: > > > Volume shows the percentage in the main title so this is inconsistent now. > > Perhaps

D24070: Don't use toolTipMainText to show info, rather use the second line

ndavis added a comment. In D24070#534378 , @broulik wrote: > Volume shows the percentage in the main title so this is inconsistent now. > Perhaps instead show some other information like remaining time in the subtext? Yes, perhaps it

D24070: Don't use toolTipMainText to show info, rather use the second line

broulik added a comment. Volume shows the percentage in the main title so this is inconsistent now. Perhaps instead show some other information like remaining time in the subtext? INLINE COMMENTS > batterymonitor.qml:82 > } > +if (powermanagementDisabled) { > +