fix #13945. support format on paste #18476

rebornix · 2017-01-12T23:36:37Z

Invoke format selection immediately after pasting event.

jrieken

Interesting approach. I was always thinking about a paste-listener and a setting but I actually like this. Left some nit comments, mostly around keybindings/menus

jrieken · 2017-01-13T11:26:03Z

src/vs/editor/contrib/format/common/formatActions.ts

+		super({
+			id: 'editor.action.formatOnPaste',
+			label: nls.localize('formatOnPaste.label', "Format on paste"),
+			alias: 'Format on paste',


Maybe 'Paste and Format'?

jrieken · 2017-01-13T11:28:03Z

src/vs/editor/contrib/format/common/formatActions.ts

+			id: 'editor.action.formatOnPaste',
+			label: nls.localize('formatOnPaste.label', "Format on paste"),
+			alias: 'Format on paste',
+			precondition: EditorContextKeys.Writable


We have context key which knows if for the current document there is someone that can format selections: ModeContextKeys.hasDocumentSelectionFormattingProvider (see FormatSelection as a sample). With that the menus will automatically update.

jrieken · 2017-01-13T11:29:12Z

src/vs/editor/contrib/format/common/formatActions.ts

+			id: 'editor.action.formatOnPaste',
+			label: nls.localize('formatOnPaste.label', "Format on paste"),
+			alias: 'Format on paste',
+			precondition: EditorContextKeys.Writable


Also register a keybinding and a menu item for this

rebornix · 2017-01-13T18:57:55Z

@jrieken addressed your comments. Bind key chord cmd+k cmd+v to this action as there aren't many choices for simple keybindings.

jrieken · 2017-01-16T09:30:05Z

On a second thought I am not so sure anymore if a separate action like 'Format and Paste' is the best solution. These are my concerns

Having two actions I wonder if the usage patterns would become confusing. I might be a formatter-fan and bind cmd+v to 'paste&format' just to learn that paste is defunct for languages that don't have a formatter. That would force users to know if a formatter is available and then use a different keybinding.
We already have settings like formatOnType and formatOnSave and I believe it would feel more natural to also have formatOnPaste

rebornix · 2017-01-17T20:47:59Z

@jrieken while implementing the command I have a feeling that ppl might prefer it as an option as well, I like formatOnPaste. I've already updated the code and follow the way we implement formatOnType.

jrieken · 2017-01-18T10:14:23Z

Code looks good to me. Waiting for @alexandrudima to review

alexdima

Looks good overall, just a few details left.

alexdima · 2017-01-19T15:37:45Z

src/vs/editor/common/commonCodeEditor.ts

+			this.cursor.trigger(source, handlerId, payload);
+			const endPosition = this.cursor.getSelection().getStartPosition();
+			if (source === 'keyboard') {
+				this.emit(editorCommon.EventType.DidPaste, {


I would prefer for the editor to always give out rich objects (i.e. Range and not IRange). It does so for all non-serializable events.

alexdima · 2017-01-19T15:38:39Z

src/vs/editor/common/editorCommon.ts

+	 * @event
+	 * @internal
+	 */
+	onDidPaste(listener: (range: Range) => void): IDisposable;


Here we say we give out a Range, but the implementation gives out an IRange. Please adjust the implementation.

alexdima · 2017-01-19T15:40:14Z

src/vs/editor/contrib/format/common/formatActions.ts

+			return;
+		}
+
+		var model = this.editor.getModel();


nitpick: prefer let over var (same two lines below)

alexdima · 2017-01-19T15:40:44Z

src/vs/editor/contrib/format/common/formatActions.ts

+
+		// no support
+		var [support] = OnTypeFormattingEditProviderRegistry.ordered(model);
+		if (!support || !support.autoFormatTriggerCharacters) {


No need for the support to have autoFormatTriggerCharacters. Perhaps a copy-paste error?

Now that I look at it, we should not read the OnTypeFormattingEditProvierRegistry, but the DocumentRangeFormatting bla bla Registry.

Yup it's a stupid copy-paste error, I made changes to one place but forgot it here.

alexdima · 2017-01-19T15:41:59Z

src/vs/editor/contrib/format/common/formatActions.ts

+			}
+			const command = new EditOperationsCommand(edits, this.editor.getSelection());
+			this.editor.executeCommand(this.getId(), command);
+			this.editor.focus();


No need to focus here, since we are not moving the focus anywhere.

rebornix · 2017-01-19T15:55:32Z

@alexandrudima comments addressed.

alexdima · 2017-01-19T16:56:17Z

Brilliant!

alexdima · 2017-01-23T16:45:00Z

@rebornix remember to create a test plan item for this

jrieken · 2017-01-23T17:02:27Z

@rebornix @alexandrudima i have done that

msftclas added the cla-not-required label Jan 12, 2017

rebornix requested review from jrieken and alexdima January 12, 2017 23:36

jrieken reviewed Jan 13, 2017

View reviewed changes

bpasero assigned alexdima Jan 13, 2017

rebornix added 2 commits January 13, 2017 10:46

fix microsoft#13945. support format on paste

a524360

update key bindings for format on paste

3f3ea06

rebornix force-pushed the FormatOnPaste branch from b8631fb to 3f3ea06 Compare January 13, 2017 18:56

formatOnPaste as editor option instead of actions

b9241a7

alexdima requested changes Jan 19, 2017

View reviewed changes

Address comments.

487c172

alexdima approved these changes Jan 19, 2017

View reviewed changes

alexdima merged commit abbd2a7 into microsoft:master Jan 19, 2017

alexdima added this to the January 2017 milestone Jan 23, 2017

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #13945. support format on paste #18476

fix #13945. support format on paste #18476

rebornix commented Jan 12, 2017

jrieken left a comment

jrieken Jan 13, 2017

jrieken Jan 13, 2017

jrieken Jan 13, 2017

rebornix commented Jan 13, 2017 •

edited

Loading

jrieken commented Jan 16, 2017

rebornix commented Jan 17, 2017

jrieken commented Jan 18, 2017

alexdima left a comment

alexdima Jan 19, 2017

alexdima Jan 19, 2017

alexdima Jan 19, 2017

alexdima Jan 19, 2017

alexdima Jan 19, 2017

rebornix Jan 19, 2017

alexdima Jan 19, 2017

rebornix commented Jan 19, 2017

alexdima commented Jan 19, 2017

alexdima commented Jan 23, 2017

jrieken commented Jan 23, 2017

fix #13945. support format on paste #18476

fix #13945. support format on paste #18476

Conversation

rebornix commented Jan 12, 2017

jrieken 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

rebornix commented Jan 13, 2017 • edited Loading

jrieken commented Jan 16, 2017

rebornix commented Jan 17, 2017

jrieken commented Jan 18, 2017

alexdima 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

rebornix commented Jan 19, 2017

alexdima commented Jan 19, 2017

alexdima commented Jan 23, 2017

jrieken commented Jan 23, 2017

rebornix commented Jan 13, 2017 •

edited

Loading