195655 - Add new smilies to message compose / message display

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Syd Logan]

Comment 1

[Syd Logan]

Attachment: Patch to add the nine new smilies

Comment 2

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

Comment on attachment 116066 [details] [diff] [review]
Patch to add the nine new smilies

requesting review from brade (the editor front end module owner)

1)

+  PRUnichar temp;

   // temporary variable used to store the glyph html text
   nsAutoString outputHTML;
+  PRBool bTestSmilie = PR_FALSE;
+  PRBool bArg = col0;


+  temp = (col0 ? text0 : text1);

I suggest something like:

PRUnichar firstChar = (col0 ? text0 : text1);

(all in one line, better variable name)


2)

+  if ( PR_TRUE == bTestSmilie && (

just do

if (bTestSmilie && (

3)

To avoid duplication and potential copy and paste error, instead of

+	   SmilyHit(aInString, aInLength, bArg,
		    NS_LITERAL_STRING(":-)").get(),  3,
		    "<img src=\"chrome://editor/content/images/smile_n.gif\"
alt=\":-)\" class=\"moz-txt-smily\" height=19 width=19 align=ABSCENTER>",
		    outputHTML, glyphTextLen)	   

consider

+	   SmilyHit(aInString, aInLength, bArg, ":-)", "smile_n.gif",
outputHTML, glyphTextLen)

And in the SmilyHit() code, you could determin the length, build up the img
tag.

4)

don't check in the Makefile stuff

Comment 3

[Kathleen :Brade]

Comment on attachment 116066 [details] [diff] [review]
Patch to add the nine new smilies

I'm not really happy to have the smiley stuff in EditorContent.css or
editorFormatToolbar.css

Can you clean up all of the SmilyHit calls to somehow be a loop or something
less verbose/bloated?

I'm denying r= on this particular patch since it includes bogus makefile
changes at the beginning.

Comment 4

[Kathleen :Brade]

-->Mailnews since this isn't a composer feature

Comment 5

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

this UI won't appear in Editor, so removing that from the summary.

Comment 6

[Syd Logan]

Attachment: Incorporate comments from Seth to clean things up even more

Incorporate comments from Seth to clean things up even more

Comment 7

[Kathleen :Brade]

Comment on attachment 116489 [details] [diff] [review]
Incorporate comments from Seth to clean things up even more

I don't have time to do a thorough review right now but here are a couple
things I noticed:

>Index: editor/ui/composer/content/ComposerCommands.js
>@@ -3049,10 +3049,36 @@
...
>         case ":-\\": strSml="s7";
>         break;
>+        case ":-\\": strSml="s8";
>+        break;
>+        case ":-\\": strSml="s9";
>+        break;

Am I missing something or are the above 3 cases identical?


...
>+        case ">:o": 
>+        case ">:-o":
>+            strSml="s10";
>+            break;

Put the break in the similar place as done in surrounding code (even tho it
seems odd in this case).


...
>+        case ":-X": strSml="s16";
>+        break;
>+
>+

remove the extra blank line above; one should be sufficient



>Index: editor/ui/composer/content/editorSmileyOverlay.xul
>@@ -55,11 +55,30 @@
...
>-                oncommand="doStatefulCommand('cmd_smiley', ':-\\' )" />           
>+                oncommand="doStatefulCommand('cmd_smiley', ':-\\' )" /> 

remove the extra space at the end of the above line


>@@ -77,10 +96,28 @@
...
>+                    oncommand="doStatefulCommand('cmd_smiley', ':\'(' )" accesskey="&smiley15Cmd.accesskey;"/>           

remove the blank spaces at the end of the above line
the quoting isn't correct above; you can't use ' character inside a string
quoted with single quotes.


>Index: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
I'd still like to see a table here but I defer to others since I'm not a module
owner for this code.


>Index: themes/modern/editor/editorFormatToolbar.css
I'd really like to see all of the smiley stuff pulled out of this file and put
into a css file of its own.

Comment 8

[Syd Logan]

Comment on attachment 116489 [details] [diff] [review]
Incorporate comments from Seth to clean things up even more

Removed makefile stuff, additional factoring of the original moz code
per-request.

Comment 9

[Syd Logan]

Comment on attachment 116489 [details] [diff] [review]
Incorporate comments from Seth to clean things up even more

Good catch Kathy on the duplicate in the case statement.

Comment 10

[Kathleen :Brade]

Comment on attachment 116489 [details] [diff] [review]
Incorporate comments from Seth to clean things up even more

syd told me he has more changes coming; I still see the duplicate cases in this
bug (in ComposerCommands.js: s7, s8 and s9 are all the same string)

Comment 11

[Syd Logan]

Attachment: Incorporate comments from Kathy and restore original logic of testing both cases

I found out after more exhaustive unit testing that both of those long if
statements that I combined as one need to be executed in serial. So, I now loop
twice, testing each condition separately based on the loop variable, and
ensuring that the original logic is preserved. That's what the while loop is
for. I don't know the history of this need to do both of these tests each time
the function is called, I suspect that the original coder got his review and
superreview with merit. We have a few options. I can keep this loop, and my
nice optimizations (related to duplicate code), and land it. Or, I can factor
out the code in the loop into a function and call it from here, using back to
back ifdef's like the original code had. Or, I can undo all my work so far and
just add in the smilies the old way. Or, I can assign this bug to someone else,
cause I need to move on :-)

Comment 12

[Syd Logan]

er, s/back to back ifdef's/back to back if statements/ in the previous post.

Comment 13

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

1)
  
+  nsAutoString str;
+  str.AssignWithConversion(tagTXT);
+  PRUnichar *uniTagTXT = ToNewUnicode( str );
+  PRBool retval;
+  PRInt32  tagLen = nsCRT::strlen(uniTagTXT);

Why the conversion and the allocation?

why not just:

+ PRInt32  tagLen = strlen(tagTXT);

and later, instead of passing uniTagTXT to ItMatchesDelimited(), which takes a
const PRUnichar *, do something like:


+        && ItMatchesDelimited(aInString, aLength,
NS_ConvertASCIItoUCS2(tagTXT).get(), tagLen, 
                               col0 ? LT_IGNORE : LT_DELIMITER, LT_IGNORE)

and then, you don't need the call to nsMemory::Free(uniTagTXT); and you don't
need retval.

2)

from http://www.mozilla.org/projects/xpcom/string-guide.html

"Avoid *WithConversion functions at all costs: AssignWithConversion,
AppendWithConversion, EqualsWithConversion, etc "

So instead of:

+    outputHTML.AppendWithConversion(imageName);
+    outputHTML.AppendWithConversion(tagTXT);

consider:

+    outputHTML.Append(NS_ConvertASCIItoUCS2(imageName));
+    outputHTML.Append(NS_ConvertASCIItoUCS2(tagTXT));

also according to the string guide:

"Use string concatenation (i.e. the "+" operator) when combining strings."

so maybe:

+    outputHTML += NS_LITERAL_STRING("<img
src=\"chrome://editor/content/images/") + NS_ConvertASCIItoUCS2(imageName) + 
                   NS_LITERAL_STRING("\" alt=\"") +
NS_ConvertASCIItoUCS2(tagTXT) + NS_LITERAL_STRING("\" class=\"moz-txt-smily\"
height=19 width=19 align=ABSCENTER>");

But I'd need to check with dmose or dbaron, or alecf to know why + would be
better than multiple calls .Append() (or multiple calls to +=.)
I remember dmose recommending this pattern to me, but I can't remember why.

3)

no need for retval or the call to ::Free, you can just return PR_FALSE or PR_TRUE;

     glyphTextLen = (col0 ? 0 : 1) + tagLen;
-    return PR_TRUE;
+    retval = PR_TRUE;
   }
   else
-    return PR_FALSE;
+    retval = PR_FALSE;
+
+  if ( uniTagTXT )
+    nsMemory::Free(uniTagTXT);
+
+  return retval;
 }
 

4)

+  PRBool bArg;

can you come up with a better variable name, like firstColumn?

5)

the original code (and the original duplication) is hurting my head.

it looks like the original author unrolled a loop, but I'm not sure why.

Your version looks like it can be further boiled down to:

