Split View (Same Document) #11820

Merged
merged 16 commits into from Dec 11, 2015

Projects

None yet
@swmitra
Contributor
swmitra commented Oct 14, 2015

This feature enables users of Brackets to open same document in both the editor panes and also enables extension developers to create full editors for documents under different containers to make full use of the code hinting features already available for Brackets.

For more details about the feature , please refer to https://trello.com/c/GezHZcCx/390-m-split-view-same-document#

Feature Details

  • Same document will be used by all the full editors to seamlessly synchronize change sets.
  • Undo/Redo stack also gets synchronized ( although not same ).
  • When opened in both the panes , the same document gets listed in both the working sets.
  • The focused full editor acts as master editor for the document at any point of time.

Implementation

The core idea of this implementation is to handle 'masterEditor' tied to a document instance. 'Document' in the new implementation maintains a list of full editors and exposes private API to switch between full editors as master editor. Full editors who are tied to the document , marks itself as master editor when focused.


Pending Tasks

  • Fix existing unit tests.
  • Add new unit test for creation.
@abose abose added this to the Release 1.6 milestone Oct 14, 2015
@nethip
Contributor
nethip commented Oct 15, 2015

@swmitra Thanks for this PR. Very nice addition to Brackets!

@MarcelGerber @zaggino @sprintr @petetnt @peterflynn Could you guys take some time in reviewing this?

@nethip
Contributor
nethip commented Oct 15, 2015

Tagging @abose

@nethip
Contributor
nethip commented Oct 15, 2015

@swmitra Please fix the JSHint errors.

@petetnt
Member
petetnt commented Oct 15, 2015

Began reviewing this against the current master 40ad8e9

Most recently used (MRU)-list is partially broken

The _makeFileMostRecent-method in in view/MainViewManager.js#L328 is logging tons of C:/foo/bar.js duplicated in mru list-notifications, because _findFileInMRUList-method only checks for the file.fullPath, instead of that combined with the pane ID, same way as the index check does in view/MainViewManager.js#L337.

Same goes for example:

Until this is fixed, for example navigating the opened files with CTRL+Tab has some really unexpected behaviors.

Has some issues with my implementation of #11749

As this feature and my feature were done at the same time, these kinds of issues were to be expected 👯

Outside of the MRU-issues, this feature seems to work (from a 30 minute test session) very well. There are some issues in the flip-view scenario though:

  1. Open foo.js in pane1 //works
  2. Open foo.js in pane2 //works
  3. Flip foo.js#pane2 to pane1 //works
  4. Flip foo.js#pane1 to pane2

This occurs:

image

For some reason the file did get flipped, but the other pane ended up in this weird limbo state. The situation can be eventually recovered to normal.

The issue occurs in view/Pane.js#L249 so the culprit is those methods should be checked out (MainVIewManager._moveview, CommandManager.execute(Command.FILE_OPEN...) and anything that's listening to viewListChange-events.).

As I am the author of the other feature I am of course happy to help with that as soon as I have some more time 👍

PerfUtils issues?
 Recursive tests with the same id are not supported. Timer id: [reent 1] CodeInspection:    C:/Users/Pete/Desktop/foo/foo.js
PerfUtils.js:144 Recursive tests with the same id are not supported. Timer id: [reent 1] CodeInspection 'ESLint':   C:/Users/Pete/Desktop/foo/foo.js
MainViewManager.js:347 C:/Users/Pete/Desktop/foo/bundle.js duplicated in mru list
MainViewManager.js:347 C:/Users/Pete/Desktop/foo/bundle.js duplicated in mru list
JSHint issues
  1. Create foo.js
  2. Open foo.js in pane1 and 2
  3. write something ->
