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

Try to create window with different SRGB config when failed (solve #921, #1178) #1183

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
None yet
4 participants
@lo48576
Copy link
Contributor

lo48576 commented Mar 13, 2018

By this patch, alacritty will try to create window with SRGB enabled at first,
but when creation failed, retry with SRGB disabled.

This may truly solve #921, and issue caused by #1178: #921 (comment).

@chrisduerr
Copy link
Collaborator

chrisduerr left a comment

Should have probably seen this coming, thanks for the follow-up!

.with_vsync(true);
let window = ::glutin::GlWindow::new(window, context, &event_loop)?;
let window = create_gl_window(window.clone(), &event_loop, true)
.or_else(|_| create_gl_window(window.clone(), &event_loop, false))?;

This comment has been minimized.

@chrisduerr

chrisduerr Mar 13, 2018

Collaborator

The second .clone() is not necessary.

.with_srgb(srgb)
.with_vsync(true);
::glutin::GlWindow::new(window, context, event_loop)
}

This comment has been minimized.

@chrisduerr

chrisduerr Mar 13, 2018

Collaborator

I'm not a huge fan of inner functions, I think a normal function would be better in this case and then just use #[inline]. But maybe @jwilm thinks differently about this.

This comment has been minimized.

@jwilm

jwilm Mar 13, 2018

Owner

I would prefer to not have the function definition nested if that's what you're getting at. #[inline] shouldn't be necessary though since this isn't exactly performance sensitive.

This comment has been minimized.

@chrisduerr

chrisduerr Mar 13, 2018

Collaborator

Yeah I was talking about the nesting. And adding #[inline] would probably not make any difference.

@jwilm
Copy link
Owner

jwilm left a comment

Thanks for the follow-up PR! I like the approach.

.with_srgb(srgb)
.with_vsync(true);
::glutin::GlWindow::new(window, context, event_loop)
}

This comment has been minimized.

@jwilm

jwilm Mar 13, 2018

Owner

I would prefer to not have the function definition nested if that's what you're getting at. #[inline] shouldn't be necessary though since this isn't exactly performance sensitive.

.with_srgb(true)
.with_vsync(true);
let window = ::glutin::GlWindow::new(window, context, &event_loop)?;
let window = create_gl_window(window.clone(), &event_loop, true)

This comment has been minimized.

@jwilm

jwilm Mar 13, 2018

Owner

Can we start without the sRGB framebuffer option first?

@fcladera

This comment has been minimized.

Copy link

fcladera commented Mar 13, 2018

This fix seems to work in Arch Linux. Thanks!

Try to create window with different SRGB config when failed
This may truly solve #921 (and issue caused by #1178)
<#921 (comment)>.

@lo48576 lo48576 force-pushed the lo48576:fix/try-srgb-switch branch from 5c066ee to e045139 Mar 13, 2018

@lo48576

This comment has been minimized.

Copy link
Contributor

lo48576 commented Mar 13, 2018

Amended the commit.

  • Stop using nested function (now create_gl_window is at global level).
  • Avoid unnecessary (second) clone.
  • Try .with_srgb(false) first, then .with_srgb(true) second.
@jwilm

jwilm approved these changes Mar 13, 2018

@jwilm

This comment has been minimized.

Copy link
Owner

jwilm commented Mar 13, 2018

perfect; thanks!

@jwilm jwilm merged commit 6debc4f into jwilm:master Mar 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lo48576 lo48576 deleted the lo48576:fix/try-srgb-switch branch Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment