60642 - Clean up New/Rename Folder Dialogs

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[Peter "jag" Annema]

Currently the New Folder dialog and Rename Folder dialog share the same js file
and dtd file, making the code cluttered by having to use function names like
newFolderNameOnLoad and renameFolderNameOKButtonCallback to prevent clashing
with their new/rename counterpart.

Fabian ([e-mail]) has done some great work cleaning this code up, to
which I added some more.

We've renamed newFolderNameDialog.* and renameFolderNameDialog.* to
newFolderDialog.* and renameFolderDialog.*, and updated all references to these
files accordingly. The "Name" bit is just silly.

Changes made:
- moved rename* code and entities into their own files
- renamed functions to onLoad, onOK and onCancel
- removed unnecessary |if|s
- added title attribute + entity to window element (also for subscribe.xul)
- removed now unused title properties from messenger.properties
- removed title handling logic from calling code
- added doEnabling() code which enables/disables the OK button
- put doEnabling() call in the name input fields
- put doEnabling() and window.resizeToContent() calls in folder picker menu

On further browsing, there's still rename folder dialog folder picker related
code in the tree which can go.

Comment 1

[Peter "jag" Annema]

Attachment: [patch] Cleaning up code

Comment 2

[Peter "jag" Annema]

cc'ing Seth, this is his code.

Comment 3

[Fabian Guisset]

Note : applying this patch would also resolve bug 60476.
I suggest we apply this patch and not the one I proposed for bug 60476. Marking
  blocks bug 60476.
Fabian.

Comment 4

[racham]

Hi jag,

Besides addressing the the problems addressed in the bug 60476, you seemed to
have cleaned up bunch of other parts of code also. I am not too familiar with
those files and may not be able to give r= at this point.

Anyway, after appying the patch (I had to do patch manually...it was
crazy....!), I checked to see if the new folder dialog and rename folder dialogs
are behaving with proper focus. They did work fine on my WinNT box.

Things I noticed :

* In the file renameFolderDialog.js, I encountered a js error on not finding one
of the variables you were trying to print..

code :
debug("uri, name in callback = " + uri + ", " + name + "\n");

error on console:
JavaScript error:
chrome://messenger/content/renameFolderDialog.js line 53: uri is not defined

So, proceed further, I had to comment that out...

* On the RenameFolder dialog, after entering the new name and clicking on OK
button, I am running into an eventQueue assertion...

assertion:
###!!! ASSERTION: event dropped because event chain is dead: 'mElderQueue', file
d:\dbgbuild\mozilla
\xpcom\threads\nsEventQueue.cpp, line 238

The folder does get renamed...but just giving heads of on this assertion as it
looked like a possible loss of event...!

I hope I didn't mess anything up while patching the fix..

Comment 5

[(not reading, please use [e-mail] instead)]

the event queue assertions are something else, I've seen that in my tree for a
few days.

jag, can you fix the uri variable problem racham mentioned, and then post a new
patch?  (cvs -q diff -p)

for the new files, can you attach them to the bug report, along with the patch?

reminder, if you add new files, please put the proper licence at the top.

re-assigning to jag, he's working on it.

cc'ing naving.  I know he has some minor changes to some of these files.

naving, either you're going to have conflicts, or jag is.

Comment 6

[Peter "jag" Annema]

Attachment: [patch] Added licenses, fixed dump/debug

Comment 7

[Peter "jag" Annema]

cc'ed people, can I get an objection or a review here?

Comment 8

[racham]

Jag,

Your changes look good to me. r= racham.

If more changes are expected, please post them and I will go through those again.

While testing the new folder functionality...I tried to create a folder after
putting a white space in the name field for the folder and I was able to do
that. That is not good. Also, I can create folder like "foo" and "foo " which
are different logically, but they look the same for the user when viewed.

So, I was thinking, we should probably add simple validity to strip all those
white spaces and make sure we get a reasonable name from the user. Here are the
code snippets that can do that. If everyone is OK, may be we should incorporate
these.

#1
(in file newFolderDialog.js : at line 51)

  // do name validity check
  name = TrimString(name);

  if (name == "")
  {
     var warningMsg = Bundle.GetStringFromName("noNameAlert");
     window.alert(warningMsg);
     return false;
  }

