Skip to content

Add automatic theme toggle feature#2224

Draft
koalazub wants to merge 1 commit into
blinksh:rawfrom
koalazub:feature/auto-theme-toggle
Draft

Add automatic theme toggle feature#2224
koalazub wants to merge 1 commit into
blinksh:rawfrom
koalazub:feature/auto-theme-toggle

Conversation

@koalazub

@koalazub koalazub commented Mar 23, 2026

Copy link
Copy Markdown
Dark Light
Simulator Screenshot - iPhone 17 Pro - 2026-03-23 at 15 23 26 Simulator Screenshot - iPhone 17 Pro - 2026-03-23 at 15 23 21
Screen.Recording.2026-03-23.at.3.23.39.pm.mov

This pull request implements automatic theme toggling for Blink Shell, addressing issue #752.

This introduces a System option in the themes list that automatically switches between light and dark themes based on the device's appearance settings. When selected, two additional rows appear for configuring which specific themes to use in light and dark modes.

This PR also introduces a centralised notification system for theme changes through a new applyCurrentTheme method in BLKDefaults. This decouples the settings UI from view updates by broadcasting a BKAppearanceChanged notification that TermController and other components observe.

Background colour retrieval was added to keep the terminal chrome consistent with the theme. Since Blink uses hterm (a JavaScript terminal emulator), themes are applied via JavaScript. This PR executes the theme script and retrieves the background colour value directly from the terminal via JavaScript evaluation, then applies it natively to the surrounding view.

Keyboard appearance now respects the system's interface style when automatic toggling is enabled, ensuring the keyboard matches the active theme. Unit tests verify the feature correctly stores preferences and determines the appropriate theme based on system appearance.

@koalazub koalazub marked this pull request as ready for review March 23, 2026 04:51
Copilot AI review requested due to automatic review settings March 23, 2026 04:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Implements an automatic “System” theme option that switches between configured light/dark themes based on the device appearance, and introduces a centralized theme-change notification flow intended to decouple settings UI from terminal view updates.

Changes:

  • Adds auto theme toggle state + light/dark theme selections to BLKDefaults, plus helpers to derive/apply the current theme.
  • Updates the Appearance settings UI to expose a “System” option and light/dark theme pickers.
  • Hooks theme-change notifications into TermController/SpaceController, and extends TermView to retrieve/apply the terminal background color after theme application.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
Settings/ViewControllers/Appearance/BKAppearanceViewController.m Adds “System” theme row and light/dark theme selection UI; triggers theme application notifications.
Settings/Model/BLKDefaults.m Persists auto-toggle + light/dark theme names; adds helpers for resolving/applying theme based on appearance.
Settings/Model/BLKDefaults.h Exposes new defaults properties and APIs for auto theme toggle.
BlinkTests/AutoThemeToggleTests.swift Adds unit tests for new defaults + notification posting behavior.
Blink/TermView.m Applies theme via JS and reads background color to sync native chrome.
Blink/TermController.swift Uses resolved theme name and observes theme-change notifications to update the terminal.
Blink/SpaceController.swift Makes keyboard appearance follow system style when auto theme toggle is enabled.
Comments suppressed due to low confidence (1)

Settings/ViewControllers/Appearance/BKAppearanceViewController.m:220

  • saveDefaultValues still posts BKAppearanceChanged with object:self, while the new applyCurrentTheme posts the same notification with object set to the theme name string. This inconsistent payload makes it easy for observers to mis-handle the notification. Consider standardizing the notification contract (e.g., always send the theme name via object or userInfo) and have saveDefaultValues call +[BLKDefaults applyCurrentTheme] instead of posting directly.
  [BLKDefaults saveDefaults];
  [[NSNotificationCenter defaultCenter]
    postNotificationName:BKAppearanceChanged
                  object:self];
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Settings/ViewControllers/Appearance/BKAppearanceViewController.m Outdated
Comment thread Settings/ViewControllers/Appearance/BKAppearanceViewController.m Outdated
Comment thread Blink/TermController.swift Outdated
Comment thread Settings/ViewControllers/Appearance/BKAppearanceViewController.m Outdated
Comment thread Settings/ViewControllers/Appearance/BKAppearanceViewController.m Outdated
Comment thread Settings/ViewControllers/Appearance/BKAppearanceViewController.m Outdated
Comment thread Settings/Model/BLKDefaults.m Outdated
Comment thread Settings/Model/BLKDefaults.m Outdated
Comment thread Settings/ViewControllers/Appearance/BKAppearanceViewController.m

@koalazub koalazub left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed all Copilot review comments.

Fixed 8 issues:

  • Light/Dark theme rows: Added bounds checking for index paths
  • Cell detail text: Uses Default cell style, now includes theme name in textLabel
  • TermController reload: Removed WKWebView reload to prevent flicker/state loss
  • Row count: Corrected to 1 (System) + themeCount + 1 (Add) + optional 2
  • Selected theme nil check: Added guard before accessing .row
  • iPad action sheet: Added popover configuration
  • saveDefaults: Added nil path guard to match loadDefaults
  • cellForRowAtIndexPath: Removed incorrect access since section reloads

Acknowledged 2 design considerations that require larger architectural changes.

@carloscabanero

carloscabanero commented Mar 23, 2026

Copy link
Copy Markdown
Member

This shouldn't be this hard. The terminal already supports ligh/dark themes, we just never shipped any. This adds a lot of cruft, and forgets about actually shipping the current variants for the default themes on light/dark. Then the light/dark switch is on Themes itself? It would make more sense as its own row, in case the user has any preferences. Then other things need to change, like keyboard, etc... And from the demo video, I do not think it actually works.

