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

DeviceIds contain no data on most platforms #338

Closed
Xaeroxe opened this issue Nov 2, 2017 · 20 comments
Closed

DeviceIds contain no data on most platforms #338

Xaeroxe opened this issue Nov 2, 2017 · 20 comments
Labels
S - api Design and usability S - platform parity Unintended platform differences

Comments

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Nov 2, 2017

This needs to be fixed: https://github.com/tomaka/winit/blob/master/src/platform/windows/mod.rs#L22

Since Windows makes up 96% of gamers at the moment this effectively makes the DeviceId useless.

EDIT: winit is not exclusively a gaming library.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

Having just browsed the Windows source, I can now tell you this isn't as bad as it first appears because the only places this is used is in keyboard and mouse input, and thanks to the distinction between those events it's actually impossible to confuse these. So the DeviceId, while useless, also isn't really needed.

@Xaeroxe Xaeroxe changed the title DeviceIds on Windows are useless DeviceIds on Windows and OSX are useless Nov 6, 2017
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

Just found this is also an issue for OSX.

@Xaeroxe Xaeroxe changed the title DeviceIds on Windows and OSX are useless DeviceIds are nearly useless Nov 6, 2017
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

After even further scouring this is literally only used for the X11 backend. DeviceId is just a dummy value for every other platform.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

So, I'd like to propose that we restructure events. Since 99% of the time DeviceEvents are just raw mouse input we should label them as such, then create a new event for non-mouse devices that will only be emitted on X11 (and maybe other platforms if support for that is ever added)

Further, we should remove most of the DeviceIds from the current event structure, as they currently only serve to confuse people and basically just lie about the library's capabilities.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

DeviceEvent::Added and DeviceEvent::Removed are completely dead. Never emitted anywhere.

@torkleyy
Copy link
Contributor

torkleyy commented Nov 6, 2017

Device ids can be useful when using multiseat AFAIK.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

@torkleyy Sure, but that's only meaningful if the device id actually carries any data. It doesn't 99% of the time, throughout most of winit it's just dead characters.

@torkleyy
Copy link
Contributor

torkleyy commented Nov 6, 2017

So what is it that you're suggesting?

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

Key:

  • M - Modify
  • R - Remove
  • A - Add

R DeviceEvent::Added

R DeviceEvent::Removed

R WindowEvent::AxisMotion

M WindowEvent Remove all DeviceIds from this enum, as they carry 0 useful data

M winit::Touch remove the DeviceId as it's also useless

M DeviceEvent::Button and DeviceEvent::Motion Either start emitting these for devices other than the mouse on platforms other than X11, or just remove it entirely. Having this behave as it does today is dangerous as it can lead developers to believe they have support for arbitrary input devices on all platforms when in fact they are far from it.

R DeviceEvent::Key - Not needed when the window isn't focused

R DeviceEvent::Text - Not needed when the window isn't focused

M WindowEvent::MouseMoved rename this to CursorMoved.

A WindowEvent::MouseMoved largely replaces today's DeviceEvent::Motion, but includes semantics for this being the mouse. Also includes both x and y, rather than just one axis.

@Ralith
Copy link
Contributor

Ralith commented Nov 6, 2017

The proposed changes include significant regressions.

Having just browsed the Windows source, I can now tell you this isn't as bad as it first appears because the only places this is used is in keyboard and mouse input, and thanks to the distinction between those events it's actually impossible to confuse these. So the DeviceId, while useless, also isn't really needed.

The purpose of DeviceId is to distinguish between different keyboards and mice (for example), not to distinguish keyboards from mice.

DeviceEvent::Added and DeviceEvent::Removed are completely dead. Never emitted anywhere.

I'm not sure how you came to this conclusion. They are used on the X11 backend, and are necessary for good hotplugging support.

