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

busykai · 2013-07-04T15:03:49Z

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

- Added FileUtils.getBaseName (jsdoc and unit tests included). - Replace usage of path-utils.js with getBaseName.

busykai · 2013-07-04T17:28:41Z

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.

Related to issues adobe#3269, adobe#4210. Based on the discussion and suggestions in adobe#3310. If a project has a .jslint.json in its root, it gets loaded and passed to each JSLint run within the project. The configuration file gets reloaded each time it is saved or refreshed.

This reverts commit 11dbdfd.

njx · 2013-07-08T18:16:38Z

To @RaymondLim

peterflynn · 2013-07-09T21:42:56Z

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...

njx · 2013-07-09T21:45:01Z

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.

busykai · 2013-07-10T04:35:34Z

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

busykai · 2013-07-11T14:28:39Z

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.

busykai · 2013-07-12T05:19:55Z

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

The tests assumed that CodeMirror modes are loaded at the time of test definition.

Conflicts: src/file/FileUtils.js

busykai · 2013-07-14T04:48:23Z

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...

busykai · 2013-07-14T06:42:59Z

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).

LanguageManager.getLanguageForPath should not use URLs. Instead, it should use local filesystem paths.

Also: Remove leftover reference to LanguageManager from FileUtils. Use existing code to implement getBaseName.

Implementation changed to use FileUtils.getBaseName and FileUtils.getFilenameExtension.

busykai · 2013-07-14T21:10:42Z

test/spec/Editor-test.js

                // verify editor content
-                var mode = LanguageManager.getLanguageForPath("http://only.org/testing/the/path.css?v=2").getMode();
+                var mode = LanguageManager.getLanguageForPath("/only/testing/the/path.css").getMode();


This is a hypothetical test case. LanguageManager is used strictly to operate on local files. I think it was inspired by the fact that PathUtils was used to parse the file path in the first place.

busykai · 2013-07-14T21:19:26Z

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

RaymondLim · 2013-07-15T05:16:25Z

@busykai

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

adrocknaphobia · 2013-07-15T16:02:56Z

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

RaymondLim · 2013-07-15T16:34:47Z

src/file/FileUtils.js

+
+    /**
+     * Get the base name of a file or a directory.
+     * @param {string} full path to a file or directory


You need to add fullPath right after {string}.

RaymondLim · 2013-07-15T16:36:54Z

src/file/FileUtils.js

+     *
+     * @param {string} full path to a file or directory
+     * @return {string} Returns the extension of a filename or empty string if
+     * the argument is a directory


Append or a filename with no extension. to this line so that we have more accurate description.

busykai · 2013-07-15T21:48:13Z

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

RaymondLim · 2013-07-16T04:48:59Z

Looks good. Merging.

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

busykai added 2 commits July 4, 2013 11:41

Fix adobe#4333 (files shown as plain text incorretly).

43885a6

- Added FileUtils.getBaseName (jsdoc and unit tests included). - Replace usage of path-utils.js with getBaseName.

Fix unit tests for FileUtils.getBaseName.

82e7986

busykai mentioned this pull request Jul 4, 2013

Files incorrectly identified as plain text if folder's name has got the "#" char #4333

Closed

busykai added 2 commits July 5, 2013 16:25

Revert "Add support for project-wide JSLint config."

c3df4c9

This reverts commit 11dbdfd.

ghost assigned RaymondLim Jul 8, 2013

busykai added 2 commits July 14, 2013 01:14

Fix CSSUtils unit tests.

b87f6a0

The tests assumed that CodeMirror modes are loaded at the time of test definition.

Merge remote-tracking branch 'upstream/master'

7551908

Conflicts: src/file/FileUtils.js

busykai mentioned this pull request Jul 14, 2013

fix "Integration / DocumentCommandHandler" unit tests #4448

Closed

busykai added 5 commits July 14, 2013 05:10

Fix Editor tests.

64bd9cb

LanguageManager.getLanguageForPath should not use URLs. Instead, it should use local filesystem paths.

Removed excessive : from the test description.

77f2140

Add getFileExtension implementation and unit tests

07850cd

Also: Remove leftover reference to LanguageManager from FileUtils. Use existing code to implement getBaseName.

Get rid of PathUtils where appropriate.

37bbc55

Implementation changed to use FileUtils.getBaseName and FileUtils.getFilenameExtension.

Remove stale jslint PathUtils refs and extra loads

8881093

busykai reviewed Jul 14, 2013
View reviewed changes

Restore jslint config to include PathUtils.

ae48141

RaymondLim reviewed Jul 15, 2013
View reviewed changes

Fix code review issues.

4b8ae42

RaymondLim added a commit that referenced this pull request Jul 16, 2013

Merge pull request #4379 from busykai/master

d0ddf4d

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

RaymondLim merged commit d0ddf4d into adobe:master Jul 16, 2013

RaymondLim mentioned this pull request Jul 16, 2013

[OPEN] Fix #4332 and #4409: Sort issues fixed on the Working Set and Project Tree #4463

Merged

core-ai-bot mentioned this pull request Aug 29, 2021

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

busykai commented Jul 4, 2013

busykai commented Jul 4, 2013

njx commented Jul 8, 2013

peterflynn commented Jul 9, 2013

njx commented Jul 9, 2013

busykai commented Jul 10, 2013

busykai commented Jul 11, 2013

busykai commented Jul 12, 2013

busykai commented Jul 14, 2013

busykai commented Jul 14, 2013

busykai Jul 14, 2013

busykai commented Jul 14, 2013

RaymondLim commented Jul 15, 2013

adrocknaphobia commented Jul 15, 2013

RaymondLim Jul 15, 2013

RaymondLim Jul 15, 2013

busykai commented Jul 15, 2013

RaymondLim commented Jul 16, 2013

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

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

Conversation

busykai commented Jul 4, 2013

busykai commented Jul 4, 2013

njx commented Jul 8, 2013

peterflynn commented Jul 9, 2013

njx commented Jul 9, 2013

busykai commented Jul 10, 2013

busykai commented Jul 11, 2013

busykai commented Jul 12, 2013

busykai commented Jul 14, 2013

busykai commented Jul 14, 2013

busykai Jul 14, 2013

Choose a reason for hiding this comment

busykai commented Jul 14, 2013

RaymondLim commented Jul 15, 2013

adrocknaphobia commented Jul 15, 2013

RaymondLim Jul 15, 2013

Choose a reason for hiding this comment

RaymondLim Jul 15, 2013

Choose a reason for hiding this comment

busykai commented Jul 15, 2013

RaymondLim commented Jul 16, 2013