Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
May 26, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3bb0cdd
EditorView: add basic drag and drop to the editor
zbaylin May 13, 2020
4e3f3ee
Feature_FileDrop: Created FileDrop feature with msg
zbaylin May 14, 2020
718199c
Merge branch 'master' into feat/drag-drop/open-file
bryphe May 15, 2020
97d500b
Merge branch 'master' into feat/drag-drop/open-file
bryphe May 15, 2020
bbdd475
Merge branch 'master' of /~https://github.com/onivim/oni2 into feat/dra…
zbaylin May 26, 2020
9e38c89
Service_OS: add stat effect
zbaylin May 26, 2020
1ee83a2
Service_OS: remove unneeded `stats` variable
zbaylin May 26, 2020
35a4832
Refactor: move Feature_FileDrop -> Feature_Editor
zbaylin May 26, 2020
88b4b71
Editor: "FilesDroppedOnEditor" -> "FilesDropped"
zbaylin May 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Feature/FileDrop/Feature_FileDrop.re
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[@deriving show({with_path: false})]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

type msg =
| FilesDroppedOnEditor({paths: list(string)});
10 changes: 10 additions & 0 deletions src/Feature/FileDrop/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
(library
(name Feature_FileDrop)
bryphe marked this conversation as resolved.
Show resolved Hide resolved
(public_name Oni2.feature.filedrop)
(libraries
Oni2.core
isolinear
base
Revery
)
(preprocess (pps ppx_let ppx_deriving.show brisk-reconciler.ppx)))
1 change: 1 addition & 0 deletions src/Model/Actions.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ type t =
message: string,
extensionId: option(string),
})
| FileDrop(Feature_FileDrop.msg)
| FileExplorer(FileExplorer.action)
| LanguageFeature(LanguageFeatures.action)
| QuickmenuShow(quickmenuVariant)
Expand Down
3 changes: 2 additions & 1 deletion src/Model/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
(library
(name Oni_Model)
(public_name Oni2.model)
(libraries
(libraries
str
bigarray
zed_oni
Expand All @@ -23,6 +23,7 @@
Oni2.service.file-watcher
Oni2.components
Oni2.feature.configuration
Oni2.feature.filedrop
Oni2.feature.theme
Oni2.feature.notification
Oni2.feature.commands
Expand Down
19 changes: 19 additions & 0 deletions src/Store/Features.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,25 @@ let update =
| None => (state, Effect.none)
}

| FileDrop(msg) =>
let eff =
switch (msg) {
| FilesDroppedOnEditor({paths}) =>
Isolinear.Effect.createWithDispatch(
~name="feature.filedrop.openFiles", dispatch => {
List.iter(
path => {
let stats = Unix.stat(path);
bryphe marked this conversation as resolved.
Show resolved Hide resolved
if (stats.st_kind == S_REG) {
dispatch(OpenFileByPath(path, None, None));
};
},
paths,
)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})
),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

};
(state, eff);

// TODO: This should live in the editor feature project
| EditorFont(Service_Font.FontLoaded(font)) => (
{...state, editorFont: font},
Expand Down
3 changes: 2 additions & 1 deletion src/Store/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
(library
(name Oni_Store)
(public_name Oni2.store)
(libraries
(libraries
Rench
Revery
yojson
Expand All @@ -22,6 +22,7 @@
Oni2.components
Oni2.ui
Oni2.feature.commands
Oni2.feature.filedrop
Oni2.feature.menus
Oni2.feature.notification
Oni2.feature.theme
Expand Down
23 changes: 13 additions & 10 deletions src/UI/EditorView.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,19 @@ module Styles = {
];
};

let onFileDropped = ({paths, _}: NodeEvents.fileDropEventParams) =>
GlobalContext.current().dispatch(
FileDrop(FilesDroppedOnEditor({paths: paths})),
);

let make = (~state: State.t, ~theme, ()) =>
if (state.zenMode) {
<View style={Styles.container(theme)}>
{switch (EditorGroups.getActiveEditorGroup(state.editorGroups)) {
<View onFileDropped style={Styles.container(theme)}>
{if (state.zenMode) {
switch (EditorGroups.getActiveEditorGroup(state.editorGroups)) {
| Some(editorGroup) => <EditorGroupView state theme editorGroup />
| None => React.empty
}}
</View>;
} else {
<View style={Styles.container(theme)}>
<EditorLayoutView state theme />
</View>;
};
};
} else {
<EditorLayoutView state theme />;
}}
</View>;