-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Windows]Refactor system context menu #1831
Conversation
As far as I am concern. I can't test on windows. Could you make a gif of the result (context) or pictures here. |
It looks the same as before. I added the picture nevertheless. |
} | ||
}); | ||
}; | ||
|
||
exports.remove = function (callback) { | ||
isRegistered(registered => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this check? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really needed since the destroy function returns an error if it can't find the Hyper key or something else goes wrong. Helps to debug if we get these menu bugs again. Also, less registry requests - faster (un)installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
Tested on Win10. Works really great! No more problem with install/uninstall. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Refactored the system context menu code. Now it's less complex and easier to debug in the future. This should fix all these #1734 #1668 #1651 #1133 bugs as far as my testing goes. Any more testing from other Win users always appreciated 😄 Otherwise it's ready for merge.