+ for (i=0;i<2;i++) {  // or use while()
+  // first pass, do...
+  // second pass, do...
+  PRUnichar firstChar = (i ? (text1) : (col0 ? text0 : text1) );
+  PRBool firstColumn = (i ? PR_FALSE ? col0);
+  if ((firstChar == ':' || firstChar == ';' || firstChar == '=' || firstChar ==
'>' || firstChar == '8' || firstChar == 'O') &&  (
+          SmilyHit(aInString, aInLength, firstColumn,
+                   ":-)",
+                   "smile_n.gif",
+                   outputHTML, glyphTextLen) ||

just trying to understand, but is it possible to hit two glyphs?  (I don't think
so.)  
if not, could we fix this code to bail out of the loop early,
skipping the second pass if the first pass had a hit?
but if you feel this would be adding more unneeded optimization, then punt on it.

Comment 14

[Syd Logan]

Attachment: Incorporate some (most) of seth's comments

Ok, I love the idea of using operator += and + like that, I did scan the online
string page and did my best to avoid those AssignWithConversions, but this is
far nicer. 

The retvals are close enough that I can agree to remove them, generally, I
would not write code with embedded returns for readability sake, but here it
seems harmless enough. I don't have a better name for that argument. As for
point #5, I'm happy with it as is -- I don't want to introduce regressions by
thinking I knew entirely what was in the mind of the creator of this code, I
just want to create a less code-duplicating homomorphism of the code that I was
forced to deal with here. A good thing to look at later, maybe you can file
another bug for someone to revisit this. But we've done a whole lot of good so
far, and I'm satified and appreciate your willingness to let it be for now.

Comment 15

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

Comment on attachment 116946 [details] [diff] [review]
Incorporate some (most) of seth's comments 

r=sspitzer, assuming this is well tested.

let's get brade to r= the editor changes.

Comment 16

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

not sure how this got to block 1.4 a, but clearing that to avoid the wrath of
drivers.

Comment 17

[Kathleen :Brade]

Comment on attachment 116946 [details] [diff] [review]
Incorporate some (most) of seth's comments 

r=brade with the same requests that sspitzer made (make sure this is well
tested)

Comment 18

[Syd Logan]

fixed

Comment 19

[Aleksey Nogin]

This have addte a warning on brad TBox:

+netwerk/streamconv/converters/mozTXTToHTMLConv.cpp:740
+ `PRBool bArg' might be used uninitialized in this function

Comment 20

[Syd Logan]

we only execute code using that variable when bTestSmilie is set to true, and
each time it is set to true, we initialize bArg. So, the warning is ignorable.
If you want, you can initialize it but it really doesn't matter.

Comment 21

[Daniel Schroeter]

I see often for the smily :-[ the descrition "vampire" (e.g.
http://paul.merton.ox.ac.uk/ascii/smileys.html). It's IMHO logical because the
peaks from [ are the teeth of the vampire.

For the smily :-] the symbol from :-[ would be okay. The peaks from ] are the
(red) jowl.

My suggestion: As long as there is no vampire symbol the :-[ should appear as it
is and the current symbol :-[ should be used for the :-] smily.

Comment 22

[Alfred Kayser]

A few comments:
1. smileys are 'hard' defined in editorContent.css, and are thus not themeable.
2. which is a shame, as the standard smileys are not so nice.
3. because they are defined in editorContent.css, the definitions in
messageBody.css in the theme (classic/modern) part doesn't do anything.
4. why not put the faces into one image file, with image-region clipping? 
    It saves diskspace, memoryspace, and number of files in a jar (which saves
startuptime). 

Is this worth a new bug? I do think so!

Comment 23

[Alfred Kayser]

Attachment: An example of nicer smileys, as one imagefile

Comment 24

[Alfred Kayser]

Attachment: One imagefile with all the smileys in a nicer make-up

Comment 25

[Syd Logan]

Please file a new bug on the themeability. But don't assign it to me, I won't be
able to work on it.

Comment 26

[Alfred Kayser]

Bug 199958 created for the themeability of the smiley's 
(and to remove the duplicating of smileys in both the themea and in content).

Comment 27

[esther]

Using trunk build 20030331 on winxp, macosx and linux the (9) new smilies are in
the smilie list and can be add to a mail compose message.  Sending and Receiving
them is OK.   Verified for first set of new smilies being included in toolbar
and basic display in message.