M DeviceEvent::Button and DeviceEvent::Motion Either start emitting these for devices other than the mouse on platforms other than X11, or just remove it entirely. Having this behave as it does today is dangerous as it can lead developers to believe they have support for arbitrary input devices on all platforms when in fact they are far from it.

These are expected to be necessary to access non-mouse/kb devices on Wayland; upstream plans preclude the normal, winit-independent linux input device methods.

A WindowEvent::MouseMoved largely replaces today's DeviceEvent::Motion, but includes semantics for this being the mouse. Also includes both x and y, rather than just one axis.

Physical mouse (as opposed to cursor) motion is not associated with a window on most platforms. This is why the DeviceEvent notion exists.

On the whole, your reasoning here seems to be "Windows support for these recently introduced features is lacking, therefore they should be removed entirely." If Windows support concerns you to that extent, I suggest completing it instead. If you are only concerned about confusion but are not actually interested in the features, documenting their completeness would probably be appropriate.

While Windows may account for "96% of gamers," winit is not only a game library, and I believe its users are not nearly so concentrated.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

The purpose of DeviceId is to distinguish between different keyboards and mice (for example), not to distinguish keyboards from mice.

It fails entirely in this regard as for every platform that isn't X11 it's Eq implementation is literally just return true;

These are expected to be necessary to access non-mouse/kb devices on Wayland; upstream plans preclude the normal, winit-independent linux input device methods.

Great! But right now we have so many other places these could be used but they just aren't. Finish the job, don't do it at all, or indicate in documentation somehow this is very incomplete.

I'm going to call a straw man fallacy on the windows portion of your comment. This started as a Windows issue but the scope has changed to encompass literally everything that isn't X11. You on the other hand seem entirely concerned with the Linux experience and have paid no heed to the impact on other platforms.

@Ralith
Copy link
Contributor

Ralith commented Nov 6, 2017

Finish the job, don't do it at all, or indicate in documentation somehow this is very incomplete.

winit is not a finished library. Refer to #252 for detailed discussion of platform support consistency. Contributions to improve and maintain documentation about this would certainly be valuable. Removing all incompletely supported features would not leave very much.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

I'm not sure how you came to this conclusion. They are used on the X11 backend, and are necessary for good hotplugging support.

My bad. I missed their usage when I did my search.

Physical mouse (as opposed to cursor) motion is not associated with a window on most platforms. This is why the DeviceEvent notion exists.

I understand that. By the time I got done with my list the DeviceEvent enum was so sparse that it made more sense to just drop it and integrate the last remaining event into WindowEvent. If we keep DeviceEvent then it makes sense to keep it there.

@Ralith
Copy link
Contributor

Ralith commented Nov 6, 2017

By the time I got done with my list the DeviceEvent enum was so sparse that it made more sense to just drop it and integrate the last remaining event into WindowEvent

WindowEvents must be associated with a window. Mouse movement may be received even if there are no windows, so this is not possible.

To be clear, I absolutely am concerned for the quality of winit's API across all major platforms. I cannot personally supply implementations for a given feature on every single platform, but I want to be sure we produce a rich and usable API that is useful and realistically implementable in most places. To my knowledge, based on discussions with @retep998, these features make perfect sense on Windows, and do not represent a disproportionate implementation challenge.

Some of the proposed changes seem reasonable to me. Wholesale removal of device IDs and events are not one of them, but e.g. the proposed renamings seem like a clarity boon.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 6, 2017

A lot of my complaints are resolved by #344 .

Before we can close this issue we should

  • Add support for arbitrary input devices on all platforms as best as we can
  • Make the DeviceId useful based on those input devices.

@Ralith
Copy link
Contributor

Ralith commented Nov 7, 2017

See also #212 for prior musing on arbitrary input device support.

francesca64 added a commit to francesca64/winit that referenced this issue Apr 22, 2018
Fixes rust-windowing#467

All variants other than Text have been implemented. While Text can
be implemented using ToUnicode, that doesn't play nice with dead
keys, IME, etc.

