Desktop: Add swapLineUp and swapLineDown keys #3363
Conversation
| @@ -26,6 +25,8 @@ import 'codemirror/mode/clike/clike'; | |||
| import 'codemirror/mode/diff/diff'; | |||
| import 'codemirror/mode/sql/sql'; | |||
|
|
|||
| const CodeMirror = require('codemirror'); | |||
There was a problem hiding this comment.
This was changed by the autoformatter. I don't personally like having it below all the other imports, but it works fine.
There was a problem hiding this comment.
Any reason why this package is imported using require() and the other ones using import? Maybe the solution is to change this to use import too?
There was a problem hiding this comment.
It's an issue with how CodeMirror is set up to work with the imports. I can't import using that syntax unless we add codemirror type checking (which I will make a pull request to address soon) and add the "esModuleInterop" flag to tsconfig.json.
I'm not too experienced with typescript so I didn't spend too long fussing around with it.
There was a problem hiding this comment.
yes, I think we need an exception for ElectronClient/gui/NoteEditor/NoteBody/CodeMirror/Editor.tsx. It currently breaks all CI builds.
| @@ -26,6 +25,8 @@ import 'codemirror/mode/clike/clike'; | |||
| import 'codemirror/mode/diff/diff'; | |||
| import 'codemirror/mode/sql/sql'; | |||
|
|
|||
| const CodeMirror = require('codemirror'); | |||
There was a problem hiding this comment.
Any reason why this package is imported using require() and the other ones using import? Maybe the solution is to change this to use import too?
| 'Cmd-/': 'toggleComment', | ||
| 'Cmd-Opt-S': 'sortSelectedLines', |
There was a problem hiding this comment.
I prefer if there's not too many shortcuts because we aren't going to document them, and they can cause problems when there are accidentally pressed. These two shortcuts in particular we could do without. In fact I'm not even sure we should have swap lines too, as we can't (nor should try to) replicate the full functionalities of a proper text editor, and there are drawbacks trying to achieve that.
There was a problem hiding this comment.
In fact I'm not even sure we should have swap lines too, as we can't (nor should try to) replicate the full functionalities of a proper text editor, and there are drawbacks trying to achieve that.
@laurent22 This came up only, because you stated in one comment that you plan to sunset the Ace editor. This functionality is essential for working with lists to re-arrange items.
Joplin is a note-taking application and working with lists is a big part of that. Re-arranging list items is paramount.
Toggle comment can be very useful (I've used it several times, but I could live without it), sorting lines is something one can do with an external editor (but if it was available in Ace, people will be used to using it).
I don't want to be negative, but you will face a huge backlash from people when you remove the Ace editor without making the key components available to whatever it is replaced with - in this case Code Mirror.
People are used to a certain workflow and it will only hurt this project when you remove functionality.
There was a problem hiding this comment.
I'd have to disagree as all these features have never been officially supported, so we have no requirement to keep them working in future versions. They might break or indeed disappear.
If some users are not happy (and what metrics do we have, are you sure it's not just you?), then why would they blame the developers when the developers never officially supported the feature. It's like if you fork the app, then complain that such or such internal feature has changed and broke your fork - that's just not the developer's responsibility.
There was a problem hiding this comment.
these features have never been officially supported, so we have no requirement to keep them working in future versions. They might break or indeed disappear.
I think you're correct to say this, but in this case I think tessus is right in that this is a useful feature.
I think the root problem here is that we aren't documenting hotkeys.
The current system that we have is to have certain hotkeys disabled if they overlap with joplins defined hotkeys. I copied this behavior over from the old aceeditor. Unfortunately that means that we still have a number of implicitly available functions in the editor. This was a problem with the old AceEditor and it's a problem with the new codemirror editor (albeit less so because codemirror has less default hotkeys).
What we should instead do is start with an empty keymap and add only features (hotkeys) that we want to support, this can be tied in with @anjulalk work on hotkeys for the system wide app, by adding a section for editor commands. This means that we can officially support useful editor features (like swapLineUp and swapLineDown) in a way that doesn't conflict with internal hotkeys.
Laurent if you approve, I'd like to get this merged (I'm biased but I think this is a useful pull request). And then I'll follow up with a pull request that replaces the code mirror keymap with only functions that we explicitly support. In the future this would then be integrated with the anjulalk's hotkey stuff.
There was a problem hiding this comment.
as all these features have never been officially supported
I didn't know that ALL of them were unsupported.
and what metrics do we have, are you sure it's not just you?
I've read several topics and a lot of posts (not necessarily in topics that were created for a shortcut related issue/festure) on the forum which mentioned these things. The only shortcuts I always use are Cmd+Z (Undo) (many times per day), and the move line up/down (at least once a day). Without those my workflow would be seriously compromised.
There was a problem hiding this comment.
I think the first step would indeed be to make a list of keyboard shortcuts we explicitly support. And that list ideally should be short because no matter what we can't recreate Sublime Text or Atom, it's not the goal of the project.
In the meantime, let's merge this.
|
I will test it in my lunch break, unless I don't have one. In that case I will do it tonight. |
|
Yes, it works on macOS:
|
As requested by @tessus https://discourse.joplinapp.org/t/syntax-to-link-notes/5264/42?u=calebjohn
I also duplicated key values to make sure they were registered on MacOs ref
@tessus could you please test this and make sure it's also working on your mac?