Skip to content

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

Merged
merged 6 commits into from
Aug 16, 2021

Conversation

kchibisov
Copy link
Member

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.

@kchibisov kchibisov requested a review from chrisduerr July 2, 2021 23:59
@chrisduerr
Copy link
Member

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.

@kchibisov
Copy link
Member Author

Yeah, it's just a rewrite, since the codebase was changed in a way that colors are now in ui_config, which was the main complain back in the days.

@kchibisov
Copy link
Member Author

I wonder whether we should move background_opacity somewhere else in a config rather than keeping at the root. I think we can move it to window.background_opacity, what do you think of it?

@chrisduerr
Copy link
Member

I wonder whether we should move background_opacity somewhere else in a config rather than keeping at the root. I think we can move it to window.background_opacity, what do you think of it?

I think this has been discussed in the past and I agree that window would be a good place to have this. Alternatively maybe in colors, but I think I prefer window. IIRC there was some reason why we didn't move it in the past, but that might have been resolved when the colors were moved out of alacritty_terminal.

@kchibisov kchibisov force-pushed the opaque-background-colors branch from 5df19b4 to c158918 Compare July 3, 2021 21:08
@kchibisov
Copy link
Member Author

I've moved background_opacity to window, should be good now.

Copy link
Member

@chrisduerr chrisduerr left a 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.

Comment on lines 487 to 491
let mut grid_cells = {
let cols = self.size_info.columns();
let lines = self.size_info.screen_lines();
Vec::with_capacity(cols * lines)
};
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@chrisduerr chrisduerr Aug 4, 2021

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.

Copy link
Member

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.

@chrisduerr chrisduerr force-pushed the opaque-background-colors branch from c158918 to b04c242 Compare July 21, 2021 02:15
@kchibisov kchibisov force-pushed the opaque-background-colors branch from b04c242 to f9ba1f7 Compare August 4, 2021 21:38
@kchibisov kchibisov requested a review from chrisduerr August 4, 2021 21:38
Copy link
Member

@chrisduerr chrisduerr left a 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.

Comment on lines 487 to 491
let mut grid_cells = {
let cols = self.size_info.columns();
let lines = self.size_info.screen_lines();
Vec::with_capacity(cols * lines)
};
Copy link
Member

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.

@kchibisov kchibisov requested a review from chrisduerr August 6, 2021 14:58
@kchibisov kchibisov force-pushed the opaque-background-colors branch 2 times, most recently from 58ecc96 to c635ad1 Compare August 6, 2021 15:41
@@ -490,6 +490,7 @@ impl Display {
for cell in &mut content {
grid_cells.push(cell);
}

Copy link
Member

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.

@kchibisov kchibisov requested a review from chrisduerr August 15, 2021 08:17
@chrisduerr chrisduerr removed their request for review August 15, 2021 22:57
kchibisov and others added 6 commits August 16, 2021 10:47

Verified

This commit was signed with the committer’s verified signature.
chrisduerr Christian Duerr
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.

Verified

This commit was signed with the committer’s verified signature.
chrisduerr Christian Duerr

Verified

This commit was signed with the committer’s verified signature.
chrisduerr Christian Duerr

Verified

This commit was signed with the committer’s verified signature.
chrisduerr Christian Duerr

Verified

This commit was signed with the committer’s verified signature.
chrisduerr Christian Duerr

Verified

This commit was signed with the committer’s verified signature.
chrisduerr Christian Duerr
@chrisduerr chrisduerr force-pushed the opaque-background-colors branch from da8f37c to 7035322 Compare August 16, 2021 08:48
Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

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

LGTM

@kchibisov kchibisov merged commit c24d7df into alacritty:master Aug 16, 2021
@kchibisov kchibisov deleted the opaque-background-colors branch August 16, 2021 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Transparency improvements
2 participants