provide separate paste, paste and indent commands #3596

kevinushey · 2018-10-04T17:03:27Z

This PR provides for separate [Paste] and [Paste and Reindent] commands. With the PR, Ctrl + V pastes without indent, while Ctrl + Shift + V pastes and reindents. The implementation works with both RStudio Server and RStudio Desktop.

Some open questions if we take this PR:

Do we want to change the default behavior so that Ctrl + V no longer reindents by default?
Should we keep the 'reindent on paste' UI preference, or instead document that one should use Ctrl + Shift + V if they do want to reindent after pasting?

jjallaire · 2018-10-04T17:08:42Z

I wonder if people have come to rely on the current behavior much more than they realize? (as it's extremely convenient that things are indented, and at the same time so subtle that you might not even realize it's happening).

So if we took this out from under people there might be a lot of unhappy users. Given this I'd probably prefer that re-indent remain the default and that the Shift variation do it w/o indent.

kevinushey · 2018-10-04T17:17:35Z

That was my concern as well, but I was thinking about having that since it would then match the behavior of other IDEs / editors. That said, perhaps it's best for us to retain the current behavior and allow a shift-paste to perform a paste without indent to avoid breaking user's muscle memory.

jjallaire · 2018-10-04T17:19:03Z

Yeah, I think users would be quite surprised at how much fixup they'd end up doing w/ a new default behavior.

kevinushey · 2018-10-04T18:06:03Z

I think this PR is good to go. The existing paste behavior is preserved, and one can now by default use Ctrl + Shift + V to paste without indent.

If the 'paste on indent' UI pref is deselected, then the behavior is reversed: Ctrl + V will paste without indent, while Ctrl + Shift + V will paste and reindent. I think this should hopefully make all camps of users happy.

jmcphers

Do we need this for 1.2?

jmcphers · 2018-10-04T18:26:23Z

src/gwt/src/org/rstudio/studio/client/workbench/commands/Commands.cmd.xml

@@ -787,6 +787,8 @@ well as menu structures (for main menu and popup menus).
         <shortcut refid="showOptions" value="Meta+," if="org.rstudio.core.client.BrowseCap.isMacintosh()"/>
         <shortcut refid="projectOptions" value="Shift+Meta+," if="org.rstudio.core.client.BrowseCap.isMacintosh()"/> 
         <shortcut refid="goToHelp" value="Ctrl+C Ctrl+V" disableModes="default,vim,sublime"/>
+         <shortcut refid="pasteAndIndent" value="Ctrl+Shift+V" if="!org.rstudio.core.client.BrowseCap.isMacintoshDesktop()"/>
+         <shortcut refid="pasteAndIndent" value="Meta+Shift+V" if="org.rstudio.core.client.BrowseCap.isMacintoshDesktop()"/>


Do we also want this on the Mac in server mode?

The intention here was to only ever register these commands in Desktop mode, as the server will 'just do the right thing' without these bindings. (We need to register them explicitly for desktop as otherwise the key combination is ignored)

Perhaps I should make this more clear by adding the notion of a 'desktop' vs. 'server' scoped command binding (as a new XML attribute). Note that combining conditions with && doesn't work as the XML parser complains :-/

jmcphers · 2018-10-04T18:26:50Z

src/gwt/src/org/rstudio/studio/client/workbench/views/source/editors/text/AceEditor.java

+                        activeEditEvent_.getType() == EditEvent.TYPE_PASTE_AND_INDENT ||
+                        monitor_.isShiftKeyDown();
+
+                  reindent ^= uiPrefs_.reindentOnPaste().getValue();


Elegant use of ^=!

I call it the 'flip-if' operator :)

jmcphers · 2018-10-04T21:58:49Z

src/gwt/src/org/rstudio/studio/client/workbench/views/source/editors/text/AceEditor.java

@@ -4082,6 +4105,7 @@ public void execute(double timestamp)
   private CollabEditor collab_;
   private Commands commands_;
   private EventBus events_;
+   private KeyboardMonitor monitor_;


Nit: could be final

jmcphers · 2018-10-04T21:59:42Z

src/gwt/src/org/rstudio/studio/client/workbench/views/source/editors/text/AceEditor.java

-                  Range range = Range.fromPoints(start, getSelectionEnd());
-                  indentPastedRange(range);
+                  boolean reindent =
+                        activeEditEvent_.getType() == EditEvent.TYPE_PASTE_AND_INDENT ||


Is there any risk at all of activeEditEvent_ or uiPrefs_ being null in this function (as paste will break if there is)?

activeEditEvent_ is set to a special 'none' event during initialization and is reset to that after the edit event completes, so it shouldn't ever be null. Similarly, uiPrefs_ is injected on initialization so shouldn't be null.

Great, that's very reassuring!

jmcphers · 2018-10-04T22:00:46Z

src/gwt/src/org/rstudio/studio/client/workbench/views/source/editors/text/AceEditor.java

@@ -341,13 +343,22 @@ public void onPaste(PasteEvent event)

            final Position start = getSelectionStart();

-            Scheduler.get().scheduleDeferred(new ScheduledCommand()
+            Scheduler.get().scheduleFinally(new ScheduledCommand()


How much time drift can occur between the time the paste event occurs and the time when the scheduler actually runs this? (wondering if we should sample the modifier key state at the time the event occurs rather than later)

scheduleFinally() basically ensures that the call is run at the end of the current event loop, whereas scheduleDeferred() runs it in the next event loop. This actually make sure the call is run before the current edit event completes, and also removes a visual 'flicker' where pasting would briefly show the un-indented text, and then quickly flash to the re-indented text.

Figuring out the active set of keys here is a bit tricky since Ace handles the actual paste event, then generates its own "paste" signal with the associated text data. For the reason, we do this dance with the KeyboardMonitor to see what modifiers were actually held during the time of paste. (In particular, the browser doesn't report the state of modifier keys as part of the paste event, so we need to explicitly track keydown / keyup events that occur to figure out what modifiers are actually down at the time of paste)

Yes, it feels a little weird that we have to be running code constantly in the background to get this right, but since Ace's paste signal doesn't include the originating event I realize it's hard to do better!

kevinushey · 2018-10-04T23:14:07Z

I don't think we need this for v1.2 (I can squash and cherry-pick to v1.3 once we're happy)

jmcphers · 2018-10-04T23:42:32Z

Yes, given that we're churning paste code (and that's historically been a real minefield, even though this seems pretty safe) I think we should target this for 1.3.

Do we also want Shift to invert the paste with/without indent behavior when pasting is performed with the mouse (via e.g. right-click -> paste, or middle click)?

kevinushey · 2018-10-08T18:47:38Z

Merged into v1.3.

provide separate paste, paste and indent commands

33e2e5b

kevinushey added 2 commits October 4, 2018 10:22

retain default indent on paste behavior

a3decd4

toggle Shift+Paste behavior based on reindentOnPaste pref

807e302

kevinushey force-pushed the bugfix/paste-and-indent branch from aee60dc to 807e302 Compare October 4, 2018 17:47

jmcphers reviewed Oct 4, 2018

View reviewed changes

add 'platform' attribute to commands parser

274780f

kevinushey force-pushed the bugfix/paste-and-indent branch from a689951 to 274780f Compare October 4, 2018 22:56

kevinushey changed the base branch from master to v1.3 October 8, 2018 18:47

kevinushey merged commit 9925fe5 into v1.3 Oct 8, 2018

jmcphers mentioned this pull request Oct 29, 2018

Copying tabulated text produces missaligned text. #3779

Closed

valerie-rstudio deleted the bugfix/paste-and-indent branch January 21, 2022 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provide separate paste, paste and indent commands #3596

provide separate paste, paste and indent commands #3596

kevinushey commented Oct 4, 2018

jjallaire commented Oct 4, 2018

kevinushey commented Oct 4, 2018

jjallaire commented Oct 4, 2018

kevinushey commented Oct 4, 2018

jmcphers left a comment

jmcphers Oct 4, 2018

kevinushey Oct 4, 2018

jmcphers Oct 4, 2018

kevinushey Oct 4, 2018

jmcphers Oct 4, 2018

jmcphers Oct 4, 2018

kevinushey Oct 4, 2018

jmcphers Oct 4, 2018

jmcphers Oct 4, 2018

kevinushey Oct 4, 2018

jmcphers Oct 4, 2018

kevinushey commented Oct 4, 2018

jmcphers commented Oct 4, 2018

kevinushey commented Oct 8, 2018

provide separate paste, paste and indent commands #3596

provide separate paste, paste and indent commands #3596

Conversation

kevinushey commented Oct 4, 2018

jjallaire commented Oct 4, 2018

kevinushey commented Oct 4, 2018

jjallaire commented Oct 4, 2018

kevinushey commented Oct 4, 2018

jmcphers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinushey commented Oct 4, 2018

jmcphers commented Oct 4, 2018

kevinushey commented Oct 8, 2018