Most of the mouse DeviceEvents were already implemented, but due
to the flags that were used when registering for raw input events,
they only worked when the window was in the foreground.

This is also a step forward for rust-windowing#338, as DeviceIds are no longer
useless on Windows. On DeviceEvents, the DeviceId contains that
device's handle. While that handle could ostensibly be used by
developers to query device information, my actual reason for
choosing it is because it's simply a very easy way to handle this.
As a fun bonus, this enabled me to create this method:
  DevideIdExt::get_persistent_identifier() -> Option<String>
Using this gives you a unique identifier for the device that
persists across replugs/reboots/etc., so it's ideal for something
like device-specific configuration.

There's a notable caveat to the new DeviceIds, which is that the
value will always be 0 for a WindowEvent. There doesn't seem to be
any straightforward way around this limitation.

I was concerned that multi-window applications would receive n
copies of every DeviceEvent, but Windows only sends them to one
window per application.

Lastly, there's a chance that these additions will cause
antivirus/etc. software to detect winit applications as keyloggers.
I don't know how likely that is to actually happen to people, but
if it does become an issue, the raw input code is neatly
sequestered and would be easy to make optional during compilation.
francesca64 added a commit to francesca64/winit that referenced this issue Apr 23, 2018
Fixes rust-windowing#467

All variants other than Text have been implemented. While Text can
be implemented using ToUnicode, that doesn't play nice with dead
keys, IME, etc.

Most of the mouse DeviceEvents were already implemented, but due
to the flags that were used when registering for raw input events,
they only worked when the window was in the foreground.

This is also a step forward for rust-windowing#338, as DeviceIds are no longer
useless on Windows. On DeviceEvents, the DeviceId contains that
device's handle. While that handle could ostensibly be used by
developers to query device information, my actual reason for
choosing it is because it's simply a very easy way to handle this.
As a fun bonus, this enabled me to create this method:
  DevideIdExt::get_persistent_identifier() -> Option<String>
Using this gives you a unique identifier for the device that
persists across replugs/reboots/etc., so it's ideal for something
like device-specific configuration.

There's a notable caveat to the new DeviceIds, which is that the
value will always be 0 for a WindowEvent. There doesn't seem to be
any straightforward way around this limitation.

I was concerned that multi-window applications would receive n
copies of every DeviceEvent, but Windows only sends them to one
window per application.

Lastly, there's a chance that these additions will cause
antivirus/etc. software to detect winit applications as keyloggers.
I don't know how likely that is to actually happen to people, but
if it does become an issue, the raw input code is neatly
sequestered and would be easy to make optional during compilation.
francesca64 added a commit to francesca64/winit that referenced this issue Apr 26, 2018
Fixes rust-windowing#467

All variants other than Text have been implemented. While Text can
be implemented using ToUnicode, that doesn't play nice with dead
keys, IME, etc.

Most of the mouse DeviceEvents were already implemented, but due
to the flags that were used when registering for raw input events,
they only worked when the window was in the foreground.

This is also a step forward for rust-windowing#338, as DeviceIds are no longer
useless on Windows. On DeviceEvents, the DeviceId contains that
device's handle. While that handle could ostensibly be used by
developers to query device information, my actual reason for
choosing it is because it's simply a very easy way to handle this.
As a fun bonus, this enabled me to create this method:
  DevideIdExt::get_persistent_identifier() -> Option<String>
Using this gives you a unique identifier for the device that
persists across replugs/reboots/etc., so it's ideal for something
like device-specific configuration.

There's a notable caveat to the new DeviceIds, which is that the
value will always be 0 for a WindowEvent. There doesn't seem to be
any straightforward way around this limitation.

I was concerned that multi-window applications would receive n
copies of every DeviceEvent, but Windows only sends them to one
window per application.

