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

Changes from 1 commit
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
32 changes: 22 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,28 @@ module Styles = {
];
};

let onFileDropped = (event: NodeEvents.fileDropEventParams) =>
List.iter(
path => {
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),
);
};
Copy link
Member

@glennsl glennsl May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

},
event.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>;