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

Fix for documents refCount issue #12405

Merged
merged 5 commits into from
May 19, 2016
Merged

Fix for documents refCount issue #12405

merged 5 commits into from
May 19, 2016

Conversation

swmitra
Copy link
Collaborator

@swmitra swmitra commented May 5, 2016

This PR provides a fix for the document refCount issue reported as part of #12396. This resolves the URL Code hints unit test failures barring the one which is to deal with '@import'.

CC @marcelgerber @petetnt @nethip @abose for review.

*/
Document.prototype._checkAssociatedEditorForPane = function (paneId) {
var editorCount, editorForPane;
for (editorCount = 0; editorCount < this._associatedFullEditors; editorCount++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing length there: editorCount < this._associatedFullEditors.length 🔥

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry. Issue with copy paste from another repo. Fixed now.

@ficristo
Copy link
Collaborator

I was trying the STR in #12396, but with this PR when i click on the filename of the inline editor it doesn't open the file.
The cursor is moved at the correct line but in the current file.

@nethip
Copy link
Contributor

nethip commented May 13, 2016

@swmitra Could you address the query posted by @ficristo ?

@swmitra
Copy link
Collaborator Author

swmitra commented May 13, 2016

@ficristo Thanks a lot for actually trying the fix 👍

I need one clarification regarding the behavior you have mentioned. I am guessing that the inline editor and the host editor in your case is pointing to the same document. If that's the case then, even without this fix the behavior is same in master.

Can you please confirm?

@ficristo
Copy link
Collaborator

@swmitra I've opened the SpecRunnerUtils.js and put the cursor on 406 (this on current master).
Pressing Ctrl-E it opens the inline editor pointing to EditorManager line 156.
If I click on the name it move the cursor on line 156 but of SpecRunnerUtils.
It should instead open EditorManager at lime 156.
I've tryed this after reload without extensions.

@swmitra
Copy link
Collaborator Author

swmitra commented May 13, 2016

Thanks @ficristo. It's clear now, I will fix it tonight.

@swmitra
Copy link
Collaborator Author

swmitra commented May 17, 2016

@ficristo @marcelgerber Can you please try with the recent changes?
Thanks in advance 😄

@ficristo
Copy link
Collaborator

Now when I click on the inline editor I have this error:
/src/editor/EditorManager.js found in pane working set but pane.addView() has not been called for the view created for it

@swmitra
Copy link
Collaborator Author

swmitra commented May 18, 2016

@ficristo It was getting caused by a missing check in _showEditor(). I had put it back now. URL Code hint test cases are passing as well. Can you please check it once more (hopefully for the last time 😄 ).
Thanks again for helping us out in verifying the fix 👍

@ficristo
Copy link
Collaborator

There is still an error on Url Code Hints, not sure if is related to this or not.
I couldn't reproduce the exact steps mentioned by @marcelgerber in #12396: I have a different _refcount number.
I tested what I could and it seems good; someone with better knowledge should look at it too.

*/
Document.prototype._checkAssociatedEditorForPane = function (paneId) {
var editorCount, editorForPane;
for (editorCount = 0; editorCount < this._associatedFullEditors.length; editorCount++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

++editorCount

@marcelgerber
Copy link
Contributor

@ficristo This one test failure is fixed on master already, so with latest master merged in, all test pass 👍
I've followed my repro steps and even though I didn't get the same numbers in the first place, as a result, I had only one document left open. Thumbs up for that! Good work, @swmitra!

Changing `editorCount++` to `++editorCount`
@nethip
Copy link
Contributor

nethip commented May 19, 2016

@swmitra I have taken liberty in changing editorCount++ to `++editorCount.

@nethip
Copy link
Contributor

nethip commented May 19, 2016

Since travis is not able to run because of API limitation, I have manually run grunt build and the build went fine. So merging this.

Thanks @swmitra 👍

And thanks to @marcelgerber @ficristo for helping this PR come to light.

@nethip nethip merged commit 2a4b491 into master May 19, 2016
@nethip nethip deleted the swmitra/RefCountFix branch May 19, 2016 13:10
@swmitra
Copy link
Collaborator Author

swmitra commented May 19, 2016

Thanks a lot @nethip @marcelgerber @ficristo for helping us resolving the issue 👍

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

Successfully merging this pull request may close these issues.

5 participants