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

GSOC2020: Ctrl-shift-U does not work work when editor is focused #9895 #9948

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yzykov
Copy link

@yzykov yzykov commented Mar 28, 2020

GSOC2020: Ctrl-shift-U does not work work when editor is focused #9895

1. I have tested the changes on Windows, Linux & Mac
2. I was following the existing style of the code. I noticed that SHORTCUT_SHIFT_KEY_MASK was missing and replaced with identical logic on line 1353. Therefore, I decided to create the variable.
3. According to [1] iBus is not supporting CTRL+SHIFT+U. Modified to use CTRL+ALT+U for Linux.

a) No users were affected on Windows and Mac OS, because preexisting behaviour is the same for Windows and Mac OS.
b) Users with Linux XIM/None settings are affected, because the hotkeys were changed.
4. Tested with XIM/None CTRL+SHIFT+U was working fine.
5. Tested with iBus/XIM/None CTRL+ALT+U is working on Linux.
[1] https://superuser.com/questions/358749/how-to-disable-ctrlshiftu-in-ubuntu-linux

@matthijskooijman
Copy link
Collaborator

Thanks for your contribution, I'm happy to test your this PR as the old behaviour was quite annoying :-)

For future reference: Instead of closing a PR and creating a new one, you can also (force) push to the same branch to add more commits or replace them. Also, if you do update a PR like this, it is helpful if you add a comment that describes what you changed and why, that makes review of the PR easier.

Then, looking at the actual code, I think you're making two distinct changes:

  • Introducing SHORTCUT_SHIFT_KEY_MASK to be used in newJMenuItemShift(). AFAICS this does not actually change anything, just refactors the code.
  • Replace the ctrl-shift-u shortcut with alt-shift-u on linux/unix/aix.

These change seem to be separate, so it would be good to have them in separate commits as well.

Furthermore, replacing the shortcut with another one is not a real fix for the problem. It helps to solve the immediate problem, at the cost of potentially making things more confusing for users that use multiple operating systems. We could maybe change the shortcut on all platforms (and maybe keep ctrl-shift-u as a secondary shortcut on platforms that support it), but I'd rather fix the underlying problem if possible.

From your comments, it is not immediately clear what the real underlying cause of this is. Apparently the "ibus" xim input method breaks this. I suppose that the editor uses xim for input, while the other parts of the interface do not?

The superuser thread you linked mostly suggests switching the input method, which does not really clarify the problem. There is one answer that has some more info (and I just edited it to add more): apparently ctrl-shift-u is the default "Insert unicode code point" shortcut for the ibus input method.

I wonder if there is anything else we can do to just actually make this work (maybe at the expense of breaking the "insert code point" shortcut), or somehow detect this and tell the user what is going on? We could maybe try changing the input method to something else, but that seems like a big change (the user might have intentionally configured this input method for some of its features). This answers suggests that you can disable some modifier keys with an env var, or maybe something can be done from within the application too?

@matthijskooijman matthijskooijman linked an issue Mar 28, 2020 that may be closed by this pull request
@yzykov
Copy link
Author

yzykov commented Mar 28, 2020

Thank you for the fast response.

The previous PR got messy, so it was easier for me to create a new branch with PR. I am not that familiar with a git functionality (TFS user), I will consider it in future. Thank you.

I did not consider the code refactoring to be standalone change, sorry about that. Should I create another PR for it, or it is "acceptable"?

As you already stated, I was following the superior thread and discussions there, so I came up with a solution that affects minimum amount of users (cross-platform & Linux XIM/None users). I did not want to interrupt the existing behaviour and change the input method for all OSes, this will cause even more confusions.

I am not familiar with the scripting system of Arduino. Basically, I do not have enough experience to say which shell script should I modify. I can modify the code if the proper solution will be made.

I have to say that CTRL+SHIFT+U does work in Visual Studio Code on Linux with iBus. Therefore, I am assuming that proposed solution will work on Linux, but I do not have enough experience (yet) to say how it should be implemented within shell script, so it will work on Windows/Mac OS.

Thanks for your contribution, I'm happy to test your this PR as the old behaviour was quite annoying :-)

For future reference: Instead of closing a PR and creating a new one, you can also (force) push to the same branch to add more commits or replace them. Also, if you do update a PR like this, it is helpful if you add a comment that describes what you changed and why, that makes review of the PR easier.

Then, looking at the actual code, I think you're making two distinct changes:

  • Introducing SHORTCUT_SHIFT_KEY_MASK to be used in newJMenuItemShift(). AFAICS this does not actually change anything, just refactors the code.
  • Replace the ctrl-shift-u shortcut with alt-shift-u on linux/unix/aix.

These change seem to be separate, so it would be good to have them in separate commits as well.

Furthermore, replacing the shortcut with another one is not a real fix for the problem. It helps to solve the immediate problem, at the cost of potentially making things more confusing for users that use multiple operating systems. We could maybe change the shortcut on all platforms (and maybe keep ctrl-shift-u as a secondary shortcut on platforms that support it), but I'd rather fix the underlying problem if possible.

From your comments, it is not immediately clear what the real underlying cause of this is. Apparently the "ibus" xim input method breaks this. I suppose that the editor uses xim for input, while the other parts of the interface do not?

The superuser thread you linked mostly suggests switching the input method, which does not really clarify the problem. There is one answer that has some more info (and I just edited it to add more): apparently ctrl-shift-u is the default "Insert unicode code point" shortcut for the ibus input method.

I wonder if there is anything else we can do to just actually make this work (maybe at the expense of breaking the "insert code point" shortcut), or somehow detect this and tell the user what is going on? We could maybe try changing the input method to something else, but that seems like a big change (the user might have intentionally configured this input method for some of its features). This answers suggests that you can disable some modifier keys with an env var, or maybe something can be done from within the application too?

// Since CTRL+SHIFT+U is not working on iBus keyboard input method
// Lets redirect the shorcut for Linux to CTRL+ALT+U
// Leaving the preexisting behaviour for Windows & Mac OS
String OS = System.getProperty("os.name").toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

We have helpers to recognize operating systems, it's better to use them.
THis could become

if (OSUtils.isWindows()) {
  item = newJMenuItemShift(tr("Upload Using Programmer"), 'U');
} else {
  item = newJMenuItemAlt(tr("Upload Using Programmer"), 'U');
}

Copy link
Author

@yzykov yzykov Mar 30, 2020

Choose a reason for hiding this comment

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

Thank you, I did not know that.
I will commit the change with Arduino Helpers.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

Ctrl-shift-U does not work work when editor is focused
4 participants