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

feat: support accelerator key strings , - . Space Tab and F13-F24 #228

Merged
merged 10 commits into from
Nov 15, 2021
Merged

feat: support accelerator key strings , - . Space Tab and F13-F24 #228

merged 10 commits into from
Nov 15, 2021

Conversation

probablykasper
Copy link
Member

@probablykasper probablykasper commented Oct 27, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Documentation
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
Add support for more accelerator keys: , - . = ; / \ ' ` [ ] Space Tab and F13-F24

@probablykasper probablykasper requested a review from a team as a code owner October 27, 2021 16:08
@probablykasper probablykasper requested a review from a team October 27, 2021 16:08
@amrbashir
Copy link
Member

amrbashir commented Oct 27, 2021

Yeah we definitely need to make sure they are consistent across all platforms before we merge this, good thing I will be back by Saturday morning and I can help test this if no one beats me to it.

src/platform_impl/macos/menu.rs Outdated Show resolved Hide resolved
@amrbashir
Copy link
Member

You can add these also to other platforms.

  • Windows - format_hotkey() function in src/platform_impl/windows/menu.rs
  • Linux - register_accelerator() function in src/platform_impl/linux/menu.rs and key_to_raw_key() function in src/platform_impl/linux/keyboard.rs, (this one is using the gdk keys from here)

@probablykasper
Copy link
Member Author

Support here is already inconsistent between platforms. Tab and Esc are already supported on Windows, but weren't on macOS. You might think specifying unsupported keys is ok because nothing happens on macOS, but on Windows it actually panics.

On Windows, the format_hotkey() function already works fine, but key_to_vk() in keyboard.rs is what's lacking support. A bunch of keys in winapi don't have an obvious one corresponding to macOS.

