Bug 301533 - Option "Show Multiple Batteries" does nothing
Summary: Option "Show Multiple Batteries" does nothing
Status: RESOLVED FIXED
Alias: None
Product: plasma4
Classification: Plasma
Component: widget-battery (show other bugs)
Version: 4.8.90 (beta2)
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Plasma Bugs List
URL:
Keywords: regression
: 314207 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-06-09 21:26 UTC by David Edmundson
Modified: 2013-04-21 23:19 UTC (History)
9 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.9.2


Attachments
Old widget on desktop. (31.72 KB, image/png)
2012-06-14 18:43 UTC, Dominik Cermak
Details
Old widget in systray. (88.08 KB, image/png)
2012-06-14 18:44 UTC, Dominik Cermak
Details
Old widget's tooltip. (27.50 KB, image/png)
2012-06-14 18:45 UTC, Dominik Cermak
Details
New widget on desktop for comparison. (28.77 KB, image/png)
2012-06-14 18:46 UTC, Dominik Cermak
Details
New widget in systray for comparison. (64.09 KB, image/png)
2012-06-14 18:47 UTC, Dominik Cermak
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Edmundson 2012-06-09 21:26:24 UTC
Code in battermonitior.qml shows the config value gets loaded to a property and that's all.

If we're dropping this feature, the config option must be removed. If we're keeping it, it needs fixing.
Comment 1 David Edmundson 2012-06-09 21:27:34 UTC
Edit: Even if it did do something this code is broken.

It's loaded by default to "false" then only changed to the config value if the config changes.
Comment 2 Dominik Cermak 2012-06-13 19:35:59 UTC
Just installed 4.9 Beta 2 (4.8.90) and the battery widget lost some functionality for me.
I have a notebook with 2 batteries and the old widget (4.8.4) shows the percentage of both and when added to the desktop (not systray) it shows two battery icons, one for each battery available.
Now with the new widget both doesn't work anymore. It's a fairly big regression for me.
Comment 3 Viranch Mehta 2012-06-14 17:50:07 UTC
How about showing the average charge percent of all batteries in the popup dialog and separate charge percent of each battery in the applet tooltip?
Comment 4 Dominik Cermak 2012-06-14 18:42:18 UTC
(In reply to comment #3)
I just made some screen-shots to compare the old widget with the new widget.
Especially I like the behavior of the old widget on the desktop: showing two battery icons. (see desktop.png). So it would be nice to have this in the new widget too.
> How about showing the average charge percent of all batteries in the popup
> dialog and separate charge percent of each battery in the applet tooltip?
If I understood your comment correctly you want to make the tooltip like the old one (tooltip.png). That's fine for me.
The old widget also showed both percentages in the popup (see systray.png).

So my wish would be that the behavior on the desktop will be the same as in the old one.
And for tooltip and/or systray it doesn't really matter for me where it is, it just should be possible to see the percentages of both batteries. To have it in the tooltip as well as in the popup would be the optimal solution, but if that's too hard to implement it also would be ok for me to have it in either the tooltip or the popup.
Comment 5 Dominik Cermak 2012-06-14 18:43:13 UTC
Created attachment 71836 [details]
Old widget on desktop.
Comment 6 Dominik Cermak 2012-06-14 18:44:24 UTC
Created attachment 71837 [details]
Old widget in systray.
Comment 7 Dominik Cermak 2012-06-14 18:45:02 UTC
Created attachment 71838 [details]
Old widget's tooltip.
Comment 8 Dominik Cermak 2012-06-14 18:46:45 UTC
Created attachment 71839 [details]
New widget on desktop for comparison.
Comment 9 Dominik Cermak 2012-06-14 18:47:24 UTC
Created attachment 71840 [details]
New widget in systray for comparison.
Comment 10 Sebastian Kügler 2012-07-16 18:38:23 UTC
For what it's worth, Viranch has created a patch for this, and IIRC, it's pure QML, so easy to test.

The patch is a bit too scary for 4.9 in this phase though, so it won't be in 4.9.0. If Dominik could test it, that would help, however.
Comment 11 Dominik Cermak 2012-07-17 06:46:56 UTC
(In reply to comment #10)
> For what it's worth, Viranch has created a patch for this, and IIRC, it's
> pure QML, so easy to test.

For QML it's simply applying the patch and reloading the applet?

> The patch is a bit too scary for 4.9 in this phase though, so it won't be in
> 4.9.0. If Dominik could test it, that would help, however.

I can test it but I have to know where it is :-). Having a link would be nice.
Comment 12 Viranch Mehta 2012-07-17 07:53:41 UTC
(In reply to comment #11)
> For QML it's simply applying the patch and reloading the applet?

Yes.

> I can test it but I have to know where it is :-). Having a link would be
> nice.

Here: https://git.reviewboard.kde.org/r/105277/diff/
Comment 13 Dominik Cermak 2012-08-01 06:11:38 UTC
(In reply to comment #12)
> (In reply to comment #11)
> Here: https://git.reviewboard.kde.org/r/105277/diff/

Sorry for the late reply, somehow bugzilla didn't notified my of you comment so I saw it just today.
Anyway this diff doesn't apply on top of RC2, and as 4.9 will be released today afternoon/evening (IIRC) I will have that soon (maybe even today, or more likely tomorrow) so it would be cool if you could rebase it on top of that.
Comment 14 Viranch Mehta 2012-08-01 13:37:01 UTC
(In reply to comment #13)
> Sorry for the late reply, somehow bugzilla didn't notified my of you comment
> so I saw it just today.
> Anyway this diff doesn't apply on top of RC2, and as 4.9 will be released
> today afternoon/evening (IIRC) I will have that soon (maybe even today, or
> more likely tomorrow) so it would be cool if you could rebase it on top of
> that.

Here's the rebased patched: http://sprunge.us/UTTW
I generated the diff on top of KDE/4.9 git branch.
Comment 15 Dominik Cermak 2012-08-01 14:54:18 UTC
(In reply to comment #14)
> Here's the rebased patched: http://sprunge.us/UTTW
> I generated the diff on top of KDE/4.9 git branch.

Thanks, I can't comment on the code, but from a users POV this works the way I wanted it to work :)
And it is even superior to the old one, nice!

Again thanks a lot for your work.
Comment 16 Viranch Mehta 2012-08-01 18:35:10 UTC
(In reply to comment #15)
> Thanks, I can't comment on the code, but from a users POV this works the way
> I wanted it to work :)
> And it is even superior to the old one, nice!
> 
> Again thanks a lot for your work.

Thanks for the quick testing. I'll soon put it up on kde-look.org so others can use it until the feature is shipped.
Comment 17 Sebastian Wessalowski 2012-08-05 14:00:42 UTC
Thanks for the patch. it works partly but only the icon in the notification updates. the battery percentage does not update itself in the popup if i click on it. Would someone fix this? I am unable to fix it on my own.
Comment 18 Duncan Townsend 2012-08-07 18:02:21 UTC
Well, this patch mostly works for me. I'm seeing the same thing as Sebastian.
Comment 19 Myriam Schweingruber 2012-09-07 10:51:56 UTC
We are past 4.9.1 now, any news on this?
Comment 20 Viranch Mehta 2012-09-07 12:46:08 UTC
(In reply to comment #19)
> We are past 4.9.1 now, any news on this?

Yes, working on it, will push the fix tonight.
Comment 21 Viranch Mehta 2012-09-08 00:19:24 UTC
Git commit 90db8129ff19ec7ce9e0ef294ae7e8a8e9398f7c by Viranch Mehta.
Committed on 08/09/2012 at 02:16.
Pushed by viranch into branch 'master'.

Implement support for multiple batteries in battery
monitor applet
REVIEW: 105277

A  +77   -0    plasma/generic/applets/batterymonitor/contents/code/logic.js
M  +18   -12   plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml
M  +73   -68   plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml

http://commits.kde.org/kde-workspace/90db8129ff19ec7ce9e0ef294ae7e8a8e9398f7c
Comment 22 Hyacinthe Cartiaux 2012-09-08 11:54:18 UTC
Thanks Viranch Mehta ! :)

I think this bug is also related to #253453 "with two batteries, remaining time is calculated for the discharging battery only" ( https://bugs.kde.org/show_bug.cgi?id=253453 ) and #304510 "Battery plasmoid does not show remaining time" ( https://bugs.kde.org/show_bug.cgi?id=304510 ).

We could not reproduce these 2 bugs and we don't know if they are fixed or not. Also, I suspect that the power management system of KDE was sometime disturbed because of these bugs.
Do we have some news on this topic ?
Comment 23 Viranch Mehta 2012-09-10 10:20:57 UTC
(In reply to comment #22)
> I think this bug is also related to #253453 "with two batteries, remaining
> time is calculated for the discharging battery only" (
> https://bugs.kde.org/show_bug.cgi?id=253453 ) and #304510 "Battery plasmoid
> does not show remaining time" ( https://bugs.kde.org/show_bug.cgi?id=304510
> ).

I'm not sure why #253453 was there in the first place, but if you think its not the case anymore, feel free to close it, because according to the latest code, the remaining time should be correct one.

#304510 is now a feature request since the plasma developers decided to remove the remaining time feature due to the inaccuracy of the same.

> We could not reproduce these 2 bugs and we don't know if they are fixed or
> not. Also, I suspect that the power management system of KDE was sometime
> disturbed because of these bugs.
> Do we have some news on this topic ?

I'm sorry I'm not aware of that.
Comment 24 Thilo-Alexander Ginkel 2012-11-17 23:53:55 UTC
The bug is marked as "fixed in 4.9.2", but the fix seems not to be included in 4.9.2 or 4.9.3. Any chance this will end up in 4.9.x some time soon?
Comment 25 Myriam Schweingruber 2012-12-16 10:21:54 UTC
(In reply to comment #24)
> The bug is marked as "fixed in 4.9.2", but the fix seems not to be included
> in 4.9.2 or 4.9.3. Any chance this will end up in 4.9.x some time soon?

Can you still reproduce this after cleaning your $HOME/.kde/ folder (or just removing the plasma*rc files from $HOME/.kde/share/config/)? The folder might be named $HOME/.kde4/.. on some systems BTW
Comment 26 Thilo-Alexander Ginkel 2012-12-23 09:27:47 UTC
The last time I looked the required fix was simply not present in the 4.9.x
branch. So, clearing any config files won't help...
Comment 27 Kevin Clevenger 2013-02-11 17:56:32 UTC
The fix is still not present in 4.9.5. The fixes in https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/90db8129ff19ec7ce9e0ef294ae7e8a8e9398f7c work.
Comment 28 Sebastian Kügler 2013-02-12 04:19:00 UTC
I've just had a look, it seems the fix was not backported to the KDE/4.9 branch at that time. It did go into KDE/4.10.

As there won't be any new releases of the 4.9 range, in order to receive this fix, update to Plasma Workspaces 4.10.

Thank you for verifying that the fix works, I don't have this kind of hardware available to test with, unfortunately.
Comment 29 Kevin Clevenger 2013-02-12 22:51:28 UTC
I spoke too soon, the fixes in C#27 do work for a desktop plasmoid, however the systray no longer updates at all and bug #253453 is still an issue.
Comment 30 David Edmundson 2013-04-21 23:19:49 UTC
*** Bug 314207 has been marked as a duplicate of this bug. ***