(at the end of the file)
function TrimString(string)
{
    if (!string) return "";
    return string.replace(/(^\s+)|(\s+$)/g, '');
}

#2
(in file messenger.properties : at line 33)
noNameAlert=Please enter a valid folder name.

bhuvan

Comment 9

[racham]

Adding myself to the cc list.

Comment 10

[Fabian Guisset]

Jag, how about we add a fix for bug 44104 (although I can't reproduce it myself) 
at the same time? It's about new folders that have the same name.
Thanks for your time Rachan. :-)
Fabian.

Comment 11

[racham]

Sorry, I forgot to add s small piece to the proposed solution to display alert
for those who enters a space (or bunch of spaces as folder name)...

(in newFolderDialog.js : line 5)
var Bundle = srGetStrBundle("chrome://messenger/locale/messenger.properties");

BTW, in the Rename dialog, if user enters a space as new name, an error dialog
is put up and looks like that is taken care of.

Comment 12

[Peter "jag" Annema]

Fabian: let's do this in small steps :-)

racham: hrm... maybe we should more clearly show leading/trailing spaces,
instead of forbidding them.

Alternatively, if we are going to strip them, I suggest we do it silently since
they were most likely put there accidentily. If people put them there
consciously because they wanted to have both "foo" and "foo " they'll get a
``Folder "foo" already exists'' warning.

cc'ing mpt for his input.

Comment 13

[racham]

jag,

Even if we don't want to add the whitespaces check/trim along with this 
patch, here are couple of things to note in general :

1. With the given code, stripping of whitespaces happens without any warning. 
The assumption here is that, if the user types in leading or trailing 
whitespaces, that must have been unintentional. The alert is thrown only when 
the name entered is nothing but a whitespace or bunch of whitespaces.

2. May be we should treat "foo " as an invalid name, as it is not easy to 
represent it in the UI. And more confusion will be in place if he already has 
folder "foo" and we don't warn about this new name that might look exactly the 
same when presented in the message folder pane.

3. Also, today (even with the patch posted in this bug), it is possible to 
create a folder with the name " " (a space). I agree this might not be a common 
case at all...but if we trim spaces, we catch these kind situations.

Comment 14

[Peter "jag" Annema]

racham: you're right, I misread your patch.

Actually, we could add this to doEnabling (value.search(/\S/) != -1), so they
can't press enter/click OK unless there's a non-whitespace character, at which
point leading/trailing whitespace will be trimmed (btw, I'd inline this).

Comment 15

[Matthew T (active 1999-2002)]

There are probably a couple of dozen people out there who start the names of some 
of their folders with spaces -- perhaps as a hackish method of making them appear 
first in the folder list (and just allowing custom folder orders in Mozilla only 
won't help, because IMAP accounts can be accessed with multiple clients). There's 
no real reason to remove this ability from them. And if the IMAP standard allows 
folder names to have leading or trailing whitespace, then Mozilla should too 
(since a folder can be moved from Local Mail to an IMAP account or vice versa).

I believe a better approach would be to show spaces as bullets whenever the 
folder name is displayed -- whether in the folder pane, or the Move to Folder 
submenu, or whatever. That way spaced-out folders would be obviously different 
from their unspaced counterparts, without placing an artificial restriction on 
the folder name.

Comment 16

[Fabian Guisset]

After discussing this with alecf, jag and who else, we decided not to check for
leading/trailing whitespaces, and eventually if "we are really really bored", we
might do the bullet thing, but not in this patch. We are thus looking for a r=
and sr= for this patch :)
Thank you,
Fabian.

Comment 17

[Peter "jag" Annema]

racham, does your r= count for the patch as is? Your comment seems to suggest as
much, with the whitespace checking as an additional (but not required for r=)
change...

Comment 18

[racham]

yes.

r=racham for the patch (id# : 19550).

Whitespace check is brought up as an discussion to see the possiblity of getting
that in, incase we think it is appropriate so that we can couple it. We need a
separate bug to tackle the whitespace issues.

Comment 19

[Peter "jag" Annema]

Ben gave a= on the condition of removing debug.
I'll check this patch in tomorrow.

Comment 20

[Peter "jag" Annema]

Fix checked in.

Comment 21

[racham]

sheela,

code verified..you can go ahead.

bhuvan

Comment 22

[Sheela Ravindran]

marking bug as verified
thanks bhuvan