Cleaner fix for #2761 (Project tree selection incorrect after opening file by means other than sidebar) #3398
Conversation
… file by means other than sidebar) - Clear selection in project tree if current document not exposed in tree anywhere. Backs out pull #2869, which made the behavior of Find in Files inconsistent with other means of opening and didn't fix other cases of the bug. Adds unit tests for several of these edge cases.
| _lastSelected = null; | ||
| _projectTree.jstree("select_node", $node); | ||
| _lastSelected = $node; | ||
| _redraw(true, true); |
There was a problem hiding this comment.
Wouldn't it be better to leave those changes in because extension for example may want to use this function?
There was a problem hiding this comment.
We're trying to finish Sprint 23 today, so I think it's best to minimize the amount of change.
I, personally, think a more useful interaction would be that a double-click on a Find in Files result would also open the file in the Working Set, but that's just me :)
There was a problem hiding this comment.
actually i agree with you^
There was a problem hiding this comment.
For the explicit 'Show in File Tree' use case, the original code worked fine as far as I'm aware... so if we're not using this for some other purpose I think leaving the original code is safest.
I do agree that double-click on a Find in Files result should add it to the working set. Looks like this is being tracked by the main Find in Files user story. (And the in-progress pull request #3151 implements it).
|
I guess I misinterpreted your "Ideally, file B would become selected in the project tree if it's exposed" comment in the bug report, but you're right that none of the other tools do that. Reviewing... |
| _projectTree.jstree("select_node", $node); | ||
| _lastSelected = $node; | ||
| _redraw(true, true); | ||
| // jsTree will automatically expand parent nodes to ensure visible |
There was a problem hiding this comment.
I don't understand this comment: "jsTree will automatically expand parent nodes to ensure visible". Isn't that what your fix is trying to avoid ?
There was a problem hiding this comment.
Ah, that's restoring the previous code. Maybe that comment is incorrect?
There was a problem hiding this comment.
No, that's correct -- that is what this code does, but this code is no longer invoked when you click a Find in Files result (see deletion in FindInFiles.js). This is now only invoked by the 'Show in File Tree' command.
|
Done with initial review. |
ghost
assigned
|
@redmunds added a bunch of responses. Let me know if anything more is needed. Thanks for the review! |
|
Looks good. Merging. |
Cleaner fix for #2761 (Project tree selection incorrect after opening file by means other than sidebar)
|
Opened issue #3404 |
Cleaner fix for #2761 -- Clear selection in project tree if current document not exposed in tree anywhere.
Backs out pull #2869, which made the behavior of Find in Files inconsistent with other means of opening files and didn't fix other cases of the bug.
Adds unit tests for several of these edge cases.