The following are keys I didn't manage to add, so I've removed them from the PR:

  • `
  • \
  • [
  • ]
  • =
  • '
  • ;
  • /
  • F25-F35

@probablykasper
Copy link
Member Author

Brought Linux up to speed as well. Esc was already supported.

Tab is supposed to already be supported, but it's not working for me (even though it shows up correctly in the menu, and doesn't panic). Not something I'll tackle in this PR tho.

@probablykasper probablykasper changed the title feat(macOS): support more accelerator key strings feat: support more accelerator key strings Nov 14, 2021
@probablykasper probablykasper changed the title feat: support more accelerator key strings feat: support accelerator key strings , - . Space Tab and F13-F24 Nov 14, 2021
@amrbashir
Copy link
Member

the missing vkeys on windows can be obtained by VkKeyScanW(']' as u16) in key_to_vk function. check how KeyCode::Digit1 is mapped in that function for reference.

@amrbashir
Copy link
Member

amrbashir commented Nov 14, 2021

Tab and Esc are not necessary imo so you can ignore them. Also F keys like >=F13 won't be used a lot so it is okay if they are not supported on all platforms.

@nothingismagick
Copy link
Member

I think tab is important for keyboard navigation for visually impaired. Right @parker-codes ?

@amrbashir
Copy link
Member

amrbashir commented Nov 14, 2021

Keyboard navigation using Tab will still work, this PR addresses custom menu Accelerators and possibly global hotkeys.

But there is no harm in adding it this PR I guess, only macOS doesn't support Tab as accelerator so there is a minor inconsistency.

@parker-codes
Copy link

Yeah, as long as keys like Tab (or Shift + Tab) and Space are usable in general, as in they aren't being overridden, we're good.

It sounds like this is for combinations and shortcuts which doesn't seem like an issue to me. In fact, that can increase accessibility by allowing apps to offer quicker access!

@probablykasper
Copy link
Member Author

the missing vkeys on windows can be obtained by VkKeyScanW(']' as u16) in key_to_vk function. check how KeyCode::Digit1 is mapped in that function for reference.

Will try that out, thanks!

About Tab, it's just on Linux that it's not working. Windows already supports it fine, and macOS support is added in this PR. Opened issue #242 for it.

@probablykasper
Copy link
Member Author

All good now!

@wusyong wusyong merged commit b047ae4 into tauri-apps:dev Nov 15, 2021
@github-actions github-actions bot mentioned this pull request Nov 15, 2021
wusyong pushed a commit that referenced this pull request Nov 15, 2021
…F13`-`F24` (#228)

* feat(macOS): support more accelerator key strings

* Move function keys together

* Add `,` `-` `.` `Space` `F20-F24` for Windows

* Remove support for accelerators not found in `winapi`

* Add `,` `-` `.` `Space` `F13-F24` for Linux

* Update .changes

* Add the rest for Windows

* Add the rest for Linux

* Add the rest on macOS

* Update accelerator-strings.md
wusyong pushed a commit that referenced this pull request Nov 15, 2021
* feat(macos): Add `unhide_application` method, closes #182 (#231)

* feat(macos): Add `unhide_application` method

* Update src/platform/macos.rs

Co-authored-by: Amr Bashir <[email protected]>

* Reanme to `show_application()`

* Remove broken doc link

Co-authored-by: Amr Bashir <[email protected]>

* feat: Allow more strings to parse to keycode (#229)

* feat: support accelerator key strings `,` `-` `.` `Space` `Tab` and `F13`-`F24` (#228)

* feat(macOS): support more accelerator key strings

* Move function keys together

* Add `,` `-` `.` `Space` `F20-F24` for Windows

* Remove support for accelerators not found in `winapi`

* Add `,` `-` `.` `Space` `F13-F24` for Linux

* Update .changes

* Add the rest for Windows

* Add the rest for Linux

* Add the rest on macOS

* Update accelerator-strings.md

* Fix git comments

Co-authored-by: Kasper <[email protected]>
Co-authored-by: Amr Bashir <[email protected]>
wusyong pushed a commit that referenced this pull request Feb 5, 2022
* refactor(windows): `begin_resize_drag` now similar to gtk's (#200)

* refactor(windows): `begin_resize_drag` now similart to gtk's

* fix

* feat(linux): skipping taskbar will now also skip pager (#198)

* refactor(linux): clean dummy device_id (#195)

* refactor(linux): clean dummy device_id

* fmt

* feat(linux): allow resizing undecorated window using touch (#199)

* refactor(windows): only skip taskbar if needed when `set_visible` is called (#196)

* fix: increase borderless resizing inset (#202)

* fix: increase borderless resizing inset

* update some comments

* Replace winapi with windows crate bindings shared with WRY (#206)

* fix(deps): update rust crate libayatana-appindicator to 0.1.6 (#190)

Co-authored-by: Renovate Bot <[email protected]>

* Add Windows crate and webview2-com-sys bindings

* Initial port to webview2-com-sys

* Finish conversion and remove winapi

* Fix renamed lint warning

* Fix all match arms referencing const variables

* Put back the assert instead of expect

* Point to the published version of webview2-com-sys

* Cleanup slightly weird BOOL handling

* Replace mem::zeroed with Default::default

* Add a summary in .changes

* Remove extra projects not in config.json

* Fix clippy warnings

* Update to 32-bit compatible webview2-com-sys

* Better fix for merge conflict

* Fix clippy errors on Windows

* Use path prefix to prevent variable shadowing

* Fix Windows clippy warnings with nightly toolchain

* Fix Linux nightly/stable clippy warnings

* Fix macOS nightly/stable clippy warnings

* Put back public *mut libc::c_void for consistency

* Re-run cargo fmt

* Move call_default_window_proc to util mod

* Remove unnecessary util::to_wstring calls

* Don't repeat LRESULT expression in match arms

* Replace bitwise operations with util functions

* Cleanup more bit mask & shift with util fns

* Prefer from conversions instead of as cast

* Implement get_xbutton_wparam

* Use *mut libc::c_void for return types

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>

* fix(keyboard): add mapping for space key on Windows (#209)

* fix(keyboard): add mapping for space key on Windows

* change file

* feat: impl Clone for EventLoopWindowTarget (#211)

* chore: add `on_issue_closed.yml` (#214)

* Update tray dependency version (#217)

* Delete on_issue_closed.yml (#221)

* refactor(linux): event loop (#233)

* Use crossbeam::channel

* Fix crossbeam channel import

* Add check on poll event

* Fix deadlock when unregistering shortcut on Linux (#230)

* Add fullscreen monitor selection support on Linux (#235)

* Add fullscreen monitor support on Linux

* Add change file

* Remove todo on videomode

* Fix clippy

* Update to 2021 edition (#236)

* Update to 2021 edition

* Fix clippy

* Add run_return on Linux (#237)

* Add run_return on Linux

* Add main context

* Add run_return trait on Linux (#238)

* Fix: #239 Update webview2-com and windows crates (#240)

* Replace webivew2-com-sys with prebuilt windows

* Use windows utility instead of direct GetLastError

* Bump windows version and add changelog

* Run cargo fmt

* Restore inverted matches macro

* Scope constants in match arms

* Fix inverted null check

* Update src/platform_impl/windows/util.rs

Co-authored-by: Amr Bashir <[email protected]>

* Use env_logger instead of simple_logger (#241)

* Use env_logger instead of simple_logger

* Make clippy happy

* Cherry pick commits to next (#244)

* feat(macos): Add `unhide_application` method, closes #182 (#231)

* feat(macos): Add `unhide_application` method

* Update src/platform/macos.rs

Co-authored-by: Amr Bashir <[email protected]>

* Reanme to `show_application()`

* Remove broken doc link

Co-authored-by: Amr Bashir <[email protected]>

* feat: Allow more strings to parse to keycode (#229)

* feat: support accelerator key strings `,` `-` `.` `Space` `Tab` and `F13`-`F24` (#228)

* feat(macOS): support more accelerator key strings

* Move function keys together

* Add `,` `-` `.` `Space` `F20-F24` for Windows

* Remove support for accelerators not found in `winapi`

* Add `,` `-` `.` `Space` `F13-F24` for Linux

* Update .changes

* Add the rest for Windows

* Add the rest for Linux

* Add the rest on macOS

* Update accelerator-strings.md

* Fix git comments

Co-authored-by: Kasper <[email protected]>
Co-authored-by: Amr Bashir <[email protected]>

* Add redraw events on Linux (#245)

* Add redraw events on Linux

* Update doc of RequestRedraw Event

* Add change file

* Fix missing menu bar on borderless window (#247)

Credit goes to irh's work on winit commit f2de8475fc4703d03a2ecc2cda627b017202e623

* refactor: improve `set_skip_taskbar` impl on Windows (#250)

* fix: emit errors on parsing an invalid accelerator for string, closes #135 (#252)

* chore: update comment

* fix(linux): fix focus events not firing properly (#253)

* fix(linux): fix focus events not firing properly

* add changelog

* chore: update focus events error message

* chore: fmt

* fix: revert windows-rs 0.28 version bump

* fix(linux): fix native menu items (#256)

* chore: remove examples commited by accident

* Update `ReceivedImeText` (#251)

* Allow receiving text without Ime on Windows

* Avoid panic todo

* Receive text without ime on mac

* Fix CursorMoved event on Linux

* Add ReceivedImeText on Linux

This only add Simple IME from GTK for now. We should add the actual IME
from system in the future.

* Fix redraw event that causes inifinite loop (#260)

* Fix redraw event that causes inifinite loop

* Refactor event loop

* Remove unused function

* Add doc comment on linux's run_return

* Ignore doc test on run_return

* Add non blocking iteration on Linux (#261)

* Docs: SystemTrayExtWindows::remove() is gone (#262)

Fix docs following #153

* Fix busy loop on Linux (#265)

* Update windows crate to 0.29.0 (#266)

* Update to windows 0.29.0

* Add change description

* Remove clippy check (#267)

* refactor(windows): align util function with win32 names

* chore: update PR template

* fix(linux): fire resized & moved events on min/maximize, closes #219 (#254)

* feat(linux): implement `raw_window_handle()` (#269)

* chore(deps): update to raw-window-handle 0.4

* add linux raw-window-handle support

* update macos/ios/android

* fix ios

* Fix core-video-sys dependency (#274)

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (#275)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

* Fix some invalid msg_send! usage (#276)

* Revert "Fix some invalid msg_send! usage (#276)" (#277)

This reverts commit a3a2e0c.

* Revert "The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (#275)" (#279)

This reverts commit 6f9c468.

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (#280)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

Co-authored-by: madsmtm <[email protected]>

* Fix some invalid msg_send! usage (#278)

Co-authored-by: madsmtm <[email protected]>

* Add exit code to ControlFlow::Exit (#281)

* Add exit code to ControlFlow::Exit

* Cargo fmt

* Add change files

Co-authored-by:  multisn8 <[email protected]>

* Add new_any_thread to Unix event loop (#282)

* Update windows crate to 0.30.0 (#283)

* Update windows crate to 0.30.0

* Simplify new-type usage

* Fix boxing in GWL_USERDATA

* Make sure everyone is using Get/SetWindowLongPtrW

* build the system_tray module when "ayatana" feature is enabled (#285)

Without those cfg feature checks, the "ayatana" feature does
actually not enable anything.

* Fix click events missing whe tray has menu (#291)

* Fix click events missing whe tray has menu

* Add change file

* Fix crash when tray has no menu (#294)

* chore: update pull request commit exmple

* fix(windows): send correct position for system tray events, closes #295 (#300)

* fix(windows): revert maximized state handling to winit impl, closes #193 (#299)

* fix(windows): revet maximized state handling to winit impl, closes #193

* add chanefile [skip ci]

* fix: `MenuItem::Quit` on Windows (#303)

* fix: `MenuItem::Close` on Windows

* use `PostQuitMessage` instead

Co-authored-by: amrbashir <[email protected]>

* feat: v1 audit by Radically Open Security (#304)

* Update to gtk 0.15 (#288)

* Update to gtk 0.15

* Fix picky none on set_geometry_hint

* Fix CursorMoved position

Co-authored-by: Amr Bashir <[email protected]>
Co-authored-by: Bill Avery <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Co-authored-by: Kasper <[email protected]>
Co-authored-by: amrbashir <[email protected]>
Co-authored-by: Jay Oster <[email protected]>
Co-authored-by: madsmtm <[email protected]>
Co-authored-by: multisn8 <[email protected]>
Co-authored-by: Aurélien Jacobs <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
wusyong added a commit that referenced this pull request Feb 5, 2022
* feat(macos): Add `unhide_application` method, closes #182 (#231)

* feat(macos): Add `unhide_application` method

* Update src/platform/macos.rs

Co-authored-by: Amr Bashir <[email protected]>

* Reanme to `show_application()`

* Remove broken doc link

Co-authored-by: Amr Bashir <[email protected]>

* feat: Allow more strings to parse to keycode (#229)

* feat: support accelerator key strings `,` `-` `.` `Space` `Tab` and `F13`-`F24` (#228)

* feat(macOS): support more accelerator key strings

* Move function keys together

* Add `,` `-` `.` `Space` `F20-F24` for Windows

* Remove support for accelerators not found in `winapi`

* Add `,` `-` `.` `Space` `F13-F24` for Linux

* Update .changes

* Add the rest for Windows

* Add the rest for Linux

* Add the rest on macOS

* Update accelerator-strings.md

* publish new versions

Co-authored-by: Kasper <[email protected]>
Co-authored-by: Amr Bashir <[email protected]>
Co-authored-by: wusyong <[email protected]>
Co-authored-by: Ngo Iok Ui (Wu Yu Wei) <[email protected]>
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.

5 participants