Fixed battery status in MacOS X.

[thrushcat]

Some time ago battery status under MacOS was broken due to changes in pmset output. In last MacOS X Sierra we have battery id an output of pmset looks like:

[thruhscat:~/develop/liquidprompt-thrushcat]↥ fix/macos-pmset 18s ± pmset -g batt
Now drawing from 'AC Power'
 -InternalBattery-0 (id=2228323)    100%; charged; 0:00 remaining present: true

[jasmas]

Tested & verified this fix works on Sierra 10.12.5 (16F73).

[augmentedfourth]

Is this going to get merged? Looks good to me.

[liujoey]

👍

[Rycieos]

We have a ton of reviewers that supposedly have tested this patch on new versions of MacOS, but none that have tested on a version before the change to pmset. I'd be happy to recommend this if someone can test for backwards compatibility.

[augmentedfourth]

@Rycieos In what OS version was the behavior changed? Is it possible to add a conditional to change the behavior only for OS versions after the change?

[augmentedfourth]

In this particular commit, though, it seems that anything that would match the old sed regex will also match the new one; it seems we're only adding cases. Also, I'm a little curious why we have [0-9]*[0-9] instead of [0-9]+, but either should work.

[Rycieos]

The change looks well made, and I think it would work correctly on older versions. I would like a test to make sure though.

We don't know what version broke pmset, as @thrushcat just said "some time ago", and no one else commented with that info. The linked issue #481 claims that it works in El Capitan (version 10.11.6) and it broke in Sierra (10.12.1). Maybe @mraible can test on their machine if it's still on an older version.

[mraible]

@Rycieos I've upgraded all my machines to High Sierra 10.13.3.

[augmentedfourth]

I don't have any systems older than Sierra (10.12.x) available to test. @Rycieos, does that mean we're stuck here unless we find someone with an El Capitan machine? It's pretty frustrating to have all these broken 10.12 shell environments if that's all we're waiting for...

[jasmas]

I could probably test in a VM. 🤷🏻‍♂️

[Rycieos]

@augmentedfourth The situation we want to avoid is breaking environments for old MacOS X versions, and have someone be affected and make a bug report.

That said, since we have so many vocal users about this issue, and since normal practice is to update MacOS, I would recommend merging this, and if it does somehow break old versions and someone is actually using an old version, we can cross that bridge then.

[augmentedfourth]

👍🏻

[augmentedfourth]

@Rycieos who's responsible for merging this? Do we need to take into account @cvigo's recommendation on #536 (which in this PR would refer to lines 1293 and 1315, adding 2 instead of 1 to the return code)?

[cmrnpref]

I have tested this patch on MacOS version 10.9.5, and got the following output:
percent=66 batt_status='discharging'
It's working for me.

[Rycieos]

Thanks @cmrnpref! I'm comfortable with merging this, since we have a test on an older version.

We will need @dolmen to take a look at it and merge it. Based on how many people have tested this, I would recommend this patch to be merged.

I do not think we should take parts from the other PR, since it does a lot more modifications and has only one tester.

[cvigo]

@Rycieos et al

I think the comment I made in #536 also applies to this.

according to the comments at line 1314 and to the expected behavior in the rest of the code, the _lp_battery() function should return:

• 2 when the battery is charging and below the thereshold

• 3 when the battery is charging and above the thereshold:

    case "$batt_status" in
        charged | "")
        return 4
        ;;
        discharging)
            echo -nE "$percent"
            # under => 0, above => 1
            return $(( percent > LP_BATTERY_THRESHOLD ))
        ;;
        *)  # "charging", "AC attached"
            echo -nE "$percent"
            # under => 2, above => 3
            return $(( 1 + ( percent > LP_BATTERY_THRESHOLD ) ))
        ;;
    esac
  

As it is now, if will return 1 when charging and below, and 2 when charging and above, as 1 + boolean_value is either 1 or 2

In other words, the function returns the same (1) when discharging+above and when charging+below.

[Rycieos]

@cvigo I hadn't looked too deeply at it before, but now I can say that it looks like you are right.

The refactoring done by commit 4636e0e seems to have introduced an off-by-one error that has the consequences you describe. I have only looked at the code and haven't done any testing yet.

However, this issue is not limited to MacOS, since the same logic, and therefore the same error, is used for the Linux version.

This should probably be reported as it's own issue/PR, since this issue+PR are solely about MacOS issues.

[augmentedfourth]

@Rycieos Who can accept this pull request? I'm really looking forward to fixed battery status on my Mac.

[Rycieos]

@augmentedfourth as I said previously, @dolmen is the sole maintainer on this project at the moment, so only he can merge pull requests.

@dolmen, as this is a bugfix, and we have had numerous testers on this patch, I would recommend we merge this.

[augmentedfourth]

@dolmen Can you please review/merge this? Lots of Mac users have been frustrated for a very long time with this.

[augmentedfourth]

@dolmen Pinging yet again. Please merge...

[nojhan]

Merged on develop, thanks!

[Rycieos]

This is now in master as fefbe01 and in v1.12-beta.1.