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

Fix proposal: safe unwrap of keyboard::Mod #780

Merged
merged 1 commit into from
Jul 8, 2018

Conversation

emirpnet
Copy link
Contributor

I had some trouble with panics caused by the unwrap of the keymod on the KeyDown event and changed the code to default to keyboard::Mod::empty().

To me that makes sense and solved the issue, but I am aware that in general this behavior might not be wanted.

@Cobrand
Copy link
Member

Cobrand commented Jul 4, 2018

That's interesting. Which keys did you press that made it panic? I'm happy to merge this, but I would like to know how you discovered this issue, might give us a clue on something else.

AFAIK rust-sdl2 is used quite a lot and this is the first time I've heard of such an issue, so this is surprising.

@Cobrand Cobrand merged commit 4bb6b9a into Rust-SDL2:master Jul 8, 2018
@Cobrand
Copy link
Member

Cobrand commented Jul 8, 2018

@emirpnet Small tip for cleaner rust code, but you could have done

keyboard::Mod::from_bits(event.keysym.mod_).unwrap_or(keyboard::Mod::empty()), it would have worked just as well. Still, thank you.

Would you mind telling us how did you discover this issue exactly?

@emirpnet
Copy link
Contributor Author

@Cobrand Thanks for the coding tip with the one-liner.

The code that panicked is the one decribed here: http://pramode.in/2016/10/12/conway-game-of-life-rust-sdl2/ (I followed it along to learn about rust-sdl. Regarding key events, that code only handles the Escape-Key to quit the program.)
The panic came up on the first key press with any key (except for Shift, Ctrl, ...). The issue occured on Ubuntu 16.04 LTS and only on a certain PC. I run the same Ubuntu version on my laptop, where I could not reproduce the problem. Maybe it is an unfortunate combination with the hardware or the installed libsdl version, but I am guessing here.

@emirpnet emirpnet deleted the fix-keymod-unwrap branch July 14, 2018 10:30
@kolen
Copy link
Contributor

kolen commented Apr 8, 2019

For me, it occurs (in sdl2 versions before this fix) on Event::KeyUp (but not KeyDown) for almost any key (including modifier keys) under Mac OS 10.13.6. I tried to add debugging to version 0.22.0:

raw.data: [1, 3, 0, 0, 5b, d, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 27, 0, 0, 0, 30, 0, 0, 0, 0, 0, 78, e9, fe, 7f, 0, 0, 82, ac, 9d, 6, 1, 0, 0, 0, 30, 55, 44, ae, 9e, 7f, 0, 0, 10, 45, 51, ae, 9e, 7f, 0, 0]
[/tmp/rust-sdl2/src/sdl2/event.rs:810] event.keysym.sym = 48
[/tmp/rust-sdl2/src/sdl2/event.rs:811] event.keysym.scancode = 39
[/tmp/rust-sdl2/src/sdl2/event.rs:812] event.keysym._mod = 32766
[/tmp/rust-sdl2/src/sdl2/event.rs:812] dbg!(event . keysym . _mod) as SDL_Keymod = 32766
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21

Hmm, 32766 is "all bits except least significant set", I can't see value for this in SDL_Keymod constants.

@kolen
Copy link
Contributor

kolen commented Apr 8, 2019

After adding Debug derive for SDL_KeyboardEvent and SDL_Keysym:

[/tmp/rust-sdl2/src/sdl2/event.rs:806] event = SDL_KeyboardEvent {
    type_: 769,
    timestamp: 2512,
    windowID: 1,
    state: 0,
    repeat: 0,
    padding2: 0,
    padding3: 0,
    keysym: SDL_Keysym {
        scancode: 39,
        sym: 48,
        _mod: 32766,
        unused: 3830513664
    }
}

@kolen
Copy link
Contributor

kolen commented Apr 8, 2019

Maybe sdl is free to set remaining bits, for which there's no SDL_Keymod constants, and therefore from_bits_truncate should be used here. However, in this case all KeyUp events on my system would have all modifiers except left shift pressed. It's weird. We have to guard against malformed key mod values returned by sdl and replace them with "no modifiers"?

@Cobrand
Copy link
Member

Cobrand commented Apr 9, 2019

@kolen can you open another issue with the exact problem there? I have quite a hard time understanding what is wrong here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants