Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Split View (Same Document) #11820

Merged
merged 16 commits into from Dec 11, 2015
Merged

Split View (Same Document) #11820

merged 16 commits into from Dec 11, 2015

Conversation

swmitra
Copy link
Collaborator

@swmitra 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
Copy link
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
Copy link
Contributor

nethip commented Oct 15, 2015

Tagging @abose

@nethip
Copy link
Contributor

nethip commented Oct 15, 2015

@swmitra Please fix the JSHint errors.

@petetnt
Copy link
Collaborator

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)

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){
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@zaggino
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

Cleaned up JSLint and JSHint warnings from the change set.
@brenovieira
Copy link

+1

I hope you merge this soon =]

@swmitra
Copy link
Collaborator Author

swmitra commented Oct 27, 2015

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

MRU list corruption fix
Suppress File Save Dialog when a dirty file is open in other panes apart
from the pane requested
@swmitra
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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.

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
@ryanstewart
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

petetnt commented Dec 7, 2015

@swmitra thanks for the quick fix!

@petetnt
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
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 added a commit that referenced this pull request Dec 11, 2015
@nethip nethip merged commit ad260bb into master Dec 11, 2015
@swmitra swmitra deleted the swmitra/SplitViewSameDoc branch December 14, 2015 05:30
@depiction
Copy link

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
Copy link
Collaborator

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
Copy link

@petetnt Thanks.

@DariusNorv
Copy link

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

@swmitra
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link

@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
Copy link

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
Copy link

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet