Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix battery tooltip text when minutesLeft is unknown #2160

Merged
merged 2 commits into from
Jun 15, 2019

Conversation

rrrapha
Copy link
Contributor

@rrrapha rrrapha commented Jun 12, 2019

Currently, the tooltip says "Time until charged: 0" or "Time left: 0" when the time is actually unknown.

The change makes all versions of the util return -1 if minutesLeft is unknown.
In src/widget/wbattery.cpp, we have:

if (minutesLeft == -1) {
    setBaseTooltip(tr("Time until charged unknown."));
}

(minutesLeft can be 0 if the time is less than an minute)

Note that upower returns 0 for unknown:
https://upower.freedesktop.org/docs/Device.html#Device:TimeToEmpty
Windows uses -1:
https://docs.microsoft.com/en-us/windows/desktop/api/winbase/ns-winbase-_system_power_status
On OSX, the property is optional and not set when unknown (i guess).

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes. It looks already good for me.
I have left some some minor comments to improve the code quality. They apply in the same way to the code of all OSs.

m_dPercentage = 0.0;
m_chargingState = Battery::UNKNOWN;
int seconds_left;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declaration can be combined with the assignment below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -15,9 +15,10 @@ BatteryWindows::~BatteryWindows() {
}

void BatteryWindows::read() {
m_iMinutesLeft = 0;
m_iMinutesLeft = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not matter at all here and it is not you fault, but in general I prefer not to mess with member variables during a function call. During to later changes, one may not aware of this and use the same member in a subroutine, wit surprising effects.

Can you move that into the else branch below. This way the member value is always valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this later, in a separate pull request?
Fixing this correctly for all OSs is a big change..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@@ -16,7 +16,7 @@ BatteryMac::~BatteryMac() {
}

void BatteryMac::read() {
m_iMinutesLeft = 0;
m_iMinutesLeft = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please define -1 as constexp in a anonymous namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done this in the latest commit (I hope I didn't misunderstand ;)

@daschuer daschuer merged commit 9775e68 into mixxxdj:master Jun 15, 2019
@rrrapha rrrapha deleted the battery-unknown branch June 15, 2019 12:08
haslersn pushed a commit to haslersn/mixxx that referenced this pull request Jun 16, 2019
Fix battery tooltip text when minutesLeft is unknown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants