Implement CM Drag & Drop #9584

marcelgerber · 2014-10-17T18:59:53Z

Implements https://trello.com/c/io24Byhm/910-drag-and-drop-to-move-selected-text-within-editor.
Adds a new pref dragDropText (which is off by default)

le717 · 2014-10-17T21:17:08Z

As if I were not already spoiled on multiple cursors, this will make coding so much better. :D
Did you check the status of #141 after enabling this (as the Trello card you linked refers to)?

marcelgerber · 2014-10-17T21:36:37Z

No, I can't repro that issue.

TomMalbran · 2014-10-18T02:55:29Z

YAY!!! Thanks!! I hope this can make it into 1.0, please :)

marcelgerber · 2014-10-25T23:14:38Z

I just made sure #141 and #3239 are not reproducible on this branch.

le717 · 2014-11-30T20:02:53Z

Is it too late to get this in 1.1?

marcelgerber · 2014-11-30T21:07:58Z

From my point of view, it's totally fine to merge it now. A test phase of ~7 days should be enough for testing (with various extensions). Also, the diff is actually quite small, so I don't see any problem there.

TomMalbran · 2014-12-06T05:45:05Z

Bumping. We want this in 1.1. :)

redmunds · 2014-12-08T22:17:26Z

src/editor/Editor.js

