fix: fixed the issue where the traffic light button on the white thumbnail is difficult to see in dark mode. #3433
Conversation
Allsochen
changed the title
|
It seems it could be done more directly by forcing the appearance of |
Thank you for the reminder. I re-implemented it on TrafficLightButton, but encountered an issue: adding a background to TrafficLightButton didn't work. Ultimately, changing the inheritance of TrafficLightButton to NSView solved the problem. |
|
I'm not sure I follow you, but I was thinking of something like this: class TrafficLightButton: NSButton {
[...]
init(_ type: TrafficLightButtonType, _ tooltip: String, _ size: CGFloat) {
[...]
appearance = .init(named: .aqua)
}
[...]
} |
I don't know how to fix it using |
src/logic/Preferences.swift
Outdated
| static var interCellPadding: CGFloat { 15 } | ||
| static var intraCellPadding: CGFloat { 8 } |
There was a problem hiding this comment.
I'm not sure to understand this change. Is it related to the goal of the MR?
There was a problem hiding this comment.
This fix is unrelated to the changes here. The modification was made because many AltTab users around me have reported that the spacing between thumbnails is too small, The spacing is smaller than the native spacing on both macOS and Windows. so I changed it to the thumbnail spacing of Windows 11. If you disagree, we can leave this part unchanged for now and consider revising it later as a separate advanced configuration option.
There was a problem hiding this comment.
Changed the thumbnail spacing equals to Windows 11 spacing.
There was a problem hiding this comment.
I think changes shouldn't be bundled. It can create confusion while reviewing MRs. If you propose to make multiple unrelated changes, I suggest opening multiple MRs, each with their goal. They can be reviewed with more clarity.
Furthermore, we use a convention on commits. You can learn a bit more here. One commit = 1 change. Here there should be a fix: for the traffic light opacity issue, since it's indeed a bug fix. Then there should be a feat: to update the spacings, since it's not a bug fix bug an improvement. Finally, for documentation, one could use docs: or refactor: for the commit which documents the existing code.
Regarding the change of spacing: I think Windows 11 Spacings should live in a Windows 11 theme. We already support Themes in Preferences > Appearance. Let's add a Windows 11 theme. Other people were asking about it I believe.
There was a problem hiding this comment.
In my usual development work, I also follow these MR standards. Moving forward, I will adhere to the conventions set forth here.
I agree with your point. However, currently only Windows has thumbnails, while macOS and Linux do not. Therefore, I think it's best to set the spacing to be the same as Windows when thumbnails are available.
|
Hi everyone, Indeed, I think this MR is quite large, for its intended goal. I was able to reproduce the issue by switching to Dark Mode. Then I applied the fix suggested by @decodism, and it fixes the issue for me. One line of code seems to fix the issue: 
|
This MR not only fixes the issue with the transparency of the traffic lights button but also does the following:
I have thoroughly tested these features and have been running them on my computer for quite some time, ensuring they are correct to the best of my abilities. If you think these changes are difficult to understand within a single MR, I can split them into multiple MRs, although this will increase my workload. |
| } | ||
|
|
||
| /// Mouse exited event handler. |
There was a problem hiding this comment.
I don't think these comments are the way to go. When we see a method whith override, we can use our IDE to display the original method's documentation, very easily. For instance, a simple hover with the mouse in AppCode shows me what the original method is about:
| let disk = NSBezierPath() | ||
| disk.appendOval(in: NSMakeRect(bounds.origin.x + 0.5, bounds.origin.y + 0.5, bounds.width - 1, bounds.height - 1)) | ||
|
|
||
| // Fill the disk with the black background color | ||
| NSColor.black.setFill() | ||
| disk.fill() | ||
|
|
||
| // Fill the disk with the background color | ||
| diskBackgroundColor.draw(in: disk, relativeCenterPosition: .zero) |
There was a problem hiding this comment.
It seems you added drawing a black disk here. What issue is it fixing?
There was a problem hiding this comment.
This is the key to fixing the issue of transparent button in the dark theme. The black disk serves as a background to prevent the buttons from being transparent.
| NSColor.black.withAlphaComponent(0.5).setFill() | ||
| disk.fill() | ||
| } else if (isMouseOver) { | ||
| if isMouseOver { |
There was a problem hiding this comment.
You removed the difference of being highlighted = 50% opacity, and being hovered by the mouse = 25% opacity. Now the mouse hover is 25%, and the highlight is gone; it's always 100%. I'm not sure why this change is made. How is it related to the stated goal of the MR?
There was a problem hiding this comment.
The button is currently an NSView, and when it needs to be displayed, it should show its color at 100% opacity. When the mouse hovers over it, it should display at 25% opacity. The NSButton has an isHighlighted state, but this state is no longer needed with the current setup.
| target = self | ||
| action = #selector(onClick) | ||
|
|
||
| // Adjust the button size to fit the specified dimensions | ||
| fit(size, size) | ||
| // Add a tracking area to detect mouse enter and exit events | ||
| addTrackingArea(NSTrackingArea(rect: bounds, options: [.mouseEnteredAndExited, .activeInKeyWindow], owner: self, userInfo: nil)) | ||
| toolTip = tooltip | ||
| wantsLayer = true | ||
| } | ||
|
|
||
| /// Required initializer for decoding the button (not used here). | ||
| required init?(coder: NSCoder) { | ||
| super.init(coder: coder) | ||
| } | ||
|
|
||
| @objc func onClick() { | ||
| if (type == .fullscreen) { | ||
| window_?.toggleFullscreen() | ||
| } else if (type == .miniaturize) { | ||
| window_?.minDemin() | ||
| } else if (type == .close) { | ||
| window_?.close() | ||
| } else if (type == .quit) { | ||
| window_?.application.quit() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It seems that you replaced this with the mouseUp override bellow. It seems to me that this approach here is closer to the intent: we react to clicks. With the approach you used, we react to mouse release, and manually define what a click is. I'm not sure I see it as an improvement. Is there something I'm missing here? An upside of mouseUp I'm not seeing perhaps?
There was a problem hiding this comment.
Thank you for the reminder, this can be changed to mouseDown.
| override func mouseEntered(with event: NSEvent) { | ||
| isMouseOver = true | ||
| setNeedsDisplay() | ||
| setNeedsDisplay(bounds) |
There was a problem hiding this comment.
What is the purpose of this change?
There was a problem hiding this comment.
In terms of the effect, their performance is the same. This class inherits from the NSView class, and through this modification, we can specify a fixed area for redrawing, which will be more precise.
| // Force the icon to repaint | ||
| icon.display() |
There was a problem hiding this comment.
Why is this necessary? What was failing before we added this new call?
There was a problem hiding this comment.
If this modification is not added, an issue arises: after the user clik the fullscreen button to enters full screen, the fullscreen button does not redraw, resulting in the user still seeing the button as if it were in fullscreen mode, rather than the defullscreen button. To make this issue clearer to you, I will open a new MR, so you can see the problem and the solution more clearly.
Allsochen
changed the title
Fixed the issue where the traffic light button on the white thumbnail is difficult to see in dark mode.
Fixed the issues #2892
In dark mode, there are two situations where it is difficult to distinguish:
After fixed: