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.

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

Support Dark Mode #449

Closed
wants to merge 31 commits into from
Closed

Support Dark Mode #449

wants to merge 31 commits into from

Conversation

LEOYoon-Tsaw
Copy link
Member

This is built upon #448
Introduce a SquirrelLayout class to store all layout properties, this makes switching between dark and light modes easy, and ready for any future additions.

@lotem
Copy link
Member

lotem commented Sep 2, 2020

#250#273 有好的解決辦法了嗎?

@LEOYoon-Tsaw
Copy link
Member Author

#250#273 有好的解決辦法了嗎?

用戶只要不寫color_scheme_dark,就不會遇到黑邊。黑色模式下的背景色應該用深色,如此,則黑邊不是問題。
這個黑邊是加在NSWindow上的,要去除比較麻煩,比如把NSWindow的appearance固定爲aqua,但讓SquirrelView直接從NSApp監聽appearance變化,但似無必要。
只有在用戶用了color_scheme_dark,且爲之設了一個淺色背景才會看到黑邊,我認爲這種情況屬於誤用color_scheme_dark

@LEOYoon-Tsaw
Copy link
Member Author

microsoft/vscode#62414
例如VSCode團隊認爲這是macOS的intended behavier

@LEOYoon-Tsaw
Copy link
Member Author

這個Fix會使得native配色不能根據黑白模式自動調整,你來判斷要不要用吧

@lotem
Copy link
Member

lotem commented Sep 8, 2020

這個Fix會使得native配色不能根據黑白模式自動調整,你來判斷要不要用吧

多謝。這是很重要的進展。
起初嘗試適配系統黑白模式的時候,發現native配色會在黑白模式更替時出現界面局部顏色未更新的情況,沒準是由於一些變化前的系統顏色值保存在了文字屬性裏,我需要梳理一遍代碼結構,解決這個問題。
拓展設置窗體外觀屬性的思路,可以嘗試在配色方案定義裏用一個可選的參數指定想要的窗體外觀。
這些我有時間了再試着做做。

@LEOYoon-Tsaw
Copy link
Member Author

似乎NSwindow的外觀是白的話沒有任何邊框,為黑才有,border_color已經可以達到一樣的效果,似乎沒必要

@lotem
Copy link
Member

lotem commented Sep 8, 2020

似乎NSwindow的外觀是白的話沒有任何邊框,為黑才有,border_color已經可以達到一樣的效果,似乎沒必要

原來如此。那麼如果今後想要實現native自動適應,只需要在上一筆提交的代碼裏排除native配色就可以了吧?

@LEOYoon-Tsaw
Copy link
Member Author

似乎NSwindow的外觀是白的話沒有任何邊框,為黑才有,border_color已經可以達到一樣的效果,似乎沒必要

原來如此。那麼如果今後想要實現native自動適應,只需要在上一筆提交的代碼裏排除native配色就可以了吧?

是的,只要爲 _window.appearance = .aqua 加上條件即可

@LEOYoon-Tsaw
Copy link
Member Author

#250#273 都解決了

@LEOYoon-Tsaw
Copy link
Member Author

還有啥問題嗎?

@xiehuc
Copy link

xiehuc commented Nov 16, 2020

cool

@LEOYoon-Tsaw
Copy link
Member Author

Universal App都支持了,為何3年前的Dark Mode還不支持?

@LEOYoon-Tsaw LEOYoon-Tsaw force-pushed the dark_mode branch 3 times, most recently from c2923e2 to a8afe95 Compare January 19, 2021 00:05
@david50407
Copy link

支持這個 PR,我在日常會使用自動模式來切換深色模式,輸入法沒辦法統一的話其實有點傷眼

