Drag and drop selection in editor #20565
Conversation
|
@rebornix, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @egamma to be potential reviewers. |
|
Now I already stop adding secondary cursors when drag and move. Instead I put a new widget to simulate the cursor, which is somehow not pixel perfect but not that ugly. The next step is moving the content cut and paste logic into that widget as well then our code is clean enough. |
rebornix
changed the title
|
The dnd now works pretty reasonably in the editor. The target cursor you see while dragging is actually virtual, which looks the same as a secondary cursor but not a real cursor. The catch here is I'm putting logic into Contrib part as I create content widget to mimic the cursor but in this way I have to listen to drag and drop related events. That's why I emit these two events in view controller but they are not 100% compatible with Browsers native drag/drop events (lack of dataTransfer). A proper way to make the data transfer work is adding the dataTransfer object to related events in CommonCodeEditor, where we can access the editor and the model. Then we maintain our own implementation of drag events. Look forward to Alex's comment on this @alexandrudima |
There was a problem hiding this comment.
Pretty good!
Some high-level observations:
- it should sit behind an option and be turned off by default
- drop outside the editor or drop on a view zone or drop on a widget, etc. all kind of targets except content or perhaps content_empty should result in a no-op
- the drop target should IMHO be rendered very differently than a regular cursor. I was confused by the drop target looking like a regular cursor.
- if we want to go on the path of the drop target looking like a regular cursor, then we must change the CSS
cursor:to something else, otherwise it is not clear that we are in a dnd operation
| @@ -346,8 +346,10 @@ class MouseDownOperation extends Disposable { | |||
|
|
|||
| private _mouseMoveMonitor: GlobalEditorMouseMoveMonitor; | |||
| private _mouseDownThenMoveEventHandler: EventGateKeeper<EditorMouseEvent>; | |||
| private _mouseDragThenMoveEventHandler: EventGateKeeper<EditorMouseEvent>; | |||
There was a problem hiding this comment.
No need for a separate handler, I suggest to use mouseState.lastMouseDownEvent (which should be renamed to something better, see other comment) to differentiate the two cases inside onMouseDownThenMove
| @@ -384,6 +386,12 @@ class MouseDownOperation extends Disposable { | |||
| () => !this._viewHelper.isDirty() | |||
| ) | |||
| ); | |||
| this._mouseDragThenMoveEventHandler = this._register( | |||
There was a problem hiding this comment.
No need for a separate handler, I suggest to code the dnd case inside onMouseDownThenMove
| @@ -407,12 +415,30 @@ class MouseDownOperation extends Disposable { | |||
| this._dispatchMouse(position, true); | |||
| } | |||
|
|
|||
| private _onMouseDragThenMove(e: EditorMouseEvent): void { | |||
There was a problem hiding this comment.
Merge into _onMouseDownThenMove and use mouseState.lastMouseDownEvent (which should be renamed to something better, see other comment) to differentiate the two cases
| private _onMouseDragThenMove(e: EditorMouseEvent): void { | ||
| this._lastMouseEvent = e; | ||
| this._mouseState.setModifiers(e); | ||
| this._mouseState.setMouseDownEvent('drag'); |
There was a problem hiding this comment.
this._mouseState.setMouseDownEvent('drag'); should move to the start method when the operation starts. The lastMouseDownEvent should be set only once when the operation starts inside start.
| && this._currentSelection.containsPosition(position.position) // single click on a selection | ||
| ) { | ||
| this._isActive = true; | ||
| this._dropTarget = this._createMouseTarget(e, true); |
There was a problem hiding this comment.
This is where this._mouseState.setMouseDownEvent('drag'); should be set
| let newCursorPosition = new Position(targetPosition.lineNumber, targetPosition.column); | ||
|
|
||
| if (this._dragSelection.containsPosition(newCursorPosition)) { | ||
| let newSelections = this._editor.getSelections().map(selection => { |
There was a problem hiding this comment.
this code can be simplified once we guard in the mouseHandler to only begin dnd if the editor has precisely one selection
| } | ||
| } | ||
|
|
||
| this._hideWidgets(); |
There was a problem hiding this comment.
a single widget => _hideWidget
| private _cursorStyle: TextEditorCursorStyle; | ||
| private _lineHeight: number; | ||
| private _typicalHalfwidthCharacterWidth: number; | ||
| private readonly _domNode: FastDomNode; |
There was a problem hiding this comment.
Sorry for the compile error, use FastDomNode<HTMLElement>
| this._domNode.setVisibility('hidden'); | ||
| this._editor.addContentWidget(this); | ||
|
|
||
| this._cursorStyle = this._editor.getConfiguration().viewInfo.cursorStyle; |
There was a problem hiding this comment.
I don't like that we reuse here the cursor style and the cursor CSS. It makes it difficult to make changes in the CSS and e.g. it makes no sense to use a block cursor to paint the insertion point.
IMHO the insertion point should always be rendered as a line, regardless of user settings. It is not a cursor, it is an insertion point.
On top, I would render the insertion point in a distinct way than the cursor. I got very confused when I tried it out, it looks too much like a cursor, it should be orange or dashed or something special to let me know that it represents the drop point and is not some editor bug where a cursor is painted.
It should also go away if the mouse is not sitting directly on text (mouseEvent.target !== CONTENT). If the drag is on top of a view zone, the margin, etc. there should be no drop. IMHO, only on top of content.
| this._domNode.setWidth(this._typicalHalfwidthCharacterWidth); | ||
| } | ||
|
|
||
| if (this._cursorStyle === TextEditorCursorStyle.Underline) { |
There was a problem hiding this comment.
I would use its own styles, doing this is nasty and to be honest, confused me a lot. I couldn't tell apart the cursor from the drop hint.
|
@alexandrudima I've made some updates to the code
Add
Right now drop on elements other than content or content_empty of current editor will lead to no-op. When you drag and move the cursor, we still try to draw the target hint in the editor, just like what we did for selection.
Now I drop dependency to Cursor and use its own CSS. The reason that I leveraged original cursor style is to make sure it looks reasonable in different third party themes. Right now I used simple dotted line to represent the drop target. About the comment you put in #20565 (comment) , the reason that I set
Right now we only update
In this case, MouseDrag and MouseDrop events are always emitted even if users just click on a selection. But it's not a big deal as we hide these events from external. I don't favor this part so you may want to change it somehow. Most comments are addressed, feel free to polish it :) |
|
Very very nice! |
Fix #1046.
Click and hold the left mouse key on a selection now can be monitored but the implementation of moving the selection is not good. I'm creating a secondary cursor to show where the text will be pasted to however it ruins our undo/redo stack as the secondary cursor is a real cursor.
The ideal experience would be similar to Hover. Draw the virtual cursor by decoration and draw a popup window with the text being moved.
TODO:
Verify: