[CLOSED] Fix #4333 (Files incorrectly identified as plain text if folder's name has got the "#" char)

[core-ai-bot]

Issue by busykai
Thursday Jul 04, 2013 at 15:03 GMT
Originally opened as adobe/brackets#4379

---

Replaced usage of PathUtils (inappropriate for the task) with new FileUtils.getBaseName. JSDoc and unit tests included.

---

busykai included the following code: https://github.com/adobe/brackets/pull/4379/commits

[core-ai-bot]

Comment by busykai
Thursday Jul 04, 2013 at 17:28 GMT

---

I also suggest that NativeFileSystem.Entry constructor (which is used to display base names in the project side bar) also takes advantage of the same FilUtils.getBaseName() facility. Currently, it's using splits path by separator and then iterates until the last name to get the base name for an entry. I intentionally programmed getBaseName so that it would support directories as well.

[core-ai-bot]

Comment by njx
Monday Jul 08, 2013 at 18:16 GMT

---

To@RaymondLim

[core-ai-bot]

Comment by peterflynn
Tuesday Jul 09, 2013 at 21:42 GMT

---

Yikes, this suggests to me that we should never use PathUtils to parse local paths -- I bet this isn't the only place where a "#" in a file/folder name breaks stuff. It seems only safe to use PathUtils on things that are actually true URLs.

I see potentially unsafe uses in DocumandCommandHandlers, JSUtils, FileIndexManager, and UrlCodeHints/main. Should we file a new bug for all that, or try to tackle them here?

Also, some unit tests of the actual affected functionality (e.g. LanguageManager APIs) are probably a good idea...

[core-ai-bot]

Comment by njx
Tuesday Jul 09, 2013 at 21:45 GMT

---

Yes, PathUtils is a misnomer--it should really be called URLUtils. But we didn't write it so we can't change the name :)

I think it would be good to fix all of them at once if it's relatively straightforward to do so.

[core-ai-bot]

Comment by busykai
Wednesday Jul 10, 2013 at 04:35 GMT

---

I can review the code for misuse of PathUtils. Should I file an issue?

[core-ai-bot]

Comment by busykai
Thursday Jul 11, 2013 at 14:28 GMT

---

Here is the report on usage of PathUtils in entire brackets codebase:

OK -- uses PathUtils properly or has no impact:
    1 ./.jshintrc
          OK -- in globals
    2 ./src/LiveDevelopment/Agents/CSSAgent.js
          OK -- parses URL
    2 ./src/help/HelpCommandHandlers.js
          OK -- parses URL
    2 ./src/preferences/PreferencesDialogs.js
          OK -- parses URL
    2 ./src/utils/UpdateNotification.js
          OK -- parses URL
    4 ./src/extensibility/Package.js
          OK -- parses URL
    36 ./test/perf/OpenFile-perf-files/brackets-concat.js
          OK -- testcase input file

Has to be fixed:
    1 ./src/brackets.js
          Clean up - jslint config
    1 ./src/document/Document.js
          Clean up - jslint config
    1 ./src/document/DocumentManager.js
          Clean up - jslint config
    1 ./src/extensibility/InstallExtensionDialog.js
          Clean up - jslint config
    1 ./src/search/FindInFiles.js
          Clean up - jslint config
    1 ./test/SpecRunner.js
          Clean up - jslint config
    2 ./src/extensions/default/QuickView/main.js
          OK -- parses URL
          Fix -- use dirName
    2 ./src/extensions/default/UrlCodeHints/main.js
          Fix -- use dirName
    2 ./src/language/JSUtils.js
          Fix -- uses to get file extension
    2 ./src/project/FileIndexManager.js
          Fix -- uses to get file extension
    4 ./src/document/DocumentCommandHandlers.js
          Fix -- use baseName, dirName

In order to fix this, two more methods corresponding to the usage of PathUtils have to be added: getDirectoryName (analogous to dirname(1)) and getFileExtension. I will amend this pull request.

[core-ai-bot]

Comment by busykai
Friday Jul 12, 2013 at 05:19 GMT

---

This pull request is no good as is. Hold on until I update it, please.

[core-ai-bot]

Comment by busykai
Sunday Jul 14, 2013 at 04:48 GMT

---

I need your advice: what is normally done when the unit tests starts failing because of loading sequence/timing has changed due to a fix?

I found that the fix is causing some unit tests to fail. I specifically looked into CSSUtils and found (after some banging my head against the wall) that the 43885a6 is changing the load sequence, so b87f6a0 fixes it.

The other tests that are failing are trickier... For example some EditorCommandHandler tests do not have Commands loaded at the time of execution. One fails with "cannot read property EDIT_DELETE_LINES of null", for example. When re-ran on its own, it passes. I'm going to resolve all these, but please let me know what you think...

[core-ai-bot]

Comment by busykai
Sunday Jul 14, 2013 at 06:42 GMT

---

Nevermind the comment on EditorCommandHandler tests, it's a separate issue #4451. There's a pull request to fix it too.

This pull request also breaks DocumentCommandHandler tests in the same fashion #4437 does. Applying #4448 fixes them (I observe only two failures while #4448 fixes three).

[core-ai-bot]

Comment by busykai
Sunday Jul 14, 2013 at 21:19 GMT

---

@RaymondLim@njx@peterflynn this is ready for review. Ended up being not quite starter bug. :)

[core-ai-bot]

Comment by RaymondLim
Monday Jul 15, 2013 at 05:16 GMT

---

@busykai

I'll be reviewing and running your unit tests soon. In the meantime, could you please sign up CLA here?

[core-ai-bot]

Comment by adrocknaphobia
Monday Jul 15, 2013 at 16:02 GMT

---

@RaymondLim :@busykai is covered under the corporate CLA we have for Intel.

[core-ai-bot]

Comment by busykai
Monday Jul 15, 2013 at 21:48 GMT

---

Thank you@adrocknaphobia ! I wasn't sure what's the right way to clarify this.
@RaymondLim -- fixed as per your comments.

[core-ai-bot]

Comment by RaymondLim
Tuesday Jul 16, 2013 at 04:48 GMT

---

Looks good. Merging.