-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add option to apply opacity to all background colors #5297
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
Add option to apply opacity to all background colors #5297
Conversation
Is this basically a rewrite due to the changes made to the codebase since the original PR? Looks like it doesn't have a ton in common. |
45ca0c9
to
5df19b4
Compare
Yeah, it's just a rewrite, since the codebase was changed in a way that |
I wonder whether we should move |
I think this has been discussed in the past and I agree that |
5df19b4
to
c158918
Compare
I've moved background_opacity to window, should be good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good to go tbh. If you have the time let me know what you think about the remaining feedback. I'm happy to help out with resolving these issues.
alacritty/src/display/mod.rs
Outdated
let mut grid_cells = { | ||
let cols = self.size_info.columns(); | ||
let lines = self.size_info.screen_lines(); | ||
Vec::with_capacity(cols * lines) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an optimization unrelated to this PR? I've benchmarked this particular change myself previously and it did not have any impact on performance. Have you made any other observations or was this just an unrelated test you were performing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wrote that way, since it's always better not to do reallocs if you know upfront the size of allocation. It could show no difference for you, since your system if fast or your allocator is performing well, but you couldn't know how it'll work out with 'not smart enough' allocator or highly loaded system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could show no difference for you, since your system if fast or your allocator is performing well, but you couldn't know how it'll work out with 'not smart enough' allocator or highly loaded system.
That's a fair point, it's probably not going to be worse. Though this will definitely cause a bigger allocation than necessary since often the screen is likely not entirely full. Since this happens a bunch ideally the allocator won't have to ask the system anyway though.
Either way not sure if I'd sneak it into an unrelated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't want to change this in this particular PR. I don't think it's uncontroversial enough to just slip it in.
c158918
to
b04c242
Compare
b04c242
to
f9ba1f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good assuming that all my suggestions are fine with you.
alacritty/src/display/mod.rs
Outdated
let mut grid_cells = { | ||
let cols = self.size_info.columns(); | ||
let lines = self.size_info.screen_lines(); | ||
Vec::with_capacity(cols * lines) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't want to change this in this particular PR. I don't think it's uncontroversial enough to just slip it in.
58ecc96
to
c635ad1
Compare
alacritty/src/display/mod.rs
Outdated
@@ -490,6 +490,7 @@ impl Display { | |||
for cell in &mut content { | |||
grid_cells.push(cell); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like it without this newline, since it keeps all the content.
stuff together. Not that big of a deal though.
In some cases it could be desired to apply 'background_opacity' to all background colors instead of just 'colors.primary.background', thus adding an 'colors.opaque_background_colors' option to control that. Fixes alacritty#741.
da8f37c
to
7035322
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In some cases it could be desired to apply 'background_opacity'
to all background colors instead of just 'colors.primary.background',
thus adding an 'colors.opaque_background_colors' option to control that.
Fixes #741.
--
Based on #4196.