1073857 - Allow app names without ASCII characters when creating new app/handle non-ASCII characters like umlauts more gracefully (repeatedly asking for folder)

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox Nightly and Aurora 20140927 on Windows 8.1

Creating an app in the WebIDE and assigning a name which doesn't contain ascii characters (e.g. "äß") will repeatedly ask for the folder in which to save the app because the creation of the subfolder fails.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The problem is this regex[1] which accidentally kills all non-ASCII letters.

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/newapp.js#130

Comment 2

[Anirudh GP(:anirudhgp)]

Hi I'd like to work on this bug. I have already built mozilla-central.

Comment 3

[Anirudh GP(:anirudhgp)]

How do i fix this bug??

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We need to change WebIDE new app folder creation so that we don't just throw away non-ASCII characters.

Instead, we should be able to keep most letters, but have to drop some that aren't allowed in a directory name.  There may already be code in-tree for filtering file names to safe characters, so we should look for and reuse it if it exists.

Comment 5

[Anirudh GP(:anirudhgp)]

How might the code for filtering file names look like? We might be able to search the tree if we have a rough idea of how the code looks for filtering file names.

Comment 6

[Yussif Barcelos]

Hi! I'm interested to fix one bug for a subject in my degree. I want to work in this bug and try to fix it on this weekend. Could someone mentor me?

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yussif, you are welcome to give it a try.

To get started with our code base, see the hacking[1] page.

You'll need to determine a good regex that allows non-ASCII characters, while still filtering out characters that would fail as part of a path.  We may have such a code path already (try some searching with DXR[2]), or else this Superuser answer seems to suggest what characters to block[3].

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: http://dxr.mozilla.org/mozilla-central/source/
[3]: http://superuser.com/a/358861/135692

Comment 8

[Yussif Barcelos]

Thanks, J. Ryan!

Comment 9

[Yussif Barcelos]

Ryan,

Windows is more restricted with characters
- windows: except ASCII's control characters and \/:*?"<>|
- linux: except null or /

Is a good idea identify the system to apply the appropriate regex? Or should I apply the same rule to both systems?

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Yussif Barcelos from comment #9)
> Ryan,
> 
> Windows is more restricted with characters
> - windows: except ASCII's control characters and \/:*?"<>|
> - linux: except null or /
> 
> Is a good idea identify the system to apply the appropriate regex? Or should
> I apply the same rule to both systems?

Let's keep the code simpler by using only one regex that is good enough for every OS.

Check around for OS X rules as well, they may differ from Linux.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I happened to find this character set randomly:

http://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManager.cpp#606

Probably a good reference!

Comment 12

[Mike Hoye [:mhoye]]

Hi, Yussif - Are you still working on this?

Comment 13

[Shubham Jindal [:shubham]]

Hi, I want to work on this bug. How should I start?

Comment 14

[Shubham Jindal [:shubham]]

Which non-ASCII characters should be allowed for creating the directories? I am currently working on OS X.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Shubham Jindal from comment #14)
> Which non-ASCII characters should be allowed for creating the directories? I
> am currently working on OS X.

There's discussion of this in earlier bug comments, so please review them.  We want to be sure it will work on all platforms.

Comment 16

[Shubham Jindal [:shubham]]

We can just want to avoid \/:*?"<>| all these characters as they cause problems on at least one of the operating systems. The regex can be just simple as projectName.replace(/[\\/:*?"<>]/g, '').toLowerCase() in [1]

[1] : http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/newapp.js#130

Comment 17

[Shubham Jindal [:shubham]]

Sorry I missed | in the regex. The code will be projectName.replace(/[\\/:*?"<>|]/g, '').toLowerCase()

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Shubham Jindal from comment #17)
> Sorry I missed | in the regex. The code will be
> projectName.replace(/[\\/:*?"<>|]/g, '').toLowerCase()

Sounds reasonable to me.  Can you attach a patch with this approach?

Comment 19

[Shubham Jindal [:shubham]]

Attachment: bug1073857.diff

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8536136 [details] [diff] [review]
bug1073857.diff

Thanks!  However, your patch is missing author information, and the commit message needs to be adjusted to the right format.  Check out the guide[1] for more details.

Attach an updated patch to fix these issues and flag me for review again.  Mark the current one as outdated when you do this.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 21

[Shubham Jindal [:shubham]]

Attachment: bug1073857.diff

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8536662 [details] [diff] [review]
bug1073857.diff

Review of attachment 8536662 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this works well here!  Thanks for working on this. :)

I've pushed this to our try server to make sure it does not break any tests.  If everything looks good, you can add "checkin-needed" to keywords field of the bug someone will land this for you.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=268ff0e65bc0

Comment 23

[Shubham Jindal [:shubham]]

Thanks..:) Can you suggest me some other bugs to work on? I can now work on bugs which are not "good first bug".

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Shubham Jindal from comment #23)
> Thanks..:) Can you suggest me some other bugs to work on? I can now work on
> bugs which are not "good first bug".

You may wish to stick with that list for now, as this bug was a pretty simple change overall, and that list includes more complex ones than this.

There's also the list of mentored bugs[1] which can be more complex, but at least there's a clear mentor who can help guide you.

I would say just generally look around for bugs that interest you.

[1]: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/482d33907204

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/482d33907204