Desktop: fix issues with shortcut editor

[tessus]

String.fromCharCode expects unicode charcodes as an argument; e.keyCode returns javascript keycodes.
Javascript keycodes and unicode charcodes are not the same thing!

Therefore I used an array to map keycodes. This seems to be the only way to make this work properly.

I also changed the width of the shortcut recorder input field to 200px, since certain key combinations are cut off, because they don't fit in the field.

[laurent22]

Seems I missed this PR. I had a look at it and still couldn't fix it so I've reverted my previous change.

Your solution might be the way but would need to be tested in various cases and OSes, also maybe with both qwerty and non-qwerty keyboards to be sure. Is there no existing package we could use to handle this for us? Just seems like the kind of problem that have various edge cases that could have already been solved by a lib.

[tessus]

Your solution might be the way but would need to be tested in various cases and OSes, also maybe with both qwerty and non-qwerty keyboards to be sure.

I thought this was one of the reasons why we had the pre-releases. I did various tests with 3 external keyboards and changed keyboard layouts. I still don't have my Proxmox machine ready, so I couldn't test on other OS than macOS.
While I would rather do all the testing myself, I think there are situations where using a pre-release to get additional input/feedback is warranted.

The problem is that even reverting your fix won't change the fact that the keyboard editor is broken. Other than the issue that I can't use certain key combinations there's also this issue (which is afaik not limited to macOS):
e.g. when I press Shift+5, the keyboard editor shows Shift+% which is wrong, because there is no Shift+% combination. % can only be generated by pressing Shift, so Shift+% would actually mean Shift+Shift+5 which is impossible to press.

The choices are rather simple: you can add this PR or remove the keyboard editor. The keyboard editor is broken on macOS and most likely on the other OS as well. The only other possible solution is to use my fix only on macOS, which is the wrong approach, but if it makes you feel any better, by all means.

While I do understand your reluctance to add features, you shouldn't think twice about merging a fix. Especially when we are still in the pre-release phase. What is the worst thing that could happen? This change breaks an already broken keyboard editor? It's not like the keyboard editor is needed to run Joplin. Which in fact is the reason why I haven't tested the UI more extensively earlier, since I tested mostly with files.

[tessus]

I had to change the previous comment a few times. Sorry, it's just too early for me. ;-)

[laurent22]

I thought this was one of the reasons why we had the pre-releases.

No, pre-releases is for versions that we think are reasonably stable, it's not for untested code. Changes should be extensively tested pre-releases or not and ideally with unit tests. I don't like this giant hard-coded list in your update, but maybe that's the only way I don't know, that needs to be investigated.

Also you've posted a giant comment but completely ignored my question. Are there any libs that could do the job for us?

[tessus]

Are there any libs that could do the job for us?

I didn't find any.

No, pre-releases is for versions that we think are reasonably stable

I think my code is reasonably stable.

I don't like this giant hard-coded list

Neither do I. But after 2 days working on this (and I invested about 7-8 hours for researching, trying different things, and testing) I couldn't find a better way. I could remove all the empty ones and specify the set ones explicitly, but that's all there's to improve here.
I tried to put the array in an extra file and export it, but it didn't work. I got errors from tsc.

[laurent22]

I had a look at it again and the implementation makes sense. Electron seems to have specific shortcut names and it's not clear where they come from, so looks like a hard-coded list like is the way to go. I might move it to a separate file at some point. Thanks for the fix!

[tessus]

I might move it to a separate file at some point.

Yes, I wanted to do that, but couldn't make it work. It always complained file not found even though I properly imported it. I am not good with the js toolchain, so I most likely missed something.