@@ -123,6 +125,7 @@ define(function (require, exports, module) {

    PreferencesManager.definePreference(CLOSE_BRACKETS,     "boolean", false);
    PreferencesManager.definePreference(CLOSE_TAGS,         "Object", { whenOpening: true, whenClosing: true, indentTags: [] });
+    PreferencesManager.definePreference(DRAG_DROP,          "boolean", true);


This new feature is enabled by default. New features are usually off by default so as to not update current behavior, but I can be swayed by public opinion. What does everyone else think?

I think that it should be enabled by default, since is not a visual feature, or something that changes any behaviour, it just enables a new action of drag and drop. Unless for some reason you were clicking and dragging, this feature shouldn't change anything, and every user that expects to be able to drag and drop text, will not need to change a preference, which are a bit hidden at this moment.

I support enabling this default. As a user, I would expect drag-and-drop to be enabled by default. Also what @TomMalbran said.

I haven't heard anyone disagree, so I guess it's ok for this new feature to be on by default.

+1 to be on by default.

redmunds · 2014-12-08T22:23:03Z

I seem to remember someone saying that dragging text within editor was not enabled because it conflicted with drag and drop of files or folders, but that seems to work ok. Does anyone know of any conflicts that may be created by this?

redmunds · 2014-12-08T22:27:47Z

When I drag text, I see a drag cursor, but I was also expecting to see a pointer cursor when hovering over selected text like ST3 and VS. Any reason that was not added?

marcelgerber · 2014-12-08T22:27:58Z

The file-dropping issue is simply fixed by the little event handling code added in this PR.

redmunds · 2014-12-08T22:29:36Z

Done with initial code review. It seems like there may be too many remaining questions to get this in for 1.1, so hopefully we'll get it in 1.2.

marcelgerber · 2014-12-09T20:35:50Z

Not showing the pointer cursor is the same thing CM does, and I think this is a CM design decision (or at least, it has to be done in CM).

redmunds · 2014-12-11T17:45:06Z

Not showing the pointer cursor is the same thing CM does, and I think this is a CM design decision (or at least, it has to be done in CM).

I'd prefer the cursor to change when over selected text, but I can't figure out how to do it with existing DOM. Does anyone else have any opinions about this? cc @larz0 @njx

njx · 2014-12-11T18:16:34Z

You should be able to do it by just making a CSS rule for the selection style that sets the cursor property. But I think we should keep it the way it is, since drag and drop of text is not the 80% use case, and the I-beam is critical for properly targeting the cursor if you don't want to drag and drop (but just want to change the selection to a cursor inside the existing selection). Also, keeping the I-beam seems to be the standard behavior on Mac in this case (tested in TextEdit as well as in native text controls within Chrome, both of which support drag and drop of text). If the standard behavior is different on Windows, we could make a Windows-specific CSS rule for this.

marcelgerber · 2014-12-11T18:22:33Z

So, I tested this with multiple Windows editors I have installed:

Sublime Text 2: Pointer cursor
Visual Studio Express 2013: Pointer cursor
Notepad++: Pointer cursor
SciTE: Pointer cursor
Atom: No drag & drop

So it looks like this is the de-facto standard on Windows.

njx · 2014-12-11T18:55:42Z

Ah, that makes sense. So yeah, let's make it a Windows-specific rule.

marcelgerber · 2014-12-11T19:35:47Z

@njx I can't get it to work either. Setting the style on .CodeMirror-selected doesn't work.

njx · 2014-12-11T20:50:12Z

Ah, it might be because the selection is drawn behind the text, so doesn't get the events. Probably worth asking Marijn for suggestions on how to fix it if we want to pursue this.

marcelgerber · 2014-12-11T22:47:53Z

@njx I asked him, and that's what I got:
https://twitter.com/marijnjh/status/543164059617857536
https://twitter.com/marijnjh/status/543166258842451968

I don't know, though, if it's worth using that addon.

njx · 2014-12-11T23:17:09Z

Seems fine to me if it doesn't degrade performance significantly.

marcelgerber · 2014-12-15T20:44:44Z

Even with the addon enabled I can't seem to get it to work. I managed to change the text color of selections, but cursor doesn't work for me, while it works perfectly well on the CM demo site.
Any help appreciated.

redmunds · 2014-12-19T23:42:37Z

@marcelgerber The problem seems to be that containing class .CodeMirror-lines has pointer-events: none;, so pointer-events: auto; needs to be added to new rule. For example:

.platform-win .CodeMirror-selectedtext {
    pointer-events: auto;
    cursor: default;
}

marcelgerber · 2014-12-20T00:06:41Z

Yeah, that worked (but the code is still kind of ugly ;) ).
There's still an issue though - while on selected non-text background (blank line), you still see the normal cursor.

redmunds · 2014-12-21T23:33:18Z

src/styles/brackets_codemirror_override.less

@@ -268,6 +268,11 @@ div.CodeMirror-cursors {
    }

    .CodeMirror-matchingtag { background: @matching-bracket; }
+
+    div.CodeMirror-selected {


Don't we want this on Windows only? (e.g. .platform-win div.CodeMirror-selected).

marcelgerber · 2014-12-22T14:05:58Z

The interesting thing is that, even with the latest commits, I don't always see the pointer cursor. If it doesn't work, I need to restart Brackets in order to get it to work again.

Also, I couldn't fix the "cursor-change-while-dragging" problem, even though I tried all the CSS pseudo classes (like :active, :focus).

In case we can't resolve these issues, I'd rather go with just showing the caret while hovering a selection, even though that's apparently not the OS default.

TomMalbran · 2014-12-23T22:04:23Z

@marcelgerber You might need to add an additional class to toggle the pointer, and remove it while dragging.

marcelgerber · 2014-12-29T19:53:09Z

I don't think that will fix the first issue.

redmunds · 2015-01-06T03:11:35Z

@marcelgerber

I don't think that will fix the first issue.

Are you referring to the "don't always see the pointer cursor" issue? If so, I haven't see that -- got recipe?

marcelgerber · 2015-01-06T13:42:04Z

I can almost reliably repro it like this:

Create a new selection in some file
Restart Brackets
Hover over the selection
If the cursor does change to cursor: default, go back to step 2

marcelgerber · 2015-01-06T13:55:16Z

I opened codemirror/codemirror5#3020 for this to go into CM directly (CM actually manages some other cursors, too, like the crosshair while holding down Alt and the Drag & Drop cursor).
Let's see how far we get.

redmunds · 2015-01-08T17:25:01Z

@marcelgerber

In case we can't resolve these issues, I'd rather go with just showing the caret while hovering a selection, even though that's apparently not the OS default.

I'm ok with merging "showing the caret while hovering a selection" for now and address the rest later. If nobody disagrees, let me know when that's ready for a final review. Note that it probably should be squashed.

marcelgerber · 2015-01-08T17:44:09Z

@redmunds Done & rebased. Ready for final review/merge.

redmunds · 2015-01-08T19:19:25Z

Looks good. Merging.

Implement CM Drag & Drop

redmunds · 2015-01-08T19:20:05Z

@marcelgerber Reminder: add new preference here

le717 · 2015-01-08T19:25:00Z

marcelgerber · 2015-01-22T16:25:46Z

Good news:
Marijn wrote an addon to show a different cursor on selection mouseover, so we can start using that in the next Release (I got a branch ready already 👍)
codemirror/codemirror5#3020 (comment)

RB-Lab · 2015-03-04T15:37:03Z

Just wanna mention that I recently updated Brackets to version 1.2 on my OS X (didn't try it on Linux yet) and had no this feature (I waited so much) enabled by default. In my preferences file there was no such preference. So I had to add it there manually and it calls dragDropText, not dragDrop as it said in the very first comment. Just in case somebody faced the same problem... (maybe there is some better place to say this, but link from Release Notes let me here)

marcelgerber · 2015-03-04T16:51:21Z

To avoid confusion, I just edited the first post to include the actual pref name and that it's off by default ;)

peterflynn · 2015-03-04T22:09:39Z

The release notes and the update notification also both make it clear you need to change a pref to enable this, and call out the pref by name...

sathyamoorthi · 2015-03-25T06:11:17Z

@marcelgerber I'm not sure whether you are aware of this issue. But drag and drop is not working with PHP files. I tried with js, html, txt, css, less and py files. All are working fine.

marcelgerber · 2015-03-25T14:49:03Z

@sathyamoorthi It definitely works for me, even in PHP files. Could you try it again with extensions disabled?

peterflynn · 2015-03-25T21:59:22Z

@sathyamoorthi Can you file a new bug so we can track this? This PR is already closed.

sathyamoorthi · 2015-03-26T04:31:52Z

@marcelgerber Yes. It is working fine without extensions. Thx.

ingorichter added F Editor needs review labels Oct 25, 2014

ingorichter removed the needs review label Nov 17, 2014

redmunds self-assigned this Dec 8, 2014

redmunds reviewed Dec 8, 2014
View reviewed changes

redmunds added the PR Triage Complete label Dec 9, 2014

redmunds reviewed Dec 21, 2014
View reviewed changes

marcelgerber mentioned this pull request Jan 6, 2015

Show pointer cursor while hovering over a selection codemirror/codemirror5#3020

Closed

redmunds mentioned this pull request Jan 7, 2015

Suggestions - Drag and Drop text, line highlights, and drop target split view panes #10325

Open

Implement CM Drag & Drop

a8984d3

marcelgerber force-pushed the cm-drag-drop branch from 49b1181 to a8984d3 Compare January 8, 2015 17:43

redmunds added a commit that referenced this pull request Jan 8, 2015

Merge pull request #9584 from MarcelGerber/cm-drag-drop

47293e9

Implement CM Drag & Drop

redmunds merged commit 47293e9 into adobe:master Jan 8, 2015

marcelgerber deleted the cm-drag-drop branch January 8, 2015 19:23

peterflynn mentioned this pull request Feb 13, 2015

Clicking on multi-line selection inside inline editor jumps scroll position way downward #10590

Open

This was referenced Aug 30, 2021

[CLOSED] Implement CM Drag & Drop brackets-archive/bracketsIssues#8550

Open

Suggestions - Drag and Drop text, line highlights, and drop target split view panes brackets-archive/bracketsIssues#13015

Open

Implement CM Drag & Drop #9584

Implement CM Drag & Drop #9584

Conversation

marcelgerber commented Oct 17, 2014

le717 commented Oct 17, 2014

marcelgerber commented Oct 17, 2014

TomMalbran commented Oct 18, 2014

marcelgerber commented Oct 25, 2014

le717 commented Nov 30, 2014

marcelgerber commented Nov 30, 2014

TomMalbran commented Dec 6, 2014

redmunds Dec 8, 2014

Choose a reason for hiding this comment

TomMalbran Dec 8, 2014

Choose a reason for hiding this comment

le717 Dec 8, 2014

Choose a reason for hiding this comment

redmunds Dec 11, 2014

Choose a reason for hiding this comment

RaymondLim Dec 11, 2014

Choose a reason for hiding this comment

redmunds commented Dec 8, 2014

redmunds commented Dec 8, 2014

marcelgerber commented Dec 8, 2014

redmunds commented Dec 8, 2014

marcelgerber commented Dec 9, 2014

redmunds commented Dec 11, 2014

njx commented Dec 11, 2014

marcelgerber commented Dec 11, 2014

njx commented Dec 11, 2014

marcelgerber commented Dec 11, 2014

njx commented Dec 11, 2014

marcelgerber commented Dec 11, 2014

njx commented Dec 11, 2014

marcelgerber commented Dec 15, 2014

redmunds commented Dec 19, 2014

marcelgerber commented Dec 20, 2014

redmunds Dec 21, 2014

Choose a reason for hiding this comment

marcelgerber commented Dec 22, 2014

TomMalbran commented Dec 23, 2014

marcelgerber commented Dec 29, 2014

redmunds commented Jan 6, 2015

marcelgerber commented Jan 6, 2015

marcelgerber commented Jan 6, 2015

redmunds commented Jan 8, 2015

marcelgerber commented Jan 8, 2015

redmunds commented Jan 8, 2015

redmunds commented Jan 8, 2015

le717 commented Jan 8, 2015

marcelgerber commented Jan 22, 2015

RB-Lab commented Mar 4, 2015

marcelgerber commented Mar 4, 2015

peterflynn commented Mar 4, 2015

sathyamoorthi commented Mar 25, 2015

marcelgerber commented Mar 25, 2015

peterflynn commented Mar 25, 2015

sathyamoorthi commented Mar 26, 2015