fix(battery): report when no charge threshold exists instead of faking success - #3385
fix(battery): report when no charge threshold exists instead of faking success#3385Mario-Mohar wants to merge 2 commits into
Conversation
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| StyledText { | ||
| width: parent.width | ||
| visible: Qt.platform.os === "linux" && !root.chargeLimitSupported |
There was a problem hiding this comment.
On a machine with no battery at all (desktop, VM) the probe exits 1, so chargeLimitSupported becomes false and this line now shows "No writable charge threshold file was found under /sys/class/power_supply." to every desktop user who opens the Battery tab. The tab is always listed in SettingsTabs.qml (no battery gating), and the Status card already handles this case with visible: BatteryService.batteryAvailable (line 152).
Gate on battery presence so the explanation only appears when there actually is a battery whose thresholds can't be written:
| visible: Qt.platform.os === "linux" && !root.chargeLimitSupported | |
| visible: Qt.platform.os === "linux" && BatteryService.batteryAvailable && !root.chargeLimitSupported |
Claude reviewSolid fix — the script logic, exit-code split and I18n usage all check out; one minor UX gap on batteryless machines.
Checked: apply/probe shell logic (glob, per-battery break, write failure vs. no-interface exit codes), Process lifecycle and binding order, |
Follow-up to the review on AvengeMedia#3385: the new explanation showed on desktops and VMs with no battery at all, where it reads as a fault report about hardware the machine was never going to have. Both the explanation and the apply button now require BatteryService.batteryAvailable. Hiding the button there is the same point the pull request makes elsewhere: offering a control that cannot work is what produced the false success in the first place. A machine without a battery has no charge threshold to write to, so the limit was never applicable. BatteryService.batteryAvailable is already the gate used twice in this file for the same reason.
|
Good catch on the batteryless case, fixed in 3835c2d. The explanation was gated on Both the explanation and the apply button now also require
|
2 participants
Description
On hardware that exposes none of the three sysfs charge threshold files, Apply to Hardware wrote nothing and still reported success. The script walks every
BAT*, matches none of the threeif/elifbranches, andshexits 0, whichonExitedcould not tell apart from a real write.This is not a rare corner. Lenovo IdeaPad and ThinkBook models on
ideapad_laptopexposecharge_types(Fast/Standard/Long_Life) instead, and the kernel says so itself: "conservation_mode attribute has been deprecated, see charge_types". I have one of those machines here, so everything below was checked on the affected hardware rather than reasoned about.Three changes:
The script now says whether it wrote anything. The
if/elifchain became a loop over the same three names in the same order, so the priority does not change, withfound=1set when a branch matches andexit 2when none did. A failed write still exits 1, so "this machine has no interface" and "the write failed" stay apart.The toast tells the truth. Exit code 2 gives "Charge limit not supported on this hardware" instead of the success toast.
The button is not offered when it cannot work. A probe runs once when the tab loads and hides the Apply button if none of the three files exists, with a line naming the reason in its place. The exit code 2 branch stays as the authority, since the probe runs once and a battery can be removed or a driver reloaded after that.
What I deliberately left alone: the slider. The issue suggests disabling it too, but
batteryChargeLimitalso drives the "Charge Limit Reached" notification inBatteryService.qml, which works on this hardware and is independent of sysfs. Disabling the slider would break a working feature to tidy up a broken one.Not in this PR:
charge_typessupport. That interface is a mode selector, not a percentage, and the cap behindLong_Lifeis firmware defined, so it needs its own control and a decision about how it sits next to a 50-100 slider. That is the follow-up the issue describes as separable, and it seems better to agree on the shape of it first.Type of change
Related issues
Fixes #3278
Screenshots / video
Taken on the affected machine, a Lenovo IdeaPad whose
BAT0hascharge_typesand none of the three threshold files, in a nested Hyprland running this branch.Checklist
I18n.tr()with translator context, reusing existing terms where possiblemake fmt, added/updated tests,make testpasses, andgo mod tidyis cleanmake lint-qmlwith no new warningsNotes on those boxes. Two new terms,
Charge limit not supported on this hardwareandNo writable charge threshold file was found under /sys/class/power_supply., both with a translator comment, and the second is shared by the toast and the inline line rather than duplicated. I ranextract_translations.pyto confirm they land as expected and then reverteden.jsonandtemplate.json, since those are yours to sync. No Go changes.make lint-qmlwants a.qmlls.inifrom a full install, so I ran/usr/lib/qt6/bin/qmllinton the file instead: nothing beyond the unresolvedqs.*imports every file produces, and themultiline-stringscount is unchanged from master, which is why the probe command is written on one line. No docs PR, the change removes a wrong message rather than adding behavior to document.How it was verified
The apply script and the probe were run against a fake
power_supplytree as well as the real one:charge_typescharge_control_end_thresholdpresentcharge_control_limit_maxwritten, as beforeBAT*at all