Explorer: Locate Current Buffer #2429
Conversation
Add command palette option "Explorer: Locate Current Buffer" that will expand directories down to the current buffer, then select the corresponding file/directory.
|
As I'm entirely new to the Oni codebase I thought I would submit this before I tackle writing tests, in the hopes someone can take a look and let me know if my approach is reasonable. |
|
I just tried this out @feltech 😍 very cool 👍, I think the approach is good although there are couple of things I noted whilst trying it. The speed with which you can navigate the explorer seems much slower, checking the logs it seems a This is more cosmetic-ish but if the explorer/sidebar is closed and you use this command it remains closed but I found that on opening it the correct file is selected, so I think the functionality did work but it was initially a bit confusing, I think a good check would be to see if the sidebar is closed and if so open it, you might be able to achieve this be using |
Codecov Report
@@ Coverage Diff @@
## master #2429 +/- ##
=========================================
+ Coverage 42.62% 43.7% +1.07%
=========================================
Files 332 343 +11
Lines 13212 13664 +452
Branches 1736 1799 +63
=========================================
+ Hits 5632 5972 +340
- Misses 7302 7454 +152
+ Partials 278 238 -40
Continue to review full report at Codecov.
|
Prevent sending pointless `SELECT_FILE_SUCCESS` actions if we're not actually in the process of selecting a file (or haven't selected the file we expect yet).
|
@akin909 Thanks for the quick feedback! I've changed the SUCCESS action logic so that it doesn't trigger unnecessarily. It seems odd that dispatching such a simple action (which isn't part of the epic) would slow things down. When I tried I could maybe feel some slight lag, but not particularly noticeable. Perhaps my machine is just able to absorb the extra effort. I do find the Explorer pane is a bit buggy when scrolling down beyond the last visible file. The pane seems to scroll jump up and down. That bug is present in the current stable release, which I'm using for development, so I'm not concerned that my changes broke it. I'll try to remember to check if there's an existing bug report and if not I'll make one. Activating the Explorer if it's closed is a nice idea, I'll add that asap. |
| @@ -50,6 +50,20 @@ export const isPathExpanded = (state: IExplorerState, pathToCheck: string): bool | |||
| return !!state.expandedFolders[pathToCheck] | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
@feltech a version of this same functionality already exists in ExplorerStore.ts maybe you could import that and use it instead
* Keep `registerCommand` calls together in the `index.tsx` and unify their style. * Use the `SidebarManager` to first ensure the Explorer is shown, before then locating the file in the `ExplorerSplit`.
|
I think re. the slowness it might have been that the epic was triggered by the select action so the whole function runs each time i will re-try it later I'm working on a laptop but a fairly well spec'ed one but everything is slower in development due to all the logging etc. Re. the flicker in the explorer that isn't because of your changes its a known bug i've opened #2182 in order to fix that |
* Strangely, `SidebarManager.setActiveEntry` will close the sidebar if the Explorer is already open and you try to open it again. * So only trigger opening the Explorer if it's not already visible.
|
@akin909 - well spotted on the duplicated Now the Explorer will open if it was closed when locating a file. |
* `epicMiddleware.replaceEpic` internally dispatches a `@@redux-observable/EPIC_END` action, which gets saved to the `mockStore`'s actions list. Every test case's `afterEach` thus adds an additional action to this actions list. This pollutes all subsequent stores created via the `mockStore` factory, which is odd. * This was accounted for in the YANK_AND_PASTE_EPICS test by assuming "an init action is sent first", which is not the case. * So move the setup that is only for the SET_ROOT_DIRECTORY test inside it's own `describe` block, remove the unnecessary `replaceEpic` call, and fix the test for expected number of actions.
|
Still more tests to write. That weird |
locate_buffer_in_explorer # Conflicts: # browser/test/Services/Explorer/ExplorerStoreTests.ts
|
@feltech thanks for adding so all those tests 🎉 |
|
@feltech is this complete? |
|
Nearly, working on the final test now (though it's late here so might take another day). |
|
That's a good plan. My one concern is that the OSX integration test for this feature did highlight a genuine issue to do with symlinks, so it would be good to include the fix for that. I suppose that could be part of a separate PR to improve the feature (along with the fixes to the CI tests). |
|
I've now split at the last successful build as #2456. However, I had to merge in master so the tests are going to run again... I still hold out some hope of fixing all this up. So it would be good to keep this PR around so I can abuse Travis a bit longer. |
|
Hmm so the split-off PR looks good as far as CI goes. Very strange. I might have to rewind and cherry pick bit by bit to isolate what causes the breakage. |
|
No freaking way. Finally Travis stops complaining! There's a bunch of debug logs to remove and other tidying. Hopefully the build still passes after that. |
|
OK then, pending review I think this is good to go. |
package.json
Outdated
| @@ -628,7 +628,7 @@ | |||
| "test:setup": | |||
| "yarn run build:test:integration && mocha -t 120000 --recursive lib_test/test/setup", | |||
| "test:integration": | |||
| "yarn run build:test:integration && mocha -t 120000 lib_test/test/CiTests.js --bail", | |||
| "yarn run build:test:integration && mocha -t 120000 lib_test/test/CiTests.js --bail --runInBand --no-cache", | |||
There was a problem hiding this comment.
Just for my info what does this change runInBand
There was a problem hiding this comment.
Yeah, apparently I got my wires crossed. Looks like it has no effect. It should ensure that all tests run in the same process. But that looks to be a jest config option, not mocha. From the jest docs
Run all tests serially in the current process, rather than creating a worker pool of child processes that run tests. This can be useful for debugging.
There was a problem hiding this comment.
I'm pushing a commit to remove those pointless flags. Fingers crossed the tests still pass!
|
From a quick look, it looks like the main changes are swapping to using a temporary workspace in some tests, since currently they run in the full Oni repo. That should help a few of the tests, but we may want to revert it if in the future we do some performance tests since the Oni repo is nice to test against as its a decent size. The other changes seem to be removing the "before" and "after" parts of the CI tests, and collapsing it down into the test itself. It doesn't look to change the tests themselves, but if it helps reliability then sounds good! Only part I wasn't quite sure on was the catching and then doing nothing with the error in the final try-finally of As for the |
|
Ah good point. A summary of the changes:
|
My thinking was that if Oni had failed to start or was in some other bad state, then presumably we could get an exception trying to stop it, which we don't really care about. I suppose the downside is that if Oni fails to shut down, then that specific test won't fail (should it?). However, I suspect in that case subsequent tests will fail, because there would be two instances of Oni running, which is bad news for these integration tests. |
|
@CrossR Re. swapping to use a temporary workspace in some tests: I did this originally in the integration test for this feature, so that I wasn't relying on a specific workspace and some arbitrary file existing in it. Then I found that subsequent tests will run in that same temporary workspace, and at least one of those tests does rely on an arbitrary file in the Oni repo. I judged that to be bad practice, so went with refactoring that test to also use a temporary workspace. I do get your point about monitoring performance. Though integration tests that break because of a side-effect of a previous integration test are a pain to debug. I'm happy to change the test I wrote to rely on running in the Oni repo as it's workspace, if that's the standard. |
Nah, its fine! Your new implementation is great, its better to setup a proper workspace rather than depend on a file which could maybe move etc. It was more just a mental note that if in the future we swap to checking performance, we'd want to update it so we are running in a bigger repo for more realistic performance. |
|
Just wanted to check this PR can be merged, since its got WIP in the title still! Otherwise looks good. I assume the split-off PR can be closed? |
|
Hey, yep it's ready to go. I'll close the other PR. |
Add command palette option "Explorer: Locate Current Buffer" that will
expand directories down to the current buffer, then select the
corresponding file/directory.