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(linux): expose XSetErrorHandler #12

Conversation

ventsislav-georgiev
Copy link

@ventsislav-georgiev ventsislav-georgiev commented Jan 5, 2022

Resolves: #11

Copy link
Member

@changkun changkun left a comment

Choose a reason for hiding this comment

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

While I appreciate that this is an attempt of resolving #11, but adding a global error channel is clearly not the solution we want. Especially the added Err API does not tell which hotkey registration was failed, which makes returned Event completely useless.

hotkey.go Outdated Show resolved Hide resolved
hotkey_linux.c Outdated Show resolved Hide resolved
@ventsislav-georgiev ventsislav-georgiev marked this pull request as draft January 5, 2022 22:19
@ventsislav-georgiev
Copy link
Author

I've taken another approach. Making the XSetErrorHandler an opt-in for Linux implementation only.
We keep the common API consistent.
We do not introduce any undesired, hidden from the user behaviour on Linux.
We provide a proper way for the user to receive the actual XErrorEvent.

@ventsislav-georgiev ventsislav-georgiev changed the title fix(linux): crash on existing hotkey grab feat(linux): expose XSetErrorHandler Jan 5, 2022
@ventsislav-georgiev
Copy link
Author

@changkun any feedback here? Do you have something against the new approach?

@changkun
Copy link
Member

changkun commented Jan 7, 2022

Thanks for contributing another workaround. The approach might solve the Linux problem, but introducing a platform-specific API is not the goal of simplicity.

The issue should be resolved with the absence of public APIs regarding explicit error handling. We either stick to the runtime panic that effectively warns the user or solve the problem entirely by knowing the registration error immediately at the Register call.

I do not have a chance to work on this before the end of next week, if this is needed urgently, you may switch back to a forked version that applies this patch.

According to the previous discussion, I'd like to close this for the clarification of rejecting any platform-specific workarounds. Any other actual solutions that match the previously described expectations remain kindly welcome and appreciated.

@changkun changkun closed this Jan 7, 2022
@ventsislav-georgiev ventsislav-georgiev deleted the linux-crash branch January 7, 2022 18:57
@ventsislav-georgiev
Copy link
Author

I see. Will be great if you manage to solve it properly. But I disagree that runtime panic is acceptable. IMO panic should happen only on startup and not based on user configuration that may change at runtime.

On another note regarding API consistency and simplicity. There currently are platform specific variables which will be better to be handled in a single wrapper in the hotkey package.
Alt key per platforms:

I've currently added per platform wrappers for these:
https://github.com/ventsislav-georgiev/prosper/tree/main/pkg/helpers/hotkeyh

@changkun
Copy link
Member

changkun commented Jan 7, 2022

The key variables are different. The actual unification is hotkey.Key and hotkey.Modifier. Using keycode directly is possible:

switch runtime.GOOS {
case "linux":
    mods = []hotkey.Modifier{0x1, 0x2, 0x3}
case "darwin":
    mods = []hotkey.Modifier{0x4, 0x5, 0x6}
case "windows":
    mods = []hotkey.Modifier{0x9, 0x8, 0x7}

without being bothered by those convenient name shortcuts.

@ventsislav-georgiev
Copy link
Author

Using hotkey.Modifier directly for unification makes the existence of the key variables redundant and is confusing to keep them for per platform usage as per your reasoning for closing this PR.

@changkun
Copy link
Member

changkun commented Jan 7, 2022

Using hotkey.Modifier directly for unification makes the existence of the key variables redundant and is confusing to keep them for per platform usage as per your reasoning for closing this PR.

If you find the package confusing then please leave it. Thanks.

@ventsislav-georgiev
Copy link
Author

I like the package, just giving my two cents about improving it. Didn't mean to offend you or the package in any way. I apologise I sounded like that.

@ventsislav-georgiev ventsislav-georgiev restored the linux-crash branch January 7, 2022 19:41
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.

BadAccess (X_GrabKey) when registering combination is obtained by others
2 participants