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

Tracking issue for different implementation between winit #470

Open
4 of 22 tasks
wusyong opened this issue Jul 10, 2022 · 11 comments
Open
4 of 22 tasks

Tracking issue for different implementation between winit #470

wusyong opened this issue Jul 10, 2022 · 11 comments
Labels
platform: All priority: medium Great to have status: needs triage This issue or pull request needs to be investigated

Comments

@wusyong
Copy link
Member

wusyong commented Jul 10, 2022

This is an issue to track which PR or commit we add that's not in winit yet.
We should either add to winit too or adjust the behavior in tao, so they are consistent.

@wusyong wusyong added status: needs triage This issue or pull request needs to be investigated priority: medium Great to have platform: All labels Jul 10, 2022
@amrbashir amrbashir pinned this issue Oct 21, 2022
@luffyfly
Copy link

luffyfly commented Nov 9, 2022

There is no clipboard implementations in winit.

@luffyfly
Copy link

luffyfly commented Nov 9, 2022

There is no global-shortcut implementations in winit, too.

@amrbashir
Copy link
Member

clipboard will be replaced with arboard and globalShortcut will be replaced with DeviceEvents

@luffyfly
Copy link

luffyfly commented Nov 9, 2022

clipboard will be replaced with arboard and globalShortcut will be replaced with DeviceEvent

Happy to hear that and is there any plan to merge tao to winit?

@FabianLars
Copy link
Member

That's the idea Amr and Yu-Wei are creating PRs at winit non-stop to get stuff we need implemented in winit so that we hopefully can switch at some point.

@fgimian
Copy link

fgimian commented Nov 28, 2022

clipboard will be replaced with arboard and globalShortcut will be replaced with DeviceEvents

I'm testing DeviceEvents on Windows as compared to a native implementation using RegisterHotKey as is performed by tao and sadly the results with DeviceEvents are inferior for the following reasons:

  • DeviceEvents seems to only send an event when a key is released, not when a key is pressed
  • Even when releasing the key as fast as I can, I see at least a 50ms difference between seeing the event registered with RegisterHotKey which is noticable in use
  • DeviceEvents doesn't take over the original shortcut like RegisterHotKey does; for example if I bind Volume Up with RegisterHotKey, I will no longer see the original volume slider as my app completely overrides it, with DeviceEvents the original action from the OS is still performed in addition to our own
  • Setting the device_event_filter to DeviceEventFilter::Never will now also process all other device events including any keybord presses and mouse movements which is much less efficient than capturing a particular set of hotkeys

Perhaps it may be worth evaluating these differences on Windows and reconsidering this?

Cheers
Fotis

@amrbashir
Copy link
Member

  • DeviceEvents seems to only send an event when a key is released, not when a key is pressed
  • Even when releasing the key as fast as I can, I see at least a 50ms difference between seeing the event registered with RegisterHotKey which is noticable in use

There is two separate events, Pressed and Released, I can see both so please double-check that otherwise it is a bug.

  • DeviceEvents doesn't take over the original shortcut like RegisterHotKey does; for example if I bind Volume Up with RegisterHotKey, I will no longer see the original volume slider as my app completely overrides it, with DeviceEvents the original action from the OS is still performed in addition to our own

I am aware of that, and I think it is a fair trade off for the flexibility of device events

  • Setting the device_event_filter to DeviceEventFilter::Never will now also process all other device events including any keybord presses and mouse movements which is much less efficient than capturing a particular set of hotkeys

That is true and might be a good reason to stick with the current implementation especially since handling these events resulted in a slower webview rendering in WRY.

Perhaps it may be worth evaluating these differences on Windows and reconsidering this?

Nothing is set in stone yet so thanks for your feedback.

@fgimian
Copy link

fgimian commented Nov 29, 2022

  • DeviceEvents seems to only send an event when a key is released, not when a key is pressed
  • Even when releasing the key as fast as I can, I see at least a 50ms difference between seeing the event registered with RegisterHotKey which is noticable in use

There is two separate events, Pressed and Released, I can see both so please double-check that otherwise it is a bug.

Ah I didn't realise this was a bug and I do see that it was fixed on a slightly newer revision than I'm using so this is likely due to an older version of winit on my end 😄

  • DeviceEvents doesn't take over the original shortcut like RegisterHotKey does; for example if I bind Volume Up with RegisterHotKey, I will no longer see the original volume slider as my app completely overrides it, with DeviceEvents the original action from the OS is still performed in addition to our own

I am aware of that, and I think it is a fair trade off for the flexibility of device events

Totally understand. I think there are use cases for both. For example, I'm building a volume control app which needs to take over the volume keys. However in most cases, this won't be needed.

Appreciate the reply
Fotis

@declantsien
Copy link
Contributor

Linux platform's WindowEvent::ModifiersChanged(state) in inconsistent with winit.
winit maintains the ModifiersState for me.
for tao I have to maintain the state my self with some hacky code like this

...(&mut self, state: ModifiersState) {
        if state.is_empty() {
            self.modifiers = state;
        } else if state.shift_key() {
            self.modifiers.set(ModifiersState::SHIFT, state.shift_key());
        } else if state.control_key() {
            self.modifiers
                .set(ModifiersState::CONTROL, state.control_key());
        } else if state.alt_key() {
            self.modifiers.set(ModifiersState::ALT, state.alt_key());
        } else if state.super_key() {
            self.modifiers.set(ModifiersState::SUPER, state.super_key());
        }

        log::trace!("modifer changed {:?}", self.modifiers);
    }```

@amrbashir
Copy link
Member

our current key handling was just a merge of new-keyboard branch from winit which winit plans to roll out soon in rust-windowing/winit#2662

@declantsien
Copy link
Contributor

declantsien commented Feb 7, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: All priority: medium Great to have status: needs triage This issue or pull request needs to be investigated
Projects
Status: 📬Proposal
Development

No branches or pull requests

6 participants