d12frosted / homebrew-emacs-plus

Emacs Plus formulae for the Homebrew package manager
MIT License
2.35k stars 181 forks source link

Natural title bar has bad behavior in emacs-28 #369

Closed quarkquartet closed 3 years ago

quarkquartet commented 3 years ago

I installed emacs-28 in my laptop on MacOS Big Sur. But the title bar seems to be different from before:

image

Look at the title bar: the font is dark even on a dark theme, which is not the case for emacs-27.2. And the font is not at the middle of title-bar.

I'm using doom-emacs configuration. I'm not sure whether this is a problem of doom or emacs-plus. But everything is OK on emacs-27.

Any hint about this?

d12frosted commented 3 years ago

Could you please share repro steps? Also would be really helpful to:

  1. Test if this happens when you start 'clean' Emacs via emacs -Q.
  2. Test if that happens with brew cask install emacs (or whatever command brew changed to).

The problem may be in Doom Emacs, emacs-plus, your configs or Emacs itself. Let's narrow the source :)

quarkquartet commented 3 years ago

Hello.

d12frosted commented 3 years ago

I tried brew install --cask emacs, which gives a emacs-27.2. The behavior is what I expect:

Is there a way to install Emacs 28 via cask?

quarkquartet commented 3 years ago

I tried brew install --cask emacs, which gives a emacs-27.2. The behavior is what I expect:

Is there a way to install Emacs 28 via cask?

I do not think so. brew install --cask emacs@28 does not work. If google for brew cask emacs, the website shows the current release is 27.2

elken commented 3 years ago

Does the following snippet help?

(add-to-list 'frameset-filter-alist '(ns-transparent-titlebar . t))
(add-to-list 'frameset-filter-alist '(ns-appearance . dark))

Or this package?

quarkquartet commented 3 years ago

Does the following snippet help?

(add-to-list 'frameset-filter-alist '(ns-transparent-titlebar . t))
(add-to-list 'frameset-filter-alist '(ns-appearance . dark))

Or this package?

I tried both. None of them works. Sorry.

thinkiny commented 3 years ago

same thing happend here

thinkiny commented 3 years ago

i found a solution: change the system-appearance.patch:89

FRAME_NS_APPEARANCE (f) = ns_appearance_dark_aqua;

to

FRAME_NS_APPEARANCE (f) = ns_appearance_vibrant_dark;

result:

Screen Shot 2021-08-25 at 2 08 07 PM
quarkquartet commented 3 years ago

i found a solution: change the system-appearance.patch:89

FRAME_NS_APPEARANCE (f) = ns_appearance_dark_aqua;

to

FRAME_NS_APPEARANCE (f) = ns_appearance_vibrant_dark;

result:

Screen Shot 2021-08-25 at 2 08 07 PM

Where should I write this code?

elken commented 3 years ago

Where should I write this code?

 change the system-appearance.patch:89

d12frosted commented 3 years ago

@ngquerol I see you are using different variants of dark depending on the system version. Could you please chime in, I think your input would be valuable here 😸 I don't remember why we have this conditional value here 😅

https://github.com/d12frosted/homebrew-emacs-plus/blob/eeba9c0a55768db6ad78ae839f3507b9781b63cd/patches/emacs-28/system-appearance.patch#L86-L94

elken commented 3 years ago

ns_appearance_dark_aqua was introduced in macos 10.14 and is the "standard" dark mode appearance.

A more preferable option would be to put the value behind an opt-out --flag as it seems to resolve for some users but not others. (no issue for me) image

elken commented 3 years ago

image

Above behaviour is light theme on global macos dark mode. Looks like the feature check only accounts for the global macos setting, not emacs itself.

EDIT: https://developer.apple.com/documentation/appkit/nsview/1483793-allowsvibrancy related flag to set?

ngquerol commented 3 years ago

Hello all!

First, apologies for the delay, just came back from vacation 😄

@d12frosted: @elken is correct, the conditional is here to use the appropriate NSAppearance depending on we are running on macOS 10.14+ or not (i.e. ns_appearance_dark_aqua if that is the case, ns_appearance_vibrant_dark otherwise).

I cannot reproduce the bug on my side (and I am unfortunately running macOS Monterey). That said, @elken & @quarkquartet could you please share what does (frame-parameters) returns when evaluating it while the bug is visible? Namely, is ns-transparent-titlebar mentioned anywhere?

elken commented 3 years ago

First, apologies for the delay, just came back from vacation

Everyone is allowed time off ;)

Namely, is ns-transparent-titlebar mentioned anywhere?

image