setting both color_scheme and color_scheme_dark to "native" to enable auto switching native color
if color_scheme_dark not set, will stay light mode
user may set only one of color_scheme and color_scheme_dark to "native" while keeping the other a coded color scheme
setting both color_scheme and color_scheme_dark to "native" to enable auto switching native color
if color_scheme_dark not set, will stay light mode
user may set only one of color_scheme and color_scheme_dark to "native" while keeping the other a coded color scheme
setting both color_scheme and color_scheme_dark to "native" to enable auto switching native color
if color_scheme_dark not set, will stay light mode
user may set only one of color_scheme and color_scheme_dark to "native" while keeping the other a coded color scheme
setting both color_scheme and color_scheme_dark to "native" to enable auto switching native color
if color_scheme_dark not set, will stay light mode
user may set only one of color_scheme and color_scheme_dark to "native" while keeping the other a coded color scheme
@lotem
Copy link
Member

lotem commented Jan 30, 2021

This PR is too large and includes commits seemingly unrelated to the feature. It would be easier to review and merge if the branch only includes this single feature rather than everything since the introduction of the feature.

@LEOYoon-Tsaw
Copy link
Member Author

LEOYoon-Tsaw commented Jan 30, 2021

This is because you didn't merge it promptly. It’s been 6 months, during which I have to rebase the code again and again

@lotem
Copy link
Member

lotem commented Jan 30, 2021

This is because you didn't merge it promptly

Not necessarily if this was a stable feature branch. That is the point I wanted to convey.
I'm sorry for not having processed the PR quickly, but in OSS projects you cannot always expect your PR be merged in time so that you can keep going with a single branch for follow up features. That's where branches are useful in making features easy to work with in the git collaboration model.

@LEOYoon-Tsaw
Copy link
Member Author

LEOYoon-Tsaw commented Jan 30, 2021

This is because you didn't merge it promptly

Not necessarily if this was a stable feature branch. That is the point I wanted to convey.
I'm sorry for not having processed the PR quickly, but in OSS projects you cannot always expect your PR be merged in time so that you can keep going with a single branch for follow up features. That's where branches are useful in making features easy to work with in the git collaboration model.

This branch only includes dark mode code, except for a1884c7 and 710a409 which optimize the gap between the first candidate and preedit area. These 2 last commits aren’t big enough to justify a seperate branch

@lotem
Copy link
Member

lotem commented Jan 30, 2021

Looks good. Works perfectly when set both color schemes.
I'm trying to fix the native color scheme which is kind of broken. Hopefully I'll be able to merge the feature soon.

@LEOYoon-Tsaw
Copy link
Member Author

LEOYoon-Tsaw commented Jan 30, 2021

Looks good. Works perfectly when set both color schemes.
I'm trying to fix the native color scheme which is kind of broken. Hopefully I'll be able to merge the feature soon.

I tested native scheme during developing this pull, should be working
Need setting color_scheme_dark to native to enable auto switching

@lotem
Copy link
Member

lotem commented Jan 30, 2021

Looks good. Works perfectly when set both color schemes.
I'm trying to fix the native color scheme which is kind of broken. Hopefully I'll be able to merge the feature soon.

I tested native scheme during developing this pull, should be working

I already fixed the color_theme_dark case. Did some code refactor too. Please stay tuned and don't introduce more changes.

The only visible issue is with blendColors: after a system appearance change, native theme gets incorrect label colors, because blendColors was called when loading config, and the blended color values were resolved under a different appearance. Any ideas on how to solve the issue?

Screen Shot 2021-01-30 at 23 33 22

One possible solution is to get the correct color value for semantic colors used, for the target light/dark mode.
At the worst, we cannot use blended colors for native color scheme.

@LEOYoon-Tsaw
Copy link
Member Author

LEOYoon-Tsaw commented Jan 30, 2021

I think we shouldn’t use blend color with semantic color, or if insist, the possible way is to wrap the output of blend color also a sementia color object

@lotem
Copy link
Member

lotem commented Jan 30, 2021

It's almost impossible to get the perfect blended color for native color scheme, unless color is calculated in real time.
The label color for hightlighted candidate rely on not only the correct foreground color with light/dark theme, but also the accent color in hightlighted background.

I shall give them the same semantic grey color. Lose a bit of accent in the hightlighted label. But the overall contrast should be fine.

@LEOYoon-Tsaw
Copy link
Member Author

Yeah, that sounds good enough

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.

None yet

5 participants