[WIP] Add macOS 10.14 Dark Mode support

[habovh]

WIP

As suggested in #276 by @jansol and @nangtrongvuon, I'm creating a PR so we can push the discussion further. Bear in mind this is my first contribution to a Swift project. 🙈

This is a work in progress, and should probably not be merged as-is.

Set theme flow

The Theme changes originally follow this flow during the app lifecycle:

• Select theme
  • App initialization
    • Get theme list from core
    • Get current theme from preferences
  • Manual theme setting
    • Get theme name from menu item
• Apply theme
  • Send a set theme message to core
  • core responds to mac with a JSON-like theme object
  • The provided theme object is parsed by mac and sets the menu item as active

Working stuff

What works in the current PR, is selecting the macOS theme after app initialization. Doing so will inhibit the set theme message usually sent to core, and update the UI with a Theme object directly.

Not working stuff

I found that setting the theme this way does not work for some keys (they are explicitly listed in comments in the code).
Opening the app when macOS theme is selected results in an error from core, because of the set theme message sent on startup with a theme name that does not exist in core. We could inhibit this as well.
core seems to still have a word to say in what's displayed on the screen somehow, and the previously selected theme will apply to the editor text color.

Thoughts

As you can see, this PR is pretty hacky, so we may want to come up with a better solution. But I feel this is definitely doable.

I feel the best solution (for a working v1) would be to have a dedicated theme menu entry that is treated like a special case that sends a set theme message to core with an existing theme (e.g. base16-ocean.light) based on the current selected system color scheme, so the editor can use the text color from core, and then set the rest of the app UI with a Theme object using semantic NSColors.

If any of you guys have insights or general tips regarding Swift development please feel free to share your feedback and contribute!

😃

[habovh]

Not sure if this is the right way to access AppDelegate. Not even sure if I'm supposed to access the AppDelegate in a VC.

[cmyr]

I would avoid storing the variable, and instead just call (NSApp.delegate as? AppDelegate)?.whatever() in line, as needed.

(edit: I mean, this is still kind of code smell, but we do it. 😕)

[habovh]

Thanks for your feedback on this, always interesting to see various practices!

[ollieatkinson]

It's generally not good practice to reference the AppDelegate inside of your view controllers (directly).

How about using a protocol?

e.g.

protocol ThemeChangeDelegate {
    func themeChanged(name: String, theme: Theme)
}


...

extension AppDelegate: ThemeChangeDelegate { }

...

weak var themeChangeDelegate: ThemeChangeDelegate? = NSApp.delegate as? ThemeChangeDelegate

It will make it much easier to write a unit test and decouples the editor view.

[terhechte]

Adding to what @ollieatkinson wrote, the best option is to use dependency injection to add the delegate into the view controller:

Here, the main view controller is setting its themeChangeDelegate on the ThemeViewController, so it is just copying its dependency into the ThemeViewController

class MainViewController: NSViewController {
  private var themeViewController: ThemeViewController?
  private var themeChangeDelegate: ThemeChangeDelegate?
  override func viewDidLoad() {
    super.viewDidLoad()
    self.themeViewController = ThemeViewController()
    self.themeViewController?.themeChangeDelegate = self.themeChangeDelegate
  }
}

Here, the AppDelegate sets up the MainViewController and installs itself as the dependency. That way, nobody ever accesses the AppDelegate except for the appdelegate itself.

class AppDelegate: NSObject, NSApplicationDelegate, ThemeChangeDelegate {
  var mainViewController = MainViewController()
  func applicationWillFinishLaunching(_ aNotification: Notification) {
    mainViewController.themeChangeDelegate = self
  }
}

[cmyr]

Okay, so a high-level thought:

Themes provide two things: they provide styling to editor components (the gutter, the background color, etc) but they mostly determine how syntax highlighting is handled.

This is why themes are provided by core, and why we notify core of theme changes, etcetera.

So I think it's important that this feature take that reality into account. For instance, you should notice that when you switch from the macOS theme to another theme and back again, the other theme's highlighting colors stay in place.

(I'm also noticing that this PR doesn't seem to ever really give me a dark theme?)

(edit: I have it now, but it's definitely a bit flakey. not sure what's up..)

In any case, what we would probably want to do, in the long-term, is to bundle two custom themes, say, 'macOS Light' and 'macOS Dark' with xi-mac. These themes would look good against the expected light/dark Mojave backgrounds, and when one of these themes was selected we would manually override certain properties in core.

For now, I think it would be enough to pick two of the current themes that are 'good enough', and tell core to use those when we're using the system styling. I think probably InspiredGithub and Solarized (dark) would do the trick. You'll have to separately stash whether we're actually using the macOS theme somewhere (and update this state as the user changes themes) because we will get the normal theme_changed notification; we'll just be overriding certain properties on the received theme in these cases.

The other solution of bundling special themes is actually pretty doable right now, and you could also investigate that, if interested; it really is the best longer-term approach. It would involve including theme files in the xi-mac bundle (sort of how we currently include the default config file) and then copying them into the themes directory of the application bundle on first run. If you took this approach things could be less hacky; you would know that if you received one of these special themes you were always supposed to override the appropriate properties.

Does that makes sense?

[cmyr]

Example of use when the previously active theme clashes:

[jansol]

I think this should use the vibrant blending mode, covered in the WWDC video, starting from around 28:30 -- it's supposed to avoid this kind of issues.

Besides that, I think making this a special theme is the wrong approach. Instead I would make it a flag that when enabled uses the theme colors and vibrant blending to tint the transparent window. It should be fine even when switching between dark and light, but in case it's not (and even if it is, because this would be quite cool) there could be an option to specify two active themes (dark and light) and have xi-mac automatically switch when the system appearance changes.

[cmyr]

Okay, I haven't dug into the mojave docs/talks yet so I defer to @jansol's suggestion!

[habovh]

@cmyr I'm glad I came out with the same analysis as your high-level thought!

I originally had in mind what you just said in your for now paragraph. I was going for the option to enable the system dark/light mode, which would use one of core's themes for each corresponding state and override the actual app properties.

I also like the long term idea you have, to bundle 2 special themes for each corresponding system theme. However there's a few things I don't get. First, I couldn't find out how/where themes are stored/defined in core. I obviously need to understand this in order to bundle themes from xi-mac. Also, I guess a theme (syntax highlighting-speaking) has a fair amount of colors that have to work in harmony. I wouldn't say I'm a bad UI designer, but there's a high chance people are used to be able to choose their syntax-highlighting independently of their app UI.

What I'm thinking is similar to what @jansol said, we could consider the best of both worlds:

• Allow users to set a flag that they want the UI to match the system light/dark setting
• Allow them to choose which syntax highlighting they want to use independently

Here's a mockup of how the user could use this:

Light system theme	Dark system theme

Thinking long-term, we could imagine that users could import their own themes, flag them as made for dark or light backgrounds, and xi-mac can automagically use them depending on the system's preferences.

However, I do not think the vibrant blending mode is quite suitable for the use case we have. As I understand it, vibrant blending mode should be used to modify colors that are displayed on top of a potentially changing background color. So one might think it would be useful to update the syntax highlighting colors, but there's a few issues:

• vibrant blending mode should not be used for non-grayscale colors (read: do not use vibrant blending mode on colored items)
• the only colors we should override are the background, gutter, and default text. Using semantic NSColors is IMHO the way to go there.

Do you guys think the two active themes and the flag solution is the way to go there?

[jansol]

Do you guys think the two active themes and the flag solution is the way to go there?

I believe so. An interesting thing to try would be to allow the user to specify just one theme and then automatically invert its colors (or maybe just their brightness/value?) for the other appearance. Some syntax themes also work quite well on both bright and dark backgrounds, especially when the default color is automatically updated to match (Monokai for example looks quite nice against either background) so automatic inversion should be optional too, if it is supported. This is of course an optional bit of functionality at this stage.

[habovh]

While inverting colors would keep the same contrast compared to the other type of background, it also breaks any color semantics the theme could've set. The rule of thumb here would be to invert only the grayscale colors, because they're supposedly the ones that are going to lack contrast against an inverted background. But again, there's no guarantee that it will actually look nice. I'm not a fan of trying to guess what the theme should look like. While I agree some themes can look good on either backgrounds, I still think that being able to choose one light and one dark theme would be better for the user.

[cmyr]

A drive-by comment: something else to keep in mind is the "unified_titlebar" feature, and how that works with this?

[cmyr]

also @habovh there's some info on how themes work here: xi-editor/xi-editor#883

[habovh]

I guess unified titlebar should update according the system theme if the flag is set to follow the system theme, otherwise we can still update it according to the theme if it isDark?

@cmyr thanks for the heads up! Will definitely read this!

[jansol]

While inverting colors would keep the same contrast compared to the other type of background, it also breaks any color semantics the theme could've set.

Hence value invert aka keep the hue & saturation but invert the lightness.

Anyway this is mostly a "could be nice" gimmick for people who are too lazy to define two separate themes.

[habovh]

Oh sorry @jansol I didn't get the invert lightness only 🙈 My bad, this is definitely a could be nice feature!

[cmyr]

@habovh @jansol is there consensus here? I'm happy to more or less stay out of it, if you two agree on an approach you have my blessing. 😁

[jansol]

I think so. To recap:

• Have a flag for switching between theme-defined background color and NSAppearance
• User can select separate light and dark syntax themes
• Leaving one of those unset causes xi to derive it from the other one by inverting the lightness of the entire palette

What I'm not sure is what the best notation for this would be in the config.. Simply presence/absence of theme_light/theme_dark keys? Using a separate theme key? That one would require the lightness to be tagged directly in the theme, which would require some more refactoring.

[cmyr]

We could just let the user pick whatever theme they want for 'light' and 'dark' and if it looks bad that's just user error?

[ollieatkinson]

Would it be too much work for each theme to define a light and dark variant?

[jansol]

It's mostly a question of knowing whether lightness should be inverted for the light or the dark variant or neither. It could be configured like this:

theme_light = "Tomorrow Night Eighties"
theme_light_invert = "true"
theme_dark = "Tomorrow Night Eighties"
theme_dark_invert = "false"

We'll want some slightly more sophisticated logic for defaults though:

• If a theme variant is specified, default to invert = "false"
• If a variant is not specified, default to the other variant
  • if invert is not specified, default to "true"

This way it is enough to specify theme_light = "InspiredGitHub" and you get a reasonable default for both light and dark variants.

@ollieatkinson For bundled themes that is a reasonable thing to do. However, if you found a nice theme online and wanted to use that you'd end up having to manually create a copy and replace every single color. So yes, I'd say that would be needlessly much work, at least if automatic inverting turns out to look anywhere near as good as I expect.

[ryanbloom]

@habovh Thanks for working on this! I have a few comments, but take them with a grain of salt since I haven't even installed Mojave yet. 😄

Find bar

When the appearance is dark, we probably want the find bar to take on a veryDarkGray color. See this definition and this method.

Theme pairing heuristics

If the user doesn't specify a separate dark theme, I see a few different things we could try:

1. If the theme contains the word "light", replace it with "dark" and see if that theme exists.
2. Switch to the most recently active theme that has a dark background.
3. Invert the colors as @jansol described.

I'm thinking we might try 1 first, then fall back to 2 or 3. If some combination of these is reliable enough, we could even do away with the whole light theme/dark theme thing in the config. There would be a single new preference:

# Based on system appearance, automatically switch between light and dark versions of the active syntax theme (if available)
auto_theme = true

The theme menu would look like it does now. When dark mode is enabled, Xi would guess what the appropriate theme is and activate that one. This makes it easier to switch themes, since you only have to change one setting instead of 2 or 4. The disadvantage, of course, is that the system might guess wrong.

Just an alternative to consider.

[ryanbloom]

When unifiedTitlebar == true, we should be using the theme color instead of the system appearance here.

[cmyr]

@habovh What's the status on this? Are you blocked on anything?

[habovh]

@cmyr Sorry for delaying this, but I'm unable to work on this PR at the moment. If anyone has time to dig into it feel free to do so!
I'll keep you posted once I have the time to work on the feature so the job doesn't end up being made twice.

[cmyr]

@habovh no worries! this is definitely not critical right now, if you get around to it eventually that's great, if someone else grabs it that's great too.

[cmyr]

@habovh I'm going to close this for now; if you're interested in picking it up again at some point feel free to reopen!