Adds directory names to the files in working tree, ... #4419
Conversation
…s with the same name are opened there.
ghost
assigned 
|
@larz0 can you please review |
|
I was literally going to fork the repo and implement this, but it seems like you got it covered @zaggino. :-) |
|
Looks good @zaggino, could we use "-" instead of "|", and use 11px font for the the directory class? Like the screenshot below: This is great. |
|
Actually, use the em dash ("—" instead). Thanks. |
|
No problem. |
|
Removing @larz0 as assignee since he's already reviewed it for UI design. We'll assign a reviewer or mark it [OPEN] soon. |
|
@larz0 I think we'd want em dash only on Mac -- Windows uses a regular hyphen. See the titlebar, for example... We should probably hoist up some sort of platform-specific separator constant from the current titlebar code in DocumentCommandHandlers.WINDOW_TITLE_STRING so it can be shared with the new code here. |
|
@peterflynn ok sounds good. #4303 could use that as well then. |
|
@peterflynn I agree that not repeating that construct in DocumentCommandHandlers would be a good thing. Any opinion on where a separator constant should go? It's not l10n related, but it could go in strings. |
|
@zaggino Sorry for the delay in looking at this. Things got a bit hectic during sprint 28. The first thing that I noticed in looking at your changes is that they feel like the way an extension would do things: augmenting the working set after the core code has done its work. On the other hand, the other code for presenting items in the working set is concerned with the presentation of single items... adding duplicate tracking code to the existing working set view code would require redrawing previously drawn items in many cases. The approach you took may just be cleaner overall. WorkingSetView-test has a reasonable collection of tests to build on. Would you be able to add some tests that exercise this functionality. At least one test that adds a duplicate file to the working set, causing two files to be displayed with the directory would be good. If that test was then extended to remove one of the two duplicate files from the working set, I think you'd spot a bug where the directory name stays displayed for the remaining file. (At least, when I read through the code, that's what it looks like would happen... I haven't tested it yet.) |
…les with same name are opened.
…e of two files with duplicate filename.
|
@dangoor Yes, you were right. I added a test for this and then fixed the problem, take a look please. I also have a question - when I run tests in Brackets for the first time, they run from actual sources, which is fine. When I make some modifications to the tests, I need to restart Brackets before running tests again otherwise they seem to be running from some cached source. Am I doing something wrong? Surely it's not optimal to restart Brackets every time I write something into the tests. |
|
@zaggino thanks for adding the test. I'll take a look at the code a bit later. For running the tests without restarting, open the dev tools from the test runner window and click on the gear icon to pull up the settings. Click on Disable Caches there. (Annoying, I know! But, at least this is something you need only do once). |
|
I don't want to complicate this too much, but I noticed that if you open |
|
Overall, the code looks good, the test looks good. I appreciate the work you've done here! |
|
That's a valid point and I actually thought of this one when I was first writing the code but wasn't really sure about the proper solution as different editors I have used in the past handle this different way. What do you think would be preferable? Show just first different directory there (this case would be 'extensibility' and 'test', I think eclipse does this) or keep adding parent directory names until strings are different (that would show 'extensibility/node' and 'test/node') ? I personally like the second option more but string could get very long in extreme cases of course. That could be solved that if there were many directories like a/c/d/e and b/c/d/e, middle common thing could be replaced to a/.../e and b/.../e. If I'll have a hint how the final thing should look like I have no problem implementing it. |
|
Maybe @larz0 has an opinion on this, but I like your suggesting of keep adding directories until the strings are different. (FWIW, Sublime seems to show the different directories starting from the top... src/extensbility/node, test/node) |
…name are opened, located in folders with same name.
|
@dangoor take a look at new commits please. I used old style for cycles in the main algorithm to be as efficient as possible in this. Also been looking if it's possible to get into an infinite loop here, but I think that would require to have one file on the file system opened twice in the working tree, what should not be possible, right? |
… loops won't happen.
|
I just pulled the latest code from your branch and noticed three things:
For the third one above, I have node_modules/mathjs/package.json, src/extensions/dev/ex1/package.json and src/extensibility/node/package.json open. In this case, it should be able to just say "package.json – mathjs", "package.json – ex1" and "package.json – node". package.json has been a good stress test for this, given how many of them there are :) |
|
I'll definitely look at that third thing sometime over the weekend. I don't really know between "package.json" and "package.json – /" ... First one would give me a feeling that the file was excluded from the functionality (and the current tests would fail if there was no .directory element next to it, when counting how many files were affected by this, but that could be of course fixed). Second one would make probably me wonder for a second or two (for the first time), where did the slash come from. |
|
@TomMalbran I wanted to ask at least three times and always forgot, do you use any code-formatting tool ? I use jsbeautify in my projects before each commit so I don't have weird diffs in git, but that seems to mess-up a code here. I fixed those JSLint errors and the last mentioned problem: Also did the merge with current master (in two commits because I fucked up submodules in the first sadly). |
|
I use Brackets, with JSLint enabled, and no other formatting tool. It looks great now. 2 more minor things. We always use |
|
Fixed |
| @@ -89,6 +90,112 @@ define(function (require, exports, module) { | |||
| } | |||
|
|
|||
| /** | |||
| * Adds directory names to elements representing passed files in working tree | |||
| * @private | |||
| * @param {Array.<FileEntry>} fileArray | |||
There was a problem hiding this comment.
Move the @private above the description.
Rename fileArray to filesList and add a description since it kinds of expects FileEntries with the same filenames. Would work with any array of FileEntries but will not do the same stuff.
|
Thanks for the fixes. I added a few more comments after going through the final code. |
|
And there are a few more JSLint errors in |
| CommandManager.execute(Commands.FILE_CLOSE) | ||
| .done(function () { didClose = true; }) | ||
| .fail(function () { gotError = true; }); | ||
| waitsFor(function () { return didClose && !gotError; }, "timeout on FILE_CLOSE", 1000); |
There was a problem hiding this comment.
You can replace from line 287 to line 291 with:
waitsForDone(CommandManager.execute(Commands.FILE_CLOSE), "timeout on FILE_CLOSE", 1000);
|
Comments in the code should be fixed, JSLint errors too. |
|
Great, thanks. Will wait to see if @dangoor has something to add, since he is assigned to this PR. There might still be the issue about using "-" for Windows and "—" for Mac, but could be fixed on separate issue too. |
|
@TomMalbran thanks for diving in to this review. I've gone ahead and assigned it to you. If you think it's all set, go ahead and merge. Thanks again @zaggino for continuing to push forward on this. I know things often don't end up being quite as quick and easy as we expect :) |
|
@dangoor np. Looks good to me. Any suggestion as to what to do with the dash? I think we could add a dash string to |
|
@TomMalbran let's take care of that in a separate issue that would make this code and DocumentCommandHandlers do the same thing. |
|
Everything looks great here. Merging :) |
Adds directory names to the files in working tree, ...
|
@dangoor @TomMalbran Thanks, this has been quite a good learning experience ;-) |
|
@zaggino this is awesome. Good job! |
... when multiple files with the same name are opened.
Any comments welcome.