Directly read macOS theme preference instead of using defaults

[BlackHoleFox]

Follows on #2197 by improving the implementation of macos_dark_mode_active.

For reference there are a lot of ways to read preferences on macOS. Here's the 4 I know of:

1. defaults CLI
2. NSUserDefaults via Objective-C
3. CFPreferences via CoreFoundation (C/C++)
4. Directly reading and parsing the file from disk

Out of all of these, defaults is the slowest because it involves spawning and executing a whole separate executable to read one value. defaults is implemented via 2, which is implemented via 3 at the end. In order to speed this up, I just used option 4 as bat's needs are fairly simple. Using CFPreferences would have also worked but the use of #![deny(unsafe_code)] indicated that would have been in conflict with the crate's goals since calling C APIs requires unsafe by nature.

If bat wanted to do this 100% as-intended by Apple, and possibly take advantage of any OS-level caching, CFPreferences.framework would be the way. If you're good with the unsafe required, happy to swap out the implementation. Though I don't ever really expect the file reading method to break.

So this new implementation reads the hardcoded settings path and directly parses the plist data with the plist crate (already in the dependency tree from another crate, which was nice). As a result, in a release build, reading the theme is now 12x faster and pretty much imperceptible. The lowest time I saw with the old implementation was 4-5ms, while a release build call to macos_dark_mode_active now takes <500us on an M1 Max MacBook.

[Enselic]

Very nice 🤩

Unsure when I will get the time to do a detailed review, but on a high level this looks good.

I have two main concerns initially. I wrote one in a comment, and the other is: if we should try to find a crate to use for "is dark mode on". If you upstream your fix, then more projects and users would benefit from it.

That said, if there is no such crate mature enough for us to use, then personally I am fine with rolling our own code. I think this code path makes a lot of sense to optimize.

[Enselic]

Is this always the path? Regardless of Mac OS version and regardless of system/user settings?

[BlackHoleFox]

Regardless of mac OS version

Yes, to the best of what I know. Its been at this path since at least 10.7 in 2011 (which is the lowest Rust supports) but probably even before that so I'm confident this will stick around.

regardless of system/user settings?

I think so? I don't have any documentation to back it up but I have never seen it anywhere else different. If this isn't enough, then we should just use CFPreferences so this is abstracted away.

[BlackHoleFox]

if we should try to find a crate to use for "is dark mode on". If you upstream your fix, then more projects and users would benefit from it.

The one I know about is dark-light, which uses some Objective-C via objc to read the app's current appearance from the system. I didn't list this one originally because its not reading any preferences directly. imo this fix wouldn't be upstreamable to a crate like that anyway which is meant to be used in more traditional applications with UI since for them, using AppKit via ObjectiveC is the Apple endorsed way to get the app's appearance for a few reasons (user app overrides being the notable one).

The upside if bat used it instead is that the function here becomes a single match block and use a much higher-level abstraction, but the downsides are:

• Extra third-party dependency for a very small implementation need
  • This also bloats the lockfile because of the other platform's dependencies
• Significantly slower then directly reading the value (~50ms)

[Enselic]

This looks good to me, so I'm going to set this as Approved. With one big caveat. Which is that I have not verified or benchmarked this change. The reason being that I have switched jobs, and lost access to my Macs. I currently don't plan on buying a Mac in the near term.

So I'll set this as Approved now, and give other maintainers a chance to try this on a Mac if they have access to one (I'm unsure if they do).

If none of us has access to any Mac any longer, I think we need to trust you and blindly merge this :)

[Enselic]

@BlackHoleFox I almost forgot: Can you add any entry about this change to CHANGELOG.md please?

[BlackHoleFox]

Sure. I added a line to the Other category.

If none of us has access to any Mac any longer, I think we need to trust you and blindly merge this :)

In the very unlikely case this breaks on someone else's computer, and/or no one else does have a Mac, I can probably do a bugfix or create a different implementation

[Enselic]

@sharkdp @keith-hall Does any one of you still run a Mac? Due to a series of coincidences, I have ended up running Windows (and Ubuntu via WSL2 as my day to day Rust dev environment).

If none of you run Mac and are able to verify this change, I think our only option is to blindly merge this.

[sharkdp]

Thank you for working on this 👍

Does any one of you still run a Mac?

I don't, but I'm okay with trusting @BlackHoleFox on this.

[BlackHoleFox]

Just realized there isn't much paper trail about the functionality here, my bad. Here's the detection on macOS 13:

Dark theme on:

Light theme on:

[Enselic]

Let's get this merged now, and handle any follow-up problems (if any) in separate PRs.