@carloscabanero

Copy link
Copy Markdown
Member

Also, if you we are going to push a branch full of "AI" commits, let's make sure they do not change other project configuration settings. That "revert TEAM_ID" should not be a commit there. It is a big smell that makes me not trust anything else. Clean up the tree, and give a single commit.

@koalazub

Copy link
Copy Markdown
Author

Also, if you we are going to push a branch full of "AI" commits, let's make sure they do not change other project configuration settings. That "revert TEAM_ID" should not be a commit there. It is a big smell that makes me not trust anything else. Clean up the tree, and give a single commit.

Yeah fair enough. I did revert BECAUSE I spotted that Xcode made some changes in the pbxproj and I did presume it was going to be squashed and rebased

@koalazub

Copy link
Copy Markdown
Author

This shouldn't be this hard. The terminal already supports ligh/dark themes, we just never shipped any. This adds a lot of cruft, and forgets about actually shipping the current variants for the default themes on light/dark. Then the light/dark switch is on Themes itself? It would make more sense as its own row, in case the user has any preferences. Then other things need to change, like keyboard, etc... And from the demo video, I do not think it actually works.

That makes more sense. I just wanted the ability to set themes, but I can see that the designs aren't exactly appealing. As for the keyboard, what do you mean specifically? Because there is consideration for the hotkey(I'm not sure if you mean that keyboard?) keyboard. The keyboard is respecting the themes when toggled. Mind giving me a bit more specifics here on what you mean?

@koalazub koalazub force-pushed the feature/auto-theme-toggle branch from fa88935 to 6c5f8ac Compare March 23, 2026 23:34
@koalazub koalazub marked this pull request as draft March 24, 2026 00:13
@koalazub

Copy link
Copy Markdown
Author

@carloscabanero Regarding your point about theme structure, are you ok with having the modal for selecting themes? Or would you prefer your current pattern of going into a new page with the options. I'm assuming you prefer the consistency so I'm happy to swap it out for that. This is what I'm talking about:
Simulator Screenshot - iPhone 17 Pro - 2026-03-24 at 11 12 58

Here are the changes I've made to address your point about clear demarcation of themes:

image

@koalazub koalazub force-pushed the feature/auto-theme-toggle branch 3 times, most recently from a26f7bf to fc15c08 Compare March 24, 2026 00:57
Introduce a dedicated Appearance section with a Follow System toggle
that automatically switches between configured light and dark themes
based on device appearance. The Themes list remains a clean selection
of available themes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@koalazub koalazub force-pushed the feature/auto-theme-toggle branch from fc15c08 to 435c4c6 Compare March 24, 2026 01:00
@carloscabanero

Copy link
Copy Markdown
Member

Hey, first of all, thanks a lot for trying to improve this and sorry for not saying it in my previous message. I just came back from a conference and I will need a moment to think about this. Let me get back to you later today.

@koalazub

Copy link
Copy Markdown
Author

Honestly don't sweat it at all. I understood your perspective re AI commits. I should've done more due diligence before just throwing out this PR. Take your time

@carloscabanero

Copy link
Copy Markdown
Member

So my original version was actually using Hterm's themes directly to change between light and dark. It is actually built in, so there was no interface necessary other than just shipping the themes, etc... But, I like your idea here of just allowing to set the light and dark themes manually, and then having the app change them. It shouldn't be too hard to just add the two fields within Appearance, and then let the theme selection happen within its own child View.

The thing is, the code here is really old, and I actually have a new interface for this that I did not have time to push: separating Appearance into Display and Style, new models, etc... I think it would be a waste of time for both you and I to make this temporary change here when we will soon change everything.

So here is my proposal, how about you give me this week so I can merge my changes into the main branch, and then you can apply this on top. I would still love for you to ship this within Blink. How does that sound?

PS: What LLM did you use for this? There are a few weird things in the code, like adding a new field at the beginning of the enum (that's a bug), etc...

@koalazub

Copy link
Copy Markdown
Author

So my original version was actually using Hterm's themes directly to change between light and dark. It is actually built in, so there was no interface necessary other than just shipping the themes, etc... But, I like your idea here of just allowing to set the light and dark themes manually, and then having the app change them. It shouldn't be too hard to just add the two fields within Appearance, and then let the theme selection happen within its own child View.

The thing is, the code here is really old, and I actually have a new interface for this that I did not have time to push: separating Appearance into Display and Style, new models, etc... I think it would be a waste of time for both you and I to make this temporary change here when we will soon change everything.

So here is my proposal, how about you give me this week so I can merge my changes into the main branch, and then you can apply this on top. I would still love for you to ship this within Blink. How does that sound?

PS: What LLM did you use for this? There are a few weird things in the code, like adding a new field at the beginning of the enum (that's a bug), etc...

Totally happy to wait until next week to rework this.

And good eye on the weird behaviour for the LLM. I forgot that I had OpenCode Go with Kimi K2.5. it was a recent subscription that I was testing out. When I refactored the code based on your suggestions(when I realised I had habitually opened opencode instead of Claude Code) that was Opus 4.6. I think what happened is that Opus just went with the changes already present and didn't run the tooling to verify that the code was correct instead of a top to bottom change. My hunch - from my experiences with nushell specifically - is that it doesn't run type checking or parsing for some types of code and in this case it might be that Obj C was just approximated to look correct.

I'll do a better job of enforcing it parses the code first before I commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants