Update RSyntaxTextArea to 2.6.1 and remove need for Arduino-specific patches #5990
Conversation
Previously, custom Actions and ActionHandlers were used to handle undo, redo, cut, copy, paste and select all. However, RTextArea.getAction() exposes action objects for these things already. This commit makes use of them. This allows slightly shrinking buildEditMenu(), but the real gain is that RTextArea automatically takes care of keeping the state these actions correct (based on the most recently focused RTextArea), which allows removing some custom event handlers and other overhead. In particular, this helps to removes the Arduino-specific changes for RSyntaxTextArea later. As a side effect, this breaks these actions when the text area is not in focus. This will be fixed in a subsequent commit.
Now that RTextArea actions are used, this class no longer serves any purpose.
Since some edit actions are handled by RTextArea Action objects directly, they no longer worked when the main text area was not focused. This is because RecordableTextAction uses TextAction.getTextComponent to figure out what component to act on, which resolves to the event source (if it is a JTextComponent) or the most recently JTextComponent otherwise. In this case, it meant that if the error output component is focused, actions will not work. To fix that, the actions are wrapped to always act on the text area in the current tab, instead of relying on getTextComponent().
This directly calls beginInternalAtomicEdit() on the UndoManagr, but without needing RsyntaxTextArea to expose its UndoManager (which is an Arduino-specific patch).
This method is no longer used, and relies on an Arduino-specific patch to RSyntaxTextArea.
This imports the unmodified upstream version of RSTA from http://repo1.maven.org/maven2/com/fifesoft/rsyntaxtextarea/2.6.1/rsyntaxtextarea-2.6.1.jar since the Arduino-specific changes are no longer required. This fixes arduino#5888
It seems this class has been unused since the switch to RSyntaxTextArea was made.
|
✅ Build completed. Please test this code using one of the following: ⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5990-BUILD-654-linux32.tar.xz ℹ️ The |
|
Looks REALLY good to me. The cleanup is really nice, and I was somehow concerned about the translations/shortcuts but they all work well (on Linux and OSX at least). |
| new RTextArea(); | ||
|
|
||
| RecordableTextAction undoAction = RTextArea.getAction(RTextArea.UNDO_ACTION); | ||
| JMenuItem undoItem = new JMenuItem(new RstaActionWrapper(undoAction)); |
There was a problem hiding this comment.
The dummy object creation to obtain the UNDO_ACTION makes me really uneasy.
Also, we already have a EditorTab.handleUndo() method, why not just call that instead of trying to fit the RSTA action?
Moreover the RSTA action is wrapped inside another ActionWrapper class that is made just to route the message to the correct EditorTab through getCurrentTab(), while it may be much more simple to do something like:
undoItem.addActionListener(() -> getCurrentTab().handleUndo());Or maybe I'm missing something?
I remember that I've already solved this in another refactoring experiment I did some time ago I'll try to recover this attempt from my old branches.
There was a problem hiding this comment.
The reason to use RSTA's actions, is that they are automatically updated to become enabled/disabled when the undo state changes, and to automatically show "Undo" or "Can't undo". If we do that manually, as is done currently, we need access to the UndoManager which needs a patch (at least for the getUndoPresentationName(), IIRC RSTA does export a canUndo() or similar). Also, we need to keep track of changes within the document with change listeners and other stuff, which is a bit messy. Having said that, this approach isn't 100% elegant either, so I can understand your being hesitant.
There was a problem hiding this comment.
thanks for the clarification, inedeed the fact that RSTA handle automatically the enable/disable is a point.
Another thing that comes to mind is how the localization is handled? is it something already in the RSTA resources? about getUndoPresentationName() it seems that the menu is always presented as "Undo" or "Redo", IMHO it's something that can be easily dropped in case.
| @@ -1610,8 +1526,6 @@ public int getCurrentTabIndex() { | |||
| */ | |||
| public void selectTab(final int index) { | |||
| currentTabIndex = index; | |||
| undoAction.updateUndoState(); | |||
| redoAction.updateRedoState(); | |||
There was a problem hiding this comment.
Removing these calls produce the correct enable/disable of menu actions?
Something to check if this PR is merged.
There was a problem hiding this comment.
Yeah, RSTA handles this internally already.
|
I've already pushed just this commit into master: |
|
fixed by #6022 |
This applies some cleanups to the undo and copy-paste handling, that make things simpler and remove the need for modifying RSTA, allowing the upstream compiled jar files to be used. This PR can replace #5960.