feat(workspace): add drag and drop support to EditorView #1759

zbaylin · 2020-05-13T19:35:05Z

If you drag multiple files onto the editor view, it will check
all the paths to make sure they're files (not dir, socket, link, etc.)
If they are files, it will dispatch an OpenFileByPath action, which
will/should open the file in the editor!

Todo:

Investigate multi-file drop (@bryphe )

Test Cases:

Single file drop on Windows (dev build)
Single file drop on Windows (packaged build)
Single file drop on OSX (dev build)
Single file drop on OSX (packaged build)
Single drop on Linux

If you drag multiple files onto the editor view, it will check all the paths to make sure they're files (not dir, socket, link, etc.) If they are files, it will dispatch an OpenFileByPath action, which will/should open the file in the editor!

glennsl

Cool! I think the general approach is good as well, but the implementation needs some architectural work.

I've also changed the title, as the idea of the scope/area part of the title is to provide a general category for the changelog entry so it can be filtered on and such. drag-and-drop is waaay too specific to be useful for that purpose. The contribution guide suggests using the area issue labels or the name of a feature project.

glennsl · 2020-05-13T20:13:20Z

src/UI/EditorView.re

+      let stats = Unix.stat(path);
+      // This tests if the path dropped is a file. We can't handle opening directories, etc. yet
+      if (stats.st_kind == S_REG) {
+        GlobalContext.current().dispatch(
+          Model.Actions.OpenFileByPath(path, None, None),
+        );
+      };


side-effects should not be triggered in the UI, as that makes it difficult to track and control what's happening, and to evolve the logic when we also need to update the state, for example. In short, it leads to spaghetti code. The rule of thumb is that the UI should only dispatch messages to be handled by stores/update functions, which then update the model and emit side-effects through Isolinear.Effect.

But even more generally, even in imperative-land, doing blocking I/O on the UI thread is considered a big no-no as it blocks the UI from rendering and in some cases can really hurt responsiveness.

Yeah that's my bad -- I kinda just quickly worked on this to get it to somewhat work. I should have added a WIP tag on the PR generally.

Thanks for the info!

zbaylin · 2020-05-14T14:07:16Z

@glennsl I re-architected this to what (I think) is more in line with the Isolinear/Oni architecture as a whole. Let me know if I messed anything up.

And thanks again to @bryphe for walking me through the architecture last night!

zbaylin · 2020-05-14T14:13:50Z

As a side note, I'm getting weird behavior with dropping multiple files that I obviously would like to fix before this gets merged. If I drop two files and then try to close one of them, it just swaps positions with the other in the tabs.

Probably easier to just watch what I mean:

You can't really tell from the framerate of the GIF but the log in the back is also going crazy -- also not sure why.

All I'm doing is dispatching, in this case, 2 OpenFileByPath Actions, so I'm not exactly sure why that would cause this behavior.

src/Feature/FileDrop/dune

bryphe · 2020-05-14T14:41:58Z

src/Feature/FileDrop/Feature_FileDrop.re

@@ -0,0 +1,3 @@
+[@deriving show({with_path: false})]


This might seem small for now, but this is the perfect place for us to add a model once we incorporate dragging 👍

I have to admit I'm a bit more concerned. Not so much because of the granularity, although that is a minor concern too, but more because this seems to be a cross-cutting concern. It's not clear to me how this will integrate with other features that need to support drag and drop. Why would dropping a file on the file explorer, for example, even need to involve a separate file drop feature project? It seems like very little code would live here, but would instead have to be implemented in the consuming feature projects (file explorer/workspace) or in the glue code, as is largely the case here.

I can see that there's a missing feature project here for this specific feature to live in however, but I think that missing project is for the workspace as a whole.

Not so much because of the granularity, although that is a minor concern too, but more because this seems to be a cross-cutting concern. It's not clear to me how this will integrate with other features that need to support drag and drop. Why would dropping a file on the file explorer, for example, even need to involve a separate file drop feature project?

Right, I think this is a fair concern! It seems like - at least the way this is implemented right now - FilesDroppedOnEditor - makes sense to be part of the Feature_Editor (since it's handling the use-case where a file is dropped on the editors, and not on the explorer or other parts of the shell, at the moment).

The main issue is that Editor is one of our least-factored features at the moment - we don't have a central msg or updater yet.

We could start a Feature_Editor.msg with FilesDropped, and stub out an updater - and then incrementally move the remaining functionality towards that (ie, there are lots of Editor* actions in Actions.t that could be migrated to this msg later).

src/Store/Features.re

bryphe · 2020-05-14T14:46:03Z

Looking good to me, @zbaylin ! Awesome work.

All I'm doing is dispatching, in this case, 2 OpenFileByPath Actions, so I'm not exactly sure why that would cause this behavior.

I'm doing some work in this area today, so I'll investigate this in a bit. Your code seems fine, so I think it's exposing / hitting a bug.

One thing we should think about are the set of test cases we should try to make sure this is ready-to-go - some tests I can think of:

Single file drop on Windows
Single file drop on OSX
Single drop on Linux

And then a couple other thoughts:

How does this work with Zen mode? Does it switch to the last file, or should it leave zen mode? 🤔
For now, we just ignore directories, but maybe as a next step we could look at changing directories / workspaces, if just a single folder is dragged.

zbaylin · 2020-05-14T14:50:50Z

I'm doing some work in this area today, so I'll investigate this in a bit. Your code seems fine, so I think it's exposing / hitting a bug.

Sounds good. Thanks for that.

One thing we should think about are the set of test cases we should try to make sure this is ready-to-go - some tests I can think of:

I can test on Linux. Unfortunately I can't get Oni to build on my Windows VM for various reasons.

For now, we just ignore directories, but maybe as a next step we could look at changing directories / workspaces, if just a single folder is dragged.

VSCode switches workspaces on a single folder being dragged. I'm not sure what the behavior is for multiple folders, or a folder mixed in with files. I can look into that today.

bryphe · 2020-05-14T14:55:40Z

I can test on Linux. Unfortunately I can't get Oni to build on my Windows VM for various reasons.

Ok - I added a list of test cases at the top so we get a nice progress bar on the PR to track whats left:

I'll test Windows for you - and seems like OSX is already covered. Are there any other test cases you can think of that we need to check?

VSCode switches workspaces on a single folder being dragged. I'm not sure what the behavior is for multiple folders, or a folder mixed in with files. I can look into that today.

Awesome, that'd be great! We can implement this in a separate PR, so it's not blocking this work. I'm thinking we may want another msg to model this, like FolderDropped

zbaylin · 2020-05-14T14:59:40Z

Not for this PR's scope -- but eventually we'll want to expand this to the file tree. I can't think of any more specific tests for this PR. If and when you figure out that bug with multiple files we can test that, but other than that I can't think of anything.

bryphe · 2020-05-14T15:01:50Z

Sounds great @zbaylin !

bryphe · 2020-05-14T15:05:33Z

One other test case I think of is verifying on the packaged application (just in case there's something busted with the Info.plist metadata - I added it to the list). Let me know if you need any help creating a packaged build via esy create-release

zbaylin · 2020-05-14T15:28:33Z

@bryphe Linux works!

GIF:

zbaylin · 2020-05-14T15:32:24Z

Same with macOS packaged!

GIF:

glennsl · 2020-05-14T15:59:42Z

You can't really tell from the framerate of the GIF but the log in the back is also going crazy -- also not sure why.

That sounds a lot like the issue I had with opening both the update screen and welcome screen initially. It would go into some kind of infinite BufferEnter loop. Solved it by just replacing the welcome screen instead, which is less than ideal, so it'd be cool if fixing this bug solved that as well.

bryphe · 2020-05-14T16:05:03Z

That sounds a lot like the issue I had with opening both the update screen and welcome screen initially

Definitely @glennsl - I'm working through cleaning up that codepath today, to reduce the awkward timing dependencies:

and then:

Remove some imperative BufferEnter dependencies
Remove the awkward RecalculateEditorView action
Merge in refactor(vim): remove synchronization effects in VimStoreConnector #1627

Hoping to fix the multiple-buffer-open issue, along with these issues today:

bryphe · 2020-05-14T16:06:34Z

It might take a bit of time to work through it - sorry that you're both blocked on this!

In the meantime, @zbaylin - a couple options:

We could restrict this PR to just a single file, and merge as-is, and add in the multiple-file behavior once I've worked those open file issues
You could switch gears to another piece of work (like the changelog links?), and we could merge this to master once I've finished that set of work and the multiple-file-open is unblocked

zbaylin · 2020-05-14T16:29:30Z

@bryphe I'm fine leaving it until the multiple files bug is fixed. I can help @glennsl with the changelog in the meantime!

glennsl · 2020-05-14T16:37:09Z

src/Feature/FileDrop/Feature_FileDrop.re

@@ -0,0 +1,3 @@
+[@deriving show({with_path: false})]


I have to admit I'm a bit more concerned. Not so much because of the granularity, although that is a minor concern too, but more because this seems to be a cross-cutting concern. It's not clear to me how this will integrate with other features that need to support drag and drop. Why would dropping a file on the file explorer, for example, even need to involve a separate file drop feature project? It seems like very little code would live here, but would instead have to be implemented in the consuming feature projects (file explorer/workspace) or in the glue code, as is largely the case here.

I can see that there's a missing feature project here for this specific feature to live in however, but I think that missing project is for the workspace as a whole.

glennsl · 2020-05-14T16:52:57Z

src/Store/Features.re

+        Isolinear.Effect.createWithDispatch(
+          ~name="feature.filedrop.openFiles", dispatch => {
+          List.iter(
+            path => {
+              let stats = Unix.stat(path);
+              if (stats.st_kind == S_REG) {
+                dispatch(OpenFileByPath(path, None, None));
+              };
+            },
+            paths,
+          )
+        })


Ir would be nice to have this effect moved out of the update function, to avoid it turning into a blob. I also think ti does too much. Ideally this would be two separate effects, one for the stat, which ought to be non-blocking and dispatch a new user-supplied message when done, and then another to open the file. That would also enable the stat effect to be moved into the feature project, while the open file effect would need to be an outmsg because it's hard-coded to Actions.t for historic reasons.

This area is still a bit murky with a lot of historical baggage and few good examples of how to do these things properly, so don't worry too much about it, but the general lesson here is that supplying the message to be dispatched when it is invoked allows effects to be generalized.

Here's one example of such an effect:

oni2/src/Store/FileExplorerStore.re

Lines 11 to 25 in d971dae

let load = (directory, languageInfo, iconTheme, configuration, ~onComplete) => {

Isolinear.Effect.createWithDispatch(~name="explorer.load", dispatch => {

let ignored =

Configuration.getValue(c => c.filesExclude, configuration);

let tree =

FileExplorer.getDirectoryTree(

directory,

languageInfo,

iconTheme,

ignored,

);

dispatch(onComplete(tree));

});

};

Used here:

oni2/src/Store/FileExplorerStore.re

Lines 52 to 59 in d971dae

Effects.load(

lastNode.path,

state.languageInfo,

state.iconTheme,

state.configuration,

~onComplete=node =>

Actions.FileExplorer(FocusNodeLoaded(node))

),

And another:

oni2/src/Feature/SCM/Feature_SCM.re

Lines 65 to 74 in d971dae

let getOriginalUri = (extHostClient, model, path, toMsg) => {

let handles =

model.providers |> List.map((provider: Provider.t) => provider.handle);

ExtHostClient.SCM.Effects.provideOriginalResource(

~handles,

extHostClient,

path,

toMsg,

);

};

Used here:

oni2/src/Store/ExtensionClientStoreConnector.re

Lines 246 to 249 in d971dae

Feature_SCM.Effects.getOriginalUri(

extHostClient, state.scm, path, uri =>

Actions.GotOriginalUri({bufferId: metadata.id, uri})

),

@bryphe and I worked this out last night and I think the consensus we came to was that eventually the drag and drop feature will have it's own state once the drag part is implemented. I.e. if you held a dragged file over a certain area for x amount of time, something happens, etc. I'm obviously not as familiar with the underlying architecture as the two of you are, so it's possible @bryphe is able to explain better than I am.

I understand what you mean with regards to separating the actions -- I can do that. Thanks for the examples!

bryphe · 2020-05-15T23:46:54Z

I can see that there's a missing feature project here for this specific feature to live in however, but I think that missing project is for the workspace as a whole.

@glennsl - do you think we should move this to a feature project like Feature_Workspace? In discussing we figured the state that we managed would potentially be a drag state for the gesture (ie, the drag events Zach implemented here: revery-ui/esy-sdl2#13

Ideally this would be two separate effects, one for the stat, which ought to be non-blocking and dispatch a new user-supplied message when done, and then another to open the file

So something like Feature_Workspace.Effects.stat: (~path: string, ~mapResult: StatResult => 'msg) => Isolinear.Effect.t('msg)?

glennsl · 2020-05-16T08:29:42Z

do you think we should move this to a feature project like Feature_Workspace?

Yeah, I think that would be more natural. You'd have a hierarchy of features, akin to a hierarchy of components, with the File Explorer as a sub-feature. With ownership of a piece of state at the highest level necessary in the hierarchy.

Having state "on-the-side" leads to unclear ownership I think.

So something like Feature_Workspace.Effects.stat: (~path: string, ~mapResult: StatResult => 'msg) => Isolinear.Effect.t('msg)?

I would say more like:

Service_Filesystem.stat: (~path: string, ~onResult: StatResult => 'msg) => Isolinear.Effect.t('msg)

That is, moved to a service project, or even an external library, and using ~onResult instead of ~mapResult as that suggests more strongly that the 'msg will be dispatched.

…g-drop/open-file

bryphe · 2020-05-26T16:47:28Z

src/Service/OS/Service_OS.rei

-module Effect: {let openURL: string => Isolinear.Effect.t(_);};
+module Effect: {
+  let openURL: string => Isolinear.Effect.t(_);
+  let stat: (string, Unix.stats => 'msg) => Isolinear.Effect.t('msg);


Thanks for the refactoring here, Zach!

glennsl changed the title ~~feat(drag-and-drop): add drag and drop support to EditorView~~ feat(workspace): add drag and drop support to EditorView May 13, 2020

glennsl reviewed May 13, 2020

View reviewed changes

zbaylin added A-workspace Area: Overall UX, editor layout, tabs/groups/splits etc. enhancement New feature or request WIP labels May 13, 2020

Feature_FileDrop: Created FileDrop feature with msg

4e3f3ee

bryphe reviewed May 14, 2020

View reviewed changes

src/Feature/FileDrop/dune Outdated Show resolved Hide resolved

bryphe reviewed May 14, 2020

View reviewed changes

src/Store/Features.re Outdated Show resolved Hide resolved

glennsl reviewed May 14, 2020

View reviewed changes

bryphe added 2 commits May 15, 2020 16:33

Merge branch 'master' into feat/drag-drop/open-file

718199c

Merge branch 'master' into feat/drag-drop/open-file

97d500b

Merge branch 'master' of /~https://github.com/onivim/oni2 into feat/dra…

bbdd475

…g-drop/open-file

zbaylin added 3 commits May 26, 2020 12:11

Service_OS: add stat effect

9e38c89

Service_OS: remove unneeded stats variable

1ee83a2

Refactor: move Feature_FileDrop -> Feature_Editor

35a4832

bryphe reviewed May 26, 2020

View reviewed changes

bryphe approved these changes May 26, 2020

View reviewed changes

Editor: "FilesDroppedOnEditor" -> "FilesDropped"

88b4b71

bryphe approved these changes May 26, 2020

View reviewed changes

zbaylin merged commit 4c63224 into master May 26, 2020

bryphe mentioned this pull request May 30, 2020

[Feature Request] Drag and Drop Support for Opening Files #586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(workspace): add drag and drop support to EditorView #1759

feat(workspace): add drag and drop support to EditorView #1759

zbaylin commented May 13, 2020 •

edited by bryphe

Loading

glennsl left a comment

glennsl May 13, 2020 •

edited

Loading

zbaylin May 13, 2020

zbaylin commented May 14, 2020

zbaylin commented May 14, 2020

bryphe May 14, 2020

glennsl May 14, 2020

bryphe May 26, 2020

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

bryphe commented May 14, 2020

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

zbaylin commented May 14, 2020

glennsl commented May 14, 2020

bryphe commented May 14, 2020 •

edited

Loading

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

glennsl May 14, 2020

glennsl May 14, 2020

glennsl May 14, 2020

zbaylin May 14, 2020

bryphe commented May 15, 2020

glennsl commented May 16, 2020

bryphe May 26, 2020

	let load = (directory, languageInfo, iconTheme, configuration, ~onComplete) => {
	Isolinear.Effect.createWithDispatch(~name="explorer.load", dispatch => {
	let ignored =
	Configuration.getValue(c => c.filesExclude, configuration);
	let tree =
	FileExplorer.getDirectoryTree(
	directory,
	languageInfo,
	iconTheme,
	ignored,
	);

	dispatch(onComplete(tree));
	});
	};

	Effects.load(
	lastNode.path,
	state.languageInfo,
	state.iconTheme,
	state.configuration,
	~onComplete=node =>
	Actions.FileExplorer(FocusNodeLoaded(node))
	),

	let getOriginalUri = (extHostClient, model, path, toMsg) => {
	let handles =
	model.providers \|> List.map((provider: Provider.t) => provider.handle);
	ExtHostClient.SCM.Effects.provideOriginalResource(
	~handles,
	extHostClient,
	path,
	toMsg,
	);
	};

	Feature_SCM.Effects.getOriginalUri(
	extHostClient, state.scm, path, uri =>
	Actions.GotOriginalUri({bufferId: metadata.id, uri})
	),

feat(workspace): add drag and drop support to EditorView #1759

feat(workspace): add drag and drop support to EditorView #1759

Conversation

zbaylin commented May 13, 2020 • edited by bryphe Loading

glennsl left a comment

Choose a reason for hiding this comment

glennsl May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbaylin commented May 14, 2020

zbaylin commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

bryphe commented May 14, 2020

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

zbaylin commented May 14, 2020

glennsl commented May 14, 2020

bryphe commented May 14, 2020 • edited Loading

bryphe commented May 14, 2020

zbaylin commented May 14, 2020

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

bryphe commented May 15, 2020

glennsl commented May 16, 2020

Choose a reason for hiding this comment

zbaylin commented May 13, 2020 •

edited by bryphe

Loading

glennsl May 13, 2020 •

edited

Loading

bryphe commented May 14, 2020 •

edited

Loading