Skip to content

Conversation

@taooceros
Copy link
Member

fix #788

@jjw24
Copy link
Member

jjw24 commented Nov 29, 2021

@taooceros fair bit of method signature changes. can you give me a quick overview of why they are changed please

@jjw24 jjw24 added the bug Something isn't working label Nov 29, 2021
@jjw24 jjw24 added this to the 1.9.0 milestone Nov 29, 2021
@jjw24 jjw24 added review in progress Indicates that a review is in progress for this PR and removed review in progress Indicates that a review is in progress for this PR labels Nov 29, 2021
@taooceros
Copy link
Member Author

@taooceros fair bit of method signature changes. can you give me a quick overview of why they are changed please

Only the global hotkey event in publicapi is obsolete, and two new api is added to handle the same work.

@taooceros
Copy link
Member Author

Unsafe is used because

  1. It's reasonable to have high performance here because every key hit will trigger the running of the function once.
  2. I would like to try the new function pointer feature🤔😂
  3. I am not sure whether some issues may exist because of unsafe, so we should take a small set of testing here. However, it is unlikely we need to unload assembly for infrastructure, so it should be fine.

onesounds
onesounds previously approved these changes Dec 5, 2021
Garulf
Garulf previously approved these changes Dec 5, 2021
@jjw24
Copy link
Member

jjw24 commented Dec 6, 2021

what's been tested with this change so far?

also need to bump Shell plugin

@onesounds
Copy link
Contributor

onesounds commented Dec 6, 2021

what's been tested with this change so far?

also need to bump Shell plugin

It is a pr to solve the problem that when win+r is pressed, the flow closes and the run command (win+r by os) operates. I understand that the code has become complicated because there is a part in the middle of the work that does not work properly related to animation. I kept testing by pressing win+r in several situations.

@jjw24
Copy link
Member

jjw24 commented Dec 6, 2021

well i dont exactly understand this change, so if @taooceros is sure this is good to go and not going to break anything then happy for one of you to approve and merge this.

@taooceros
Copy link
Member Author

well i dont exactly understand this change, so if @taooceros is sure this is good to go and not going to break anything then happy for one of you to approve and merge this.

The main change is because the multicast delegate (event) fails to return all return values so that the win+r may not work with the presence of the windows walker. The main change of the pull request change the event to a list of Func to retrieve the return value correctly.
Also, because c# 9 added the function pointer, so I think it's cool to use that in the hook for better performance because the hook is fired in every single keyboard click.

jjw24
jjw24 previously requested changes Dec 7, 2021
Copy link
Member

@jjw24 jjw24 left a comment

Choose a reason for hiding this comment

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

somehow the artifacts are smaller, doesnt seem normal.

image

@taooceros taooceros force-pushed the GlobalHotkeyRefactor branch from d70bd85 to f701c0b Compare December 7, 2021 04:37
@Garulf
Copy link
Member

Garulf commented Dec 8, 2021

  • Build looks good
  • Win+R fixed
  • Fix for Tab complete
  • Working with Window Walker
  • No crashes

Good to go?

@jjw24 jjw24 changed the title Global hotkey refactor [dev] Global hotkey refactor Dec 8, 2021
@jjw24 jjw24 removed this from the 1.9.0 milestone Dec 8, 2021
@jjw24 jjw24 dismissed their stale review December 8, 2021 09:53

fixed

Copy link
Member

@jjw24 jjw24 left a comment

Choose a reason for hiding this comment

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

unsure of code changes but tested working fine, will merge since this addresses an issue in current dev

@jjw24 jjw24 merged commit 2855c6c into dev Dec 8, 2021
@jjw24 jjw24 deleted the GlobalHotkeyRefactor branch December 8, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Win+R with Shell plugin doesn't work well (with windows walker)

5 participants