Lastly, there's a chance that these additions will cause
antivirus/etc. software to detect winit applications as keyloggers.
I don't know how likely that is to actually happen to people, but
if it does become an issue, the raw input code is neatly
sequestered and would be easy to make optional during compilation.
francesca64 added a commit to francesca64/winit that referenced this issue Apr 27, 2018
Fixes rust-windowing#467

All variants other than Text have been implemented. While Text can
be implemented using ToUnicode, that doesn't play nice with dead
keys, IME, etc.

Most of the mouse DeviceEvents were already implemented, but due
to the flags that were used when registering for raw input events,
they only worked when the window was in the foreground.

This is also a step forward for rust-windowing#338, as DeviceIds are no longer
useless on Windows. On DeviceEvents, the DeviceId contains that
device's handle. While that handle could ostensibly be used by
developers to query device information, my actual reason for
choosing it is because it's simply a very easy way to handle this.
As a fun bonus, this enabled me to create this method:
  DevideIdExt::get_persistent_identifier() -> Option<String>
Using this gives you a unique identifier for the device that
persists across replugs/reboots/etc., so it's ideal for something
like device-specific configuration.

There's a notable caveat to the new DeviceIds, which is that the
value will always be 0 for a WindowEvent. There doesn't seem to be
any straightforward way around this limitation.

I was concerned that multi-window applications would receive n
copies of every DeviceEvent, but Windows only sends them to one
window per application.

Lastly, there's a chance that these additions will cause
antivirus/etc. software to detect winit applications as keyloggers.
I don't know how likely that is to actually happen to people, but
if it does become an issue, the raw input code is neatly
sequestered and would be easy to make optional during compilation.
francesca64 added a commit that referenced this issue Apr 28, 2018
Fixes #467

All variants other than Text have been implemented. While Text can
be implemented using ToUnicode, that doesn't play nice with dead
keys, IME, etc.

Most of the mouse DeviceEvents were already implemented, but due
to the flags that were used when registering for raw input events,
they only worked when the window was in the foreground.

This is also a step forward for #338, as DeviceIds are no longer
useless on Windows. On DeviceEvents, the DeviceId contains that
device's handle. While that handle could ostensibly be used by
developers to query device information, my actual reason for
choosing it is because it's simply a very easy way to handle this.
As a fun bonus, this enabled me to create this method:
  DevideIdExt::get_persistent_identifier() -> Option<String>
Using this gives you a unique identifier for the device that
persists across replugs/reboots/etc., so it's ideal for something
like device-specific configuration.

There's a notable caveat to the new DeviceIds, which is that the
value will always be 0 for a WindowEvent. There doesn't seem to be
any straightforward way around this limitation.

I was concerned that multi-window applications would receive n
copies of every DeviceEvent, but Windows only sends them to one
window per application.

Lastly, there's a chance that these additions will cause
antivirus/etc. software to detect winit applications as keyloggers.
I don't know how likely that is to actually happen to people, but
if it does become an issue, the raw input code is neatly
sequestered and would be easy to make optional during compilation.
@francesca64
Copy link
Member

DeviceId is potentially useful on Windows now (#482).

@francesca64 francesca64 added S - api Design and usability S - platform parity Unintended platform differences labels May 6, 2018
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented May 22, 2018

Thanks @francesca64 !

@Xaeroxe Xaeroxe changed the title DeviceIds are nearly useless DeviceIds contain no data on most platforms Jun 2, 2018
@francesca64
Copy link
Member

I should note that there doesn't seem to be any solution to this on macOS, and I'm also not optimistic about a solution for WindowEvent on Windows.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jun 3, 2018 via email

@Xaeroxe Xaeroxe closed this as completed Jun 3, 2018
tmfink pushed a commit to tmfink/winit that referenced this issue Jan 5, 2022
madsmtm pushed a commit to madsmtm/winit that referenced this issue Jun 11, 2022
* fix(Windows): fix a deadlock in `WindowState`

* add change file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - api Design and usability S - platform parity Unintended platform differences
Development

No branches or pull requests

4 participants