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

Dynamic battery widget #975

Merged
merged 2 commits into from Jan 15, 2023

Conversation

RaresCon
Copy link
Contributor

@RaresCon RaresCon commented Jan 13, 2023

Description

A description of the change, what it does, and why it was made. If relevant (such as any change that modifies the UI), please provide screenshots of the changes:

For bottom to know that there are no batteries on the system, I added the battery::Manager to the options.rs file because here is the first moment bottom verifies battery configuration by reading the config file, which may or may not contain the battery field, but for a better UX, it doesn't matter what bottom finds in the config file now, if it doesn't retrieve battery data, it just ignores the battery widget all together. Added cfg checks for battery feature.
If needed, it can be adjusted so that if the config file contains the battery field, it will still show the widget even without batteries on the system.

PR discussed at UPB-CS-OpenSourceUpstream (both accounts for commits are mine)

Issue

If applicable, what issue does this address?

closes: #793

Testing

If relevant, please state how this was tested. All changes must be tested to work:

It was tested with and without config files on a PC without battery and on a laptop with and without battery installed.

If this is a code change, please also indicate which platforms were tested:

  • Windows
  • macOS
  • Linux

Checklist

If relevant, ensure the following have been met:

  • Areas your change affects have been linted using rustfmt (cargo fmt)
  • The change has been tested and doesn't appear to cause any unintended breakage
  • Documentation has been added/updated if needed (README.md, help menu, doc pages, etc.)
  • The pull request passes the provided CI pipeline
  • There are no merge conflicts
  • If relevant, new tests were added (don't worry too much about coverage)

RaresCon and others added 2 commits January 11, 2023 14:59
For bottom to know that there are no batteries on the system,
I added the battery::Manager to the options.rs file because
here is the first moment bottom verifies battery configuration
by reading the config file, which may or may not contain the
battery field, but for a better UX, it doesn't matter what bottom
finds in the config file now, if it doesn't retrieve battery data,
it just ignores the battery widget all together.
If needed, it can be adjusted so that if the config file contains
the battery field, it will still show the widget.
I guarded the options.rs in two places for battery module that can be missing in the feature list.
@ClementTsang ClementTsang self-assigned this Jan 13, 2023
@ClementTsang ClementTsang self-requested a review January 13, 2023 23:13
Copy link
Owner

@ClementTsang ClementTsang left a comment

Choose a reason for hiding this comment

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

Seems fine to me, will merge in a bit. Thanks!

@ClementTsang ClementTsang merged commit f8db340 into ClementTsang:master Jan 15, 2023
@ClementTsang
Copy link
Owner

ClementTsang commented Jan 15, 2023

@all-contributors please add @NitrogenDev for code.

Repository owner deleted a comment from allcontributors bot Jan 15, 2023
@ClementTsang
Copy link
Owner

@all-contributors please add @RaresCon for code.

@allcontributors
Copy link
Contributor

@ClementTsang

@RaresCon already contributed before to code

@RaresCon RaresCon deleted the dynamic_battery_widget branch January 20, 2023 23:18
@jamartin9 jamartin9 mentioned this pull request Nov 19, 2023
10 tasks
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.

Dynamic battery widget
3 participants