It's set to t on both light and dark on my end, but only light gives the bug (vice versa if I change global macos theme to be light)

MacOS Dark MacOS Light
Emacs Dark image image
Emacs Light image image
ngquerol commented 3 years ago

Does changing the ns-appearance frame parameter have any impact on your end? It should update Emacs' window appearance w/ regards to the argument, i.e. 'dark or 'light. It does nothing on my end, but the good news is that I know how to fix it.

In any case, it will be up to users to set proper values of the ns-appearance & ns-transparent-titlebar frame parameters to ensure the window's title stays readable; For example, if the titlebar is set to be transparent, and the current theme is light, ns-appearance should be set to 'light, and vice versa.

elken commented 3 years ago

No change on my end either, that makes sense though.

ngquerol commented 3 years ago

Great, I'll have a stab at implementing a fix tonight if possible.

ngquerol commented 3 years ago

Alright, the real bug here was that updating the ns-appearance frame parameter did not update Emacs' window appearance. That was probably due to an incorrect rebase of my changes wrt to upstream's master branch 😬

The updated patch is available here.

@elken, @quarkquartet & @d12frosted please test it if possible, and if everything seems good I'll make a PR to update the patch in this repo.

elken commented 3 years ago

@ngquerol Patch seems to resolve it on my end :)

Although a side effect on my end it seems to be forcing menu-bar-mode image

Have since removed the patch and gone back to normal, but I can indeed confirm it fixes the colour issue.

@quarkquartet @thinkiny and I think @shaunsingh was also having this issue.

ngquerol commented 3 years ago

@elken Looking at your screenshot, it seems that it is rather ´tool-bar-mode´ that is enabled?

elken commented 3 years ago

@elken Looking at your screenshot, it seems that it is rather ´tool-bar-mode´ that is enabled?

Sorry yes, I get them mixed up. It's definitely disabled in config, reverting back to the previous patch it's disabled again.

thinkiny commented 3 years ago

I tried this patch, LGTM

Alright, the real bug here was that updating the ns-appearance frame parameter did not update Emacs' window appearance. That was probably due to an incorrect rebase of my changes wrt to upstream's master branch 😬

The updated patch is available here.

@elken, @quarkquartet & @d12frosted please test it if possible, and if everything seems good I'll make a PR to update the patch in this repo.

ngquerol commented 3 years ago

@thinkiny I take it you did not encounter the same issue as @elken regarding the toolbar?

shaunsingh commented 3 years ago

... and I think @shaunsingh was also having this issue.

yup, patch fixes it for me. I do have the same issue as @elken with toolbar-mode being forced though. I have it disabled in my config (and I believe doom should do it, by default)

ngquerol commented 3 years ago

Okay, I'll try to reproduce this behavior with or without Doom. I don't really know what's happening here as the patch does not touch any code related to the toolbar.

quarkquartet commented 3 years ago

same as @shaunsingh

ngquerol commented 3 years ago

Alright, did some testing and it definitely seems Doom-related.

I did not see the "bug" because I do not use this distribution; My configuration disables the toolbar by doing the following in early-init.el:

;; Disable most GUI widgets early on
(setq default-frame-alist '((horizontal-scroll-bars . nil)
                            (vertical-scroll-bars . nil)
                            (menu-bar-lines . 0)
                            (tool-bar-lines . 0) ;; <----- here
                            (internal-border-width . 0)
                            (height . 50)
                            (width . 95)))

While Doom does the following in core/core-ui.el, which is loaded from early-init.el:

(push '(menu-bar-lines . 0)   default-frame-alist)
(push '(tool-bar-lines . 0)   default-frame-alist) ;; <----- here
(push '(vertical-scroll-bars) default-frame-alist)

The code above can be found here in Doom's repository.

Copying that very snippet directly into Doom's early-init.el results in the desired behavior, i.e. no toolbar upon startup. It seems core/core-ui.el is loaded a little too late in the init process, which could explain the fact that if you do C-x 5 2 to open a new frame, this one will not have a toolbar, as opposed to the initial frame created upon startup.

I don't really know how to proceed further, and if the patch is really to blame for that behavior. Maybe @hlissner has an idea?

shaunsingh commented 3 years ago

I believe he mentioned a while back that he wants to move more things to early-init.el or something of that sort, so I assume it'll fix itself eventually?

thinkiny commented 3 years ago

@thinkiny I take it you did not encounter the same issue as @elken regarding the toolbar?

sorry, i turn toolbar off by default

elken commented 3 years ago

@ngquerol If this is the case then I personally consider the patch to resolve the issue so if @d12frosted is happy I'd say PR and well done for fixing it :)