[CLOSED] Adds directory names to the files in working tree, ... #4094
Comments
|
|
|
I was literally going to fork the repo and implement this, but it seems like you got it covered |
|
Looks good This is great. |
|
Actually, use the em dash ("—" instead). Thanks. |
|
No problem. |
|
Removing |
|
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. |
|
|
|
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.) |
|
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. |
|
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 |
|
|
|
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. |
|
Written a new algorithm for this. Take a look - it's giving slightly different results than the one before. |
|
That's definitely better. There's still one case I'm not as happy about... if you open test/node/package.json and src/extensibility/node/package.json, you get "package.json – node" (for test/node) and "package.json – extensibility/node". I like the fact that it's not src/extensibility/node now, but I think the other one should be "test/node". Does that seem right to you? |
|
You might want to reverse the folder check order. Since the last element in the path array is the top most folder, you could first check the top most folder against all the top most folders of the files with the same filename. If you find a folder with the same name, go down one folder and check again. Do this until you find a unique folder, or get to the last folder in the path. If you got to the last one without finding a unique folder, just use the whole path since the path from another file with the same name will eventually be longer and have a unique folder. You can optimize this by checking all the paths from the files with the same topmost folders/filename together. Start by creating an array with a pair You then iterate through the array I hope this can help you with the algorithm. |
|
... when I started this feature I thought it will be much easier :)
|
|
Nice, the implementation looks good. I had 1 comment on it. You should also fix 2 JSLint errors (spaces between As nice bonus, I like how it shows the project folder when this is the first different folder, instead of showing I was opening lots of packages.json files and opened:
Which correctly showed:
Where extensibility and test are the first different directories, but the list is still really long. We might want to think about also using ... when there are 4 or more folders to display.
There is also a conflict issue in this branch, you should merge with master and fix it. |
|
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 |
|
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 |
|
Comments in the code should be fixed, JSLint errors too. |
Thursday Jul 11, 2013 at 00:43 GMT
Originally opened as adobe/brackets#4419
... when multiple files with the same name are opened.
Any comments welcome.
zaggino included the following code: https://github.com/adobe/brackets/pull/4419/commits
The text was updated successfully, but these errors were encountered: