Details
- Reviewers
philip - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rS355978: top: display battery capacity remaining
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Except for some very minor style issues, this looks like a good change! Thanks for the submission.
| usr.bin/top/display.c | ||
|---|---|---|
| 1328 ↗ | (On Diff #65799) | Follow the style of the rest of the file please. |
| usr.bin/top/display.h | ||
| 27 ↗ | (On Diff #65799) | This should be in alphabetical order like the other variables (style). |
This looks generally good, one minor comment and I think it's ready to go.
| usr.bin/top/machine.c | ||
|---|---|---|
| 384 ↗ | (On Diff #65853) | It would be nice to have a quick comment here saying that only batteries reported via ACPI are counted. Anybody still running APM won't have these numbers, but that's totally fine: APM is likely to be deleted soon anyway as laptops that require you use APM aren't really viable anymore as laptops... I'm also unsure how the 'pinebook' reports this information, but that too is a niche thing that's kinda weird still.... So just a quick comment will suffice. |
- I know someone in the city who has a Pinebook, should I try running on it? 2) So now I should just set a comment that says this is ACPI only? 😄
| usr.bin/top/machine.c | ||
|---|---|---|
| 384 ↗ | (On Diff #65853) | Should I try running this on a Pinebook? I know someone in the city who has one. |
1: sure, why not. I wouldn't hold this up for that testing, it's more of a good to know read.
2: Yup.