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

Replace in Files can create a broken state with same file in both panes #9569

Closed
peterflynn opened this issue Oct 15, 2014 · 4 comments · Fixed by #9677
Closed

Replace in Files can create a broken state with same file in both panes #9569

peterflynn opened this issue Oct 15, 2014 · 4 comments · Fixed by #9677

Comments

@peterflynn
Copy link
Member

  1. Split your view
  2. Invoke 'Replace in Files' and enter a search term
  3. Double-click the first result in the bottom pane, adding that file to the working set
  4. Focus the other pane
  5. Click Replace

Result: the file jumps over to the second pane, but the working set still shows it in the first pane. The first pane is blank (without the "no editor" watermark) but its heading still shows the name of the file that is now shown in the second pane.

Expected: file is focused in first pane again, just as if you'd opened it via some other command like Quick Open.

@peterflynn
Copy link
Member Author

There are two bugs at play here:

  • A few parts of the UI, including Replace in Files, call MainViewManager._edit() directly. Its docs say it should only be called by unit tests and File > New. It seems like the other callers could just invoke the normal FILE_OPEN command instead...
  • PR Clicking on items in the working set seems slow #9439 has a bug -- the if statement it added in _edit() is reversed, so the optimization is actually inactive in the common case it was trying to speed up (i.e. the code does just as much work as it did before 9439). But in the edge cases where the pane-check is needed, the optimization erroneously kicks in and skips it.

So a simple fix would be to invert the if statement in _edit()... or just remove it since it seems like it doesn't have any significant effect on performance (so the added complexity may not be worth keeping).

Ideally, it would be nice to clean up those stray callers to stop using _edit(), too. Once that's done, we could even consider baking in the optimization instead (since no one should be calling it anymore in a context where that pane-check is necessary).

@JeffryBooher
Copy link
Contributor

@peterflynn I was thinking that if there are no files open when the file is added to the working set on the second pane that it should probably go ahead and open the first file in the working set. I searched for "MainViewManager._edit(" and replaced with "FILE_OPEN" and at step 5 in your recipe it adds the remaining files to the second pane but none of them are open because DocumentManager just adds the files to the working set -- it doesn't open them. Should we just do that in the PR to fix this?

@JeffryBooher
Copy link
Contributor

@peterflynn I have a pr #9636 that fixes this.

@redmunds
Copy link
Contributor

Confirmed. Closing.

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

Successfully merging a pull request may close this issue.

3 participants