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

Simplify/optimize battery util on linux #2287

Merged
merged 1 commit into from Sep 22, 2019

Conversation

rrrapha
Copy link
Contributor

@rrrapha rrrapha commented Sep 18, 2019

Instead of looping over a copy of the device objects, get the appropriate device directly via up_client_get_display_device().
Checking is-present makes sure that hot pluggable batteries are handled correctly.

The upower client is only created once in the class constructor.
(There was a comment about this in the code, but I could not reproduce the issue)

Note that this removes support for upower < 0.99.0. (It should still work on Xenial)

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

The uncontrollable side-effects caused by redefining symbols in the header file are not acceptable. Revert those changes and replace with a forward declaration.

src/util/battery/batterylinux.h Outdated Show resolved Hide resolved
src/util/battery/batterylinux.h Outdated Show resolved Hide resolved
src/util/battery/batterylinux.h Outdated Show resolved Hide resolved
src/util/battery/batterylinux.cpp Outdated Show resolved Hide resolved
src/util/battery/batterylinux.cpp Outdated Show resolved Hide resolved
src/util/battery/batterylinux.cpp Outdated Show resolved Hide resolved
@uklotzde
Copy link
Contributor

uklotzde commented Sep 18, 2019

I'm not convinced that the code gets easier to follow by using deeply nested if blocks. Safely initializing the members with invalid/unknown default values and returning as soon as possible when you are not able to read the required values is simpler and more robust.

@rrrapha rrrapha force-pushed the batterylinux branch 2 times, most recently from 7a21b47 to 4d33dde Compare September 18, 2019 23:00
@rrrapha rrrapha changed the title Smplify/optimize battery util on linux Simplify/optimize battery util on linux Sep 18, 2019
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Much easier to read now. Just some minor remarks.

src/util/battery/batterylinux.h Show resolved Hide resolved
src/util/battery/batterylinux.cpp Outdated Show resolved Hide resolved
@uklotzde
Copy link
Contributor

The void* member is the correct solution here.

@uklotzde
Copy link
Contributor

Thank you for taking care of these little performance details that easily slip through unnoticed!

LGTM

@uklotzde uklotzde self-requested a review September 22, 2019 08:43
@uklotzde uklotzde merged commit a7ae01d into mixxxdj:master Sep 22, 2019
@rrrapha
Copy link
Contributor Author

rrrapha commented Sep 23, 2019

I discovered an interesting side effect of the 'create the upower client once' change.
When I stop the dbus daemon while mixxx is running, mixxx receives a SIGTERM signal and terminates immediately.
This is not ideal IMO, but I don't know how to 'fix' it ..
@uklotzde Do you think we can live with that?

@uklotzde
Copy link
Contributor

Stopping the dbus service kills my whole desktop. When stopping the upower service I don't experience any issues.

@rrrapha rrrapha deleted the batterylinux branch October 17, 2019 20:37
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.

None yet

2 participants