Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Add macOS 10.14 Dark Mode support #287

Closed
wants to merge 3 commits into from
Closed

[WIP] Add macOS 10.14 Dark Mode support #287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c062c85
Add static systemTheme using 10.14 semantic s
habovh Oct 3, 2018
b18a89f
Disable theme-based override on OSX >= 10.14
habovh Oct 3, 2018
c42512e
Add macOS Debug -> Theme menu item and handle specific case
habovh Oct 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions XiEditor/EditViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class EditViewController: NSViewController, EditViewDataSource, FindDelegate, Sc

@IBOutlet weak var editViewHeight: NSLayoutConstraint!
@IBOutlet weak var editViewWidth: NSLayoutConstraint!

let appDelegate = NSApp.delegate as! AppDelegate
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cmyr cmyr Oct 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😕)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  }
}


lazy var findViewController: FindViewController! = {
let storyboard = NSStoryboard(name: NSStoryboard.Name(rawValue: "Main"), bundle: nil)
Expand Down Expand Up @@ -148,11 +150,15 @@ class EditViewController: NSViewController, EditViewDataSource, FindDelegate, Sc

statusBar.updateStatusBarColor(newBackgroundColor: self.theme.background, newTextColor: self.theme.foreground, newUnifiedTitlebar: unifiedTitlebar)
findViewController.updateColor(newBackgroundColor: self.theme.background, unifiedTitlebar: unifiedTitlebar)

if color.isDark && unifiedTitlebar {
window.appearance = NSAppearance(named: NSAppearance.Name.vibrantDark)
if #available(OSX 10.14, *) {
// Let system decide, disregarding active theme color
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
window.appearance = NSAppearance(named: NSAppearance.Name.aqua)
if color.isDark && unifiedTitlebar {
window.appearance = NSAppearance(named: NSAppearance.Name.vibrantDark)
} else {
window.appearance = NSAppearance(named: NSAppearance.Name.aqua)
}
}
}
}
Expand Down Expand Up @@ -614,6 +620,11 @@ class EditViewController: NSViewController, EditViewDataSource, FindDelegate, Sc
// MARK: - Debug Methods
@IBAction func debugSetTheme(_ sender: NSMenuItem) {
guard sender.state != NSControl.StateValue.on else { print("theme already active"); return }
if sender.title == "macOS" {
let theme = Theme.systemTheme()
appDelegate.themeChanged(name: sender.title, theme: theme)
return
}
let req = Events.SetTheme(themeName: sender.title)
document.dispatcher.coreConnection.sendRpcAsync(req.method, params: req.params!)
}
Expand Down Expand Up @@ -755,6 +766,12 @@ class EditViewController: NSViewController, EditViewDataSource, FindDelegate, Sc
.first?.title

pluginsMenu.removeAllItems()

let systemThemeMenuItem = NSMenuItem(title: "macOS", action: #selector(EditViewController.debugSetTheme(_:)), keyEquivalent: "")
systemThemeMenuItem.state = NSControl.StateValue(rawValue: ("macOS" == currentlyActive) ? 1 : 0)

pluginsMenu.addItem(systemThemeMenuItem)
pluginsMenu.addItem(NSMenuItem.separator())
for theme in themes {
let item = NSMenuItem(title: theme, action: #selector(EditViewController.debugSetTheme(_:)), keyEquivalent: "")
item.state = NSControl.StateValue(rawValue: (theme == currentlyActive) ? 1 : 0)
Expand Down
23 changes: 23 additions & 0 deletions XiEditor/Theme.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,29 @@ extension Theme {
shadow: nil
)
}

static func systemTheme() -> Theme {
if #available(OSX 10.13, *) {
return Theme(foreground: NSColor.textColor,
background: NSColor.textBackgroundColor,
caret: NSColor.red, // Seems to be ignored
lineHighlight: NSColor.green, // Seems to be ignored
findHighlight: NSColor.blue, // Seems to be ignored
findHighlightForeground: NSColor.purple, // Seems to be ignored
gutter: NSColor.textBackgroundColor,
gutterForeground: NSColor.secondaryLabelColor,
selection: NSColor.selectedTextBackgroundColor,
selectionForeground: NSColor.systemPink, // Seems to be ignored
selectionBorder: NSColor.yellow, // Seems to be ignored
inactiveSelection: NSColor.brown, // Seems to be ignored
inactiveSelectionForeground: NSColor.cyan, // Seems to be ignored
shadow: NSColor.magenta // Seems to be ignored
)
} else {
// Fallback on earlier versions
return defaultTheme()
}
}

init(jsonObject dict: [String: AnyObject]) {
let foreground = NSColor(jsonRgbaColor: dict["foreground"] as? [String: AnyObject] ?? [:])
Expand Down