Skip to content
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

Bump glutin to 0.29.0 #6079

Merged
merged 1 commit into from
Aug 10, 2022
Merged

Conversation

kchibisov
Copy link
Member

@kchibisov kchibisov commented May 23, 2022

Fixes #5975.
Fixes #5767.

@kchibisov
Copy link
Member Author

@sshock could you verify that tab issue you've mentioned in winit rust-windowing/winit#2238 gets resolved with this patch? I'm not sure what to look for myself on macOS for that...

@kchibisov kchibisov force-pushed the update-winit-0.27 branch 4 times, most recently from d3bd9a6 to 6b32bca Compare June 12, 2022 19:46
CHANGELOG.md Outdated

### Changed

- The `--help` output was reworked with a new colorful syntax
- OSC 52 is now disabled on unfocused windows
- `SpawnNewInstance` no longer inherits initial `--command`
- Client side decorations should have proper text rendering now on Wayland
- Search bar is now respecting cursor thickness
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this should be fixed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a part of IME commit.

CHANGELOG.md Outdated
- Incorrect built-in glyphs for `U+2567` and `U+2568`
- Cursor not hiding on GNOME Wayland
- Font having different scale factor after monitor powering off/on on X11
- Wrong mouse input when opening a new tabbed window on macOS
Copy link
Member

Choose a reason for hiding this comment

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

Rather than describing the mouse part here, I'd instead mention that the entire window content was offset. It affected rendering too, even with no mouse usage.

alacritty.yml Outdated Show resolved Hide resolved
alacritty.yml Outdated Show resolved Hide resolved
alacritty/build.rs Outdated Show resolved Hide resolved
@sshock
Copy link

sshock commented Jun 13, 2022

@sshock could you verify that tab issue you've mentioned in winit rust-windowing/winit#2238 gets resolved with this patch? I'm not sure what to look for myself on macOS for that...

Yep, it's working!

@kchibisov kchibisov force-pushed the update-winit-0.27 branch 3 times, most recently from df75b81 to 15a9dc7 Compare July 12, 2022 15:14
@kchibisov kchibisov marked this pull request as ready for review July 27, 2022 00:37
@kchibisov kchibisov changed the title [WIP] Bump winit to 0.27.0 Bump glutin to 0.29.0 Jul 27, 2022
@kchibisov
Copy link
Member Author

kchibisov commented Jul 27, 2022

@chrisduerr I've removed IME commit for now and implemented it like we had before. Will rewrite it and send separately.

Note, that I have bumped msrv to 1.61 for now. Probably will sort that stuff out tomorrow, since it'll require a winit bump to downgrade it back to 1.57.

MSRV issue got resolved.

alacritty/Cargo.toml Outdated Show resolved Hide resolved
@kchibisov kchibisov force-pushed the update-winit-0.27 branch 2 times, most recently from 15d0236 to 190ba88 Compare July 30, 2022 21:37
alacritty/src/event.rs Outdated Show resolved Hide resolved
alacritty/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 93 to 99
let exit_code = match result {
Ok(code) => code,
Err(err) => {
eprintln!("Error: {}", err);
EXIT_FAILURE
},
};
Copy link
Member

Choose a reason for hiding this comment

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

Why bother with exit codes and all that stuff when Err(err) is mapped to an exit code anyway? Just so we don't print the error?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was to propagate the error code from the event loop somehow, since now run_return will return non zero code in case of failures.

I basically wanted to indicate an error with non zero exit status nothing more. If you have an idea on how to handle that more reliably let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Why not return the actual error? That seems to be the sensible choice to me.

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.

See my comment about returning a Result.

@@ -62,7 +62,7 @@ use crate::event::{Event, Processor};
#[cfg(target_os = "macos")]
use crate::macos::locale;

fn main() {
fn main() -> Result<(), Box<dyn Error>> {
Copy link
Member

Choose a reason for hiding this comment

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

I've only ever gotten extremely poor results from doing this. I don't think it's a great idea?

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 think that's fine, like it'll print error? At least that's how I've parsed your lets return Result, but now I see that you wanted to return it from run_return, but you can't do that.

I'm not sure that this is any different than then println and then exit that what it should do anyway, no?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think my main issue with last time I tried it is that you don't get any backtrace by doing this. Which should be fine in our case since we were just using println anyway.

Comment on lines 1353 to 1351
// Stash the error and exit with non-zero code.
*loop_error.lock().unwrap() = result.into();
*control_flow = ControlFlow::ExitWithCode(1);
return;
Copy link
Member

Choose a reason for hiding this comment

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

Wait can we not just exit with any generic result, which is then returned from the closure?! That's kinda garbage.

Let's maybe just print the error directly inside the run_return, then exit upwards with just any random error text?

I definitely don't want to throw mutexes at a silly one-off error that can only happen right before the event loop crashes anyway.

Copy link
Member Author

@kchibisov kchibisov Aug 7, 2022

Choose a reason for hiding this comment

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

We can, sure.

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.

I think it would be nice to have the ability to return from winit's run_return with any type parameter, but this should be fine for now.

@@ -62,7 +62,7 @@ use crate::event::{Event, Processor};
#[cfg(target_os = "macos")]
use crate::macos::locale;

fn main() {
fn main() -> Result<(), Box<dyn Error>> {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think my main issue with last time I tried it is that you don't get any backtrace by doing this. Which should be fine in our case since we were just using println anyway.

@kchibisov kchibisov merged commit 7d708d5 into alacritty:master Aug 10, 2022
@kchibisov kchibisov deleted the update-winit-0.27 branch August 10, 2022 12:48
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.

Reopening Terminal Powering off screns results in inconsistent DPI/font-size
4 participants