Exception in 'editorChange' listener on Editor TypeError: Cannot read property 'length' of undefined TypeError: Cannot read property 'length' of undefined
    at Object.maybeIdentifier (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/JavaScriptCodeHints/HintUtils.js:67:28)
    at Session.getQuery (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/JavaScriptCodeHints/Session.js:257:31)
    at JSHints.getHints (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/JavaScriptCodeHints/main.js:477:35)
    at _updateHintList (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/CodeHintManager.js:426:40)
    at _handleChange (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/CodeHintManager.js:618:17)
    at Editor.trigger (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/utils/EventDispatcher.js:222:40)
    at Editor._handleEditorChange (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/Editor.js:870:14)
    at file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/Editor.js:412:18
    at Editor.trigger (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/utils/EventDispatcher.js:222:40)
    at file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/Editor.js:941:18
    at CodeMirror.signal (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/thirdparty/CodeMirror/lib/codemirror.js:8112:49)
    at endOperation_finish (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/thirdparty/CodeMirror/lib/codemirror.js:3127:7)
    at endOperations (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/thirdparty/CodeMirror/lib/codemirror.js:3021:7)
    at endOperation (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/thirdparty/CodeMirror/lib/codemirror.js:3004:7)
    at runInOp (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/thirdparty/CodeMirror/lib/codemirror.js:3137:15)
    at TextareaInput.copyObj.poll (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/thirdparty/CodeMirror/lib/codemirror.js:1434:7)
@busykai busykai commented on an outdated diff Oct 15, 2015
src/document/Document.js
@@ -196,12 +201,15 @@ define(function (require, exports, module) {
*/
Document.prototype._makeEditable = function (masterEditor) {
if (this._masterEditor) {
- console.error("Document is already editable");
- } else {
- this._text = null;
- this._masterEditor = masterEditor;
- masterEditor.on("change", this._handleEditorChange.bind(this));
+ //Already a master editor is associated , so preserve the old editor in list of full editors
+ if(this._associatedFullEditors.indexOf(this._masterEditor) < 0){
@busykai
busykai Oct 15, 2015 Member

You need to use Brackets with JSLint enabled when contributing to Brackets. Please re-check your code. In this specific case, there's a space missing between if and ( which JSLint would complain about.

@busykai busykai commented on an outdated diff Oct 15, 2015
src/editor/Editor.js
@@ -961,6 +961,7 @@ define(function (require, exports, module) {
this._codeMirror.on("focus", function () {
self._focused = true;
self.trigger("focus", self);
+ self.document._toggleMasterEditor(self);//Added to toggle fulleditors as master editor
@busykai
busykai Oct 15, 2015 Member

Nit: missing space after // in comments. Here and multiple other places. Just look around to see what the comments should like.

@busykai busykai commented on an outdated diff Oct 15, 2015
src/document/Document.js
@@ -84,6 +84,11 @@ define(function (require, exports, module) {
EventDispatcher.makeEventDispatcher(Document.prototype);
/**
+ * list of editors who were initialized as master editors for this doc.
@busykai
busykai Oct 15, 2015 Member

Nit: First letter of the first word in a sentence should be capitalized.

@zaggino
Member
zaggino commented Oct 15, 2015

Recursive tests with the same id are not supported. is not an extension issue, it's a core issue. I'm guessing in this case it's the fact that eslint is called for twice (for each view) and PerfUtils id is created from the file name, so id is the same in both cases.

@busykai
Member
busykai commented Oct 15, 2015

@zaggino, I though I fixed that in #9392. I'll take a look at what this does to cause it again.

@petetnt
Member
petetnt commented Oct 16, 2015

@zaggino yeah, I explained that pretty badly as I was in a bit of a hurry and might have jumped into conclusions as linting the same file without any extensions enabled (with JSLint) seems to work fine, but doing the same while using brackets-eslint seemed to end up in EventDispatcher.js:224 Exception.

But you are correct, it doesn't have anything to do with ESLint itself, my bad. I've updated the post.

@petetnt
Member
petetnt commented Oct 16, 2015

More issues:

De-synchronized files
  1. Open bar.js in pane 1 and 2
  2. Type `var helloworld = "Hello, world!" to pane 1 (should duplicate to pane 2)
  3. Open foo.js in pane 2
  4. Modify the pane 1 text to var foobar = "Foo, bar!
  5. Open bar.js in pane 2 again
  6. Start typing in pane 1
  7. Destruction ensues as letters get duplicated in pane 1 and the cursor position is completely off in pane 2.
Working set behavior

Working set should be synchronized too: if a file gets modified (is marked dirty) in pane 2 and is open in pane 1, it should appear in both working sets.

After closing a file, the pane states that it is still the same file but shows the contents of the other file

Haven't been able to repeat this in a repeatable manner, but sometimes when closing files that are duplicated, pane text and the contents of the file don't match. Might be related to the working set behavior. Saving the file in this state brings tons of unsuspected behaviors.

@swmitra
Contributor
swmitra commented Oct 16, 2015

Thanks a lot guys for the quick start and please excuse me for responding so late.
@petetnt I will fix the mru list issue and also go through the flip scenarios once I get back to work.
@busykai I will fix all those lint issues as soon as I come back.

@swmitra swmitra JSLint and JSHint warning removal
Cleaned up JSLint and JSHint warnings from the change set.
b1251ce
@brenovieira

+1

I hope you merge this soon =]

@swmitra
Contributor
swmitra commented Oct 27, 2015

@brenovieira Sure! Sorting out few issues with the changes.

@swmitra swmitra MRU list corruption fix
MRU list corruption fix
Suppress File Save Dialog when a dirty file is open in other panes apart
from the pane requested
51a1799
@swmitra
Contributor
swmitra commented Oct 27, 2015

@zaggino @petetnt Updated the PR with MRU List fix.
@petetnt Can you please let check it once more with your feature?
@busykai Fixed all the lint warnings.
I have done some changes to DocumentCommandHandler to ensure it doesn't display the File Save dialog if file close is requested for a pane and the file is listed in other panes as well.
I thought that would be a logical behavior.

@petetnt
Member
petetnt commented Oct 28, 2015

@swmitra seems to be working with my feature now 👍

I did run into that de-syncronized issue described here: #11820 (comment) again

Also found a new bug:

  1. Open foo.js to pane 1 and 2
  2. Type something in pane 1 -> gets added to left working set
  3. Switch to pane 2 and press CTRL+Z / undo -> gets added to right working set
@swmitra
Contributor
swmitra commented Oct 28, 2015

@petetnt To me it seems like expected as undo/redo stack should be in sync. If I am missing something here please let me know.

I have tried to reproduce de-synchronized issue but it's intermittent. Looks like change listner is getting registered multiple times. I am looking into this and push the required changes soon.

@petetnt
Member
petetnt commented Oct 28, 2015

@swmitra Check out this screenshot flow if it's any help (redo and undo stack work as expected, the error is in the working set):

image

Write in the first panel

image

Write in the second panel

image

Press CTRL+Z / Undo when second panel is still active (you might need to do it a few times, it doesn't always trigger for me):
image

Not sure if it would make sense that the working sets would be in sync with each other altogether: if foo.js is modified in left panel while it's also in right panel, both working sets should show the file. Then again it might just create additional noise to the working sets.

@swmitra swmitra Split view shortcuts to target panes
Adding support to open a document in FIRST pane or SECOND pane
irrespective of active pane. Cmd-Alt + Left Click to open a file in
SECOND pane , Cmd + Left click to open a file in FIRST pane.

TODO : Shortcut to open a file in both the panes
1571821
@ryanstewart
Member

Just took a look at this, sorry for the delay. I didn't do a ton of testing, but so far it looks great and is a very nice addition.

Open question on the shortcuts: Should we use Left Pane/Right Pane instead of First Pane/Second Pane? I suppose the latter scales better for if/when we get multiple splits.

@swmitra
Contributor
swmitra commented Dec 2, 2015

@petetnt Finally I have modified the working set behavior in two use cases.

  • If same document is opened in both the panes as view only and not edited yet, first edit in any of the panes will force the document to get added to the other panes working set along with the originating panes working set. ( you have already asked for this :-) )
  • If a document is opened in one pane and present in that working set with dirty flag set to true, while opening the same document in other pane, it gets added to the other panes working set as well.
@abose
Contributor
abose commented Dec 2, 2015

There is one pending issue, if the same doc is open in both panes, after doing some edits, if you do undo till the start state, the document will not get undirtied one all changes are undone.

@swmitra
Contributor
swmitra commented Dec 3, 2015

There are two pending issues @abose @nethip @petetnt -

  • In some open/close combination ( not yet zeroed on the actual sequence ) of the same document in multiple panes and inline widgets, change list gets applied to the editors multiple times.
  • if the same doc is open in both panes, after doing some edits, if you do undo till the start state, the document will not revert back to clean state when all changes are undone.

I am working on both of these and hopefully today we can resolve both these issues and probably good to go for a merge.

@swmitra swmitra self-assigned this Dec 4, 2015
@swmitra
Contributor
swmitra commented Dec 4, 2015

@nethip @petetnt @abose @ficristo Please use this feature and let me know if you run into any issues. This branch is up to date with changes in master.

@petetnt petetnt and 1 other commented on an outdated diff Dec 4, 2015
src/project/FileViewController.js
- paneId = (paneId || MainViewManager.ACTIVE_PANE);
+ paneId = (paneId || ((event.ctrlKey || event.metaKey) && event.altKey ? MainViewManager.SECOND_PANE : null) || ((event.ctrlKey || event.metaKey) ? MainViewManager.FIRST_PANE : null) || MainViewManager.ACTIVE_PANE);
@petetnt
petetnt Dec 4, 2015 Member

Could (should?) this check be a method, for example FileViewController#_getPaneId()?

@swmitra
swmitra Dec 4, 2015 Contributor

I will extract it out as a method.

@petetnt
Member
petetnt commented Dec 4, 2015

I have been using 1e613cf for about an hour now and it works great so far!

When trying to deliberately break the system, I managed to do some things described below, but how they happened were most definitely beyond anyone`s even extreme use of Brackets. Doing things such as holding down keys while highlighting text with the mouse and rapidly changing between documents while doing other actions, so these should be consider very minor and just for reference if they come up later:

I screwed around the editor trying to get the split view to bug out and eventually I started getting these when I changed files:

Possible memory leak: 16 'dirtyFlagChange' listeners attached to Object {_eventHandlers: Object, on: function, off: function, one: function, trigger: function…}

I also managed to redo the out-of-sync issue here #11820 (comment) couple of times, but it required absolute destruction of files so I doubt it will ever happen in normal use.

@swmitra
Contributor
swmitra commented Dec 4, 2015

@petetnt Memory leak issue looks to me a timing issue which would be very difficult to reproduce under normal usage. But my fear is that , it shouldn't become a ghost issue like Ctrl + Z .

This should have been prevented I guess. I will try to check the implementation. Looks like this is the '_doWorkingSetSync' handler which is being attached to DocumentManager. Probably , it's not getting de-registered, although in Editor's destroy I have an 'off' call.

Thanks a ton for using the feature and prompt feedback.

@petetnt
Member
petetnt commented Dec 7, 2015

@swmitra Oh crap, found a rather massive oversight. Try opening any image file in Brackets. 🔥 🚒 💣

image

 Exception in 'select' listener on ProjectModel TypeError: undefined is not a function TypeError: undefined is not a function
    at Pane.addView (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/view/Pane.js:1114:14)
    at _createImageView (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/ImageViewer.js:427:18)
    at Object.MainViewFactory.registerViewFactory.openFile (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/ImageViewer.js:471:20)
    at file:///C:/Program%20Files%20(x86)/Brackets/dev/src/view/MainViewManager.js:1243:29
    at FileSystemEntry.exists (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/filesystem/FileSystemEntry.js:286:13)
    at Object._open (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/view/MainViewManager.js:1240:18)
    at _doOpen (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/document/DocumentCommandHandlers.js:326:29)
    at _doOpenWithOptionalPath (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/document/DocumentCommandHandlers.js:396:22)
    at handleFileOpen (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/document/DocumentCommandHandlers.js:453:9)
    at Command.handleDocumentOpen [as _commandFn] (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/document/DocumentCommandHandlers.js:502:9)
@swmitra
Contributor
swmitra commented Dec 7, 2015

@petetnt Silly me :-( . Fixed now by handling custom views. Need to do some more unit testing on this area though.

@petetnt
Member
petetnt commented Dec 7, 2015

@swmitra thanks for the quick fix!

@petetnt
Member
petetnt commented Dec 8, 2015

@swmitra Just now, after getting the Possible memory leak warnings for a while, another file foo.js showed the content of another file bar.js (that was open on Pane 2 at the same time). foo.js didn't start to show the correct context until Brackets was restarted (the situation persisted despite closing all the related files in the working set).

@swmitra
Contributor
swmitra commented Dec 9, 2015

@petetnt Fixed the possible memory leak issue. Found two new problems(?) while testing with the flip feature.

  • If I open a document as view only (without adding it to working set ) and then do a flip , it gets added to the other panes working set. I thought it should be again view only as it was not in working set.
  • If I do any edit in a document and then try to close it using the close link in flip toolbar, it doesn't ask for save confirmation and marks the document clean. So the edited content is lost. I was trying to debug this issue and noticed that handleFileClose handler in DocumentCommandHandlers is never getting called in this case. But if I try to close the doc in same scenario using close link in working set it does go though the FILE_CLOSE command which takes care of these things.

This might be an issue with the integration of both these features , but please have a look to identify any obvious pointer which I would have missed during debugging.

@swmitra
Contributor
swmitra commented Dec 9, 2015

@petetnt I have fixed the second problem mentioned in my earlier post. It was happening because paneId was not being sent as part of command data while invoking FILE_CLOSE. Included the pane info now.

@petetnt
Member
petetnt commented Dec 9, 2015

@swmitra I think the first issue should be fixed by checking if the current file is in the working set before triggering viewListChanges here: https://github.com/adobe/brackets/blob/master/src/view/Pane.js#L252

I think that might be an error in the original implementation too, not directly related to integration itself

@nethip
Contributor
nethip commented Dec 11, 2015

LGTM. Thanks @swmitra

This is a great addition to Brackets! Thanks @petetnt @busykai @zaggino for helping us out in the code review as well as testing activities. We really appreciate it!

@nethip nethip merged commit ad260bb into master Dec 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@swmitra swmitra deleted the swmitra/SplitViewSameDoc branch Dec 14, 2015
@depiction

How is this feature used? I can't find any documentation and am not sure how to view a file in both panes in 1.6.

@petetnt
Member
petetnt commented Jan 20, 2016

@depiction Open Split View and open a file to the first pane. Then focus the other pane and open the same file again (from the file tree). You should now see the same file on both panes.

@depiction

@petetnt Thanks.

@DariusNorv

Hi, how can I disable this feature? Unfortunately it's very unusual for me

@swmitra
Contributor
swmitra commented Jan 26, 2016

It's an explicit feature. You can always choose not to have the same doc on both the panes.

@petetnt
Member
petetnt commented Jan 26, 2016

To be honest it confused me too (at first) a lot before getting used to it (and now I quite enjoy it 😸)

Not sure how hard it would be to add a preference to it though. At best it would be just checking which pane to open on (if the file is already open).

@DariusNorv

@swmitra When I choose a document to edit from working directory (double-click) it automatically opens in active pane. I prefer I my workspase to keep different file types in different panes, so it will be great to enable and disable this feature when needed

@Siphon880gh

Works terrifically!

A tip for devs with many files in the File Tree and find it frustrating to find where the file is to open in the other panel:

First make sure the panel you want to open the file in as well - that it's active or in focus. Then right click the file at the top left panel (rather than the File Tree), then go to "Show in Finder" (on Mac or whatever equivalent on Windows). Open the file from there. It should open in the active panel! Now you have two panels editing the same file.

@paulmz
paulmz commented Nov 4, 2016

I saw a single post above asking this question but no real answer. Is there any way to disable this "feature"? I personally do not like it as it constantly takes files that are already open in one pane and then opens them in the other pane when clicking tabs. This is insanely annoying and disrupts my workflow.

@mxxrten mxxrten referenced this pull request in gruehle/MarkdownPreview Nov 22, 2016
Open

Preview in separate Window (as for html) or on the right side #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment