768919 - Refine the theme of chat messages

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Jb Piacentino]

IRC conversations do not need fancy layout, and probably not big avatars, but do need color codes.
I think we should 
- either remove avatars or display a very short (as opposed to tall) version of it
- shrink space between exchanges
- add color codes in the participants list and in the message author header

Comment 1

[Andreas Nilsson (:andreasn)]

Attachment: IRC mockup

IRC mockup

Comment 2

[Andreas Nilsson (:andreasn)]

In the mockup above, I've added color codes for your own name (to tell apart your own sentences from others) and when someone mentions your name. Status messages are gray.

Comment 3

[Florian Quèze [:florian]]

(In reply to Andreas Nilsson (:andreasn) from comment #2)
> In the mockup above, I've added color codes for your own name (to tell apart
> your own sentences from others) and when someone mentions your name.

Why not a color for each nick? (nicks are already colored in the list of participants)

Comment 4

[Andreas Nilsson (:andreasn)]

> Why not a color for each nick? (nicks are already colored in the list of
> participants)

Yeah, I was thinking that too at first, but I'm not really sure how much value that actually adds and if it's actually just adds noise to the conversation. If we only use it for your own nick and when people message you, we would get more out of the color coding, since those two are more crucial, especially the name highlight.

Comment 5

[Patrick Cloke [:clokep]]

(In reply to Andreas Nilsson (:andreasn) from comment #4)
> > Why not a color for each nick? (nicks are already colored in the list of
> > participants)
> 
> Yeah, I was thinking that too at first, but I'm not really sure how much
> value that actually adds and if it's actually just adds noise to the
> conversation.
I generally find it really difficult to follow an IRC conversation without color. Color is an important piece of information that quickly lets you discern patterns and group messages together.

> If we only use it for your own nick and when people message
> you, we would get more out of the color coding, since those two are more
> crucial, especially the name highlight.
There could be another way to differentiate your messages and pings besides just color, no? (Bold, bright colors, border, I don't know...)

Comment 6

[Florian Quèze [:florian]]

(In reply to Andreas Nilsson (:andreasn) from comment #4)
> > Why not a color for each nick? (nicks are already colored in the list of
> > participants)
> 
> Yeah, I was thinking that too at first, but I'm not really sure how much
> value that actually adds and if it's actually just adds noise to the
> conversation.

Colors are very very useful, they make the conversation readable, it's definitely not noise.

Comment 7

[Jb Piacentino]

I think it would be very useful to have a color for each nick, based on the colors in the list of participants.

When it comes to highlighting a message which mentions your name (or a searched word, or a permanent search - I thnk Instant Bird has this but not sure TB), maybe you could mark the word in bold + some sort of background for the message.  

Otherwise, I really like the compressed layout. good job!

Comment 8

[Jb Piacentino]

(In reply to Jb Piacentino from comment #7)
.. 
> searched word, or a permanent search
I meant Tracked words

Comment 9

[Andreas Nilsson (:andreasn)]

All right, colors it is. I'll update the mockups and start the implementation.

Comment 10

[Richard Marti (:Paenglab)]

As I understand this bug, this new layout should only be for IRC? If yes and if this should be done through CSS we need a selector like #Chat[account="IRC"]... to apply this only for IRC. Maybe this could also help for other accounts in the future to style account specific things.

For the color it exists on the bubbles a sendercolor="color: hsl(113, 100%, 40%);" etc. We could code like:

.message[sendercolor="color: hsl(113, 100%, 40%);"] .sender {
  color: hsl(113, 100%, 40%);
}

I don't know how many different colors are used but this would need for every known color such a rule.
I think it would be better to change the logic for the, on TB unused, sendercolor to style directly the color for .sender. This would then also work on other accounts.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Florian:

Hm - DOMi isn't showing me anything that Richard could use as a selector to make IRC conversations distinct. Is there something we could use that I'm not seeing, or do we need to special case IRC conversations somehow?

-Mike

Comment 12

[Patrick Cloke [:clokep]]

(In reply to Mike Conley (:mconley) from comment #11)
> Hm - DOMi isn't showing me anything that Richard could use as a selector to
> make IRC conversations distinct. Is there something we could use that I'm
> not seeing, or do we need to special case IRC conversations somehow?

So there's currently no way of special casing a conversation like this via the Adium message style API we implement (see [1]). It should be possible to modify fairly easily I 'pose. [2] would need to add the protocol ID so it's available in the message style. That method seems to have a conversation as an input...so it should be possible.

[1] https://wiki.instantbird.org/Instantbird:Message_Styles_reference
[2] http://lxr.instantbird.org/instantbird/source/chat/modules/imThemes.jsm#601

Comment 13

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #12)
> (In reply to Mike Conley (:mconley) from comment #11)
> > Hm - DOMi isn't showing me anything that Richard could use as a selector to
> > make IRC conversations distinct. Is there something we could use that I'm
> > not seeing,

Mike, DOMi isn't very helpful for message theme as only the HTML that we intend to use is actually inserted in the document through text replacements. They are documented at https://wiki.instantbird.org/Instantbird:Message_Styles:replacements
 
> So there's currently no way of special casing a conversation like this via
> the Adium message style API we implement (see [1]).

You can't special case the conversation for a protocol, for good reasons (on Instantbird the protocol used for a conversation can change in the middle of a conversation), but you can easily special case messages per protocol with the %service% replacement, defined at http://lxr.instantbird.org/instantbird/source/chat/modules/imThemes.jsm#418

To use this, you would do:
-<div class="bubble %messageClasses%" senderColor="%senderColor%">
+<div class="bubble %messageClasses%" senderColor="%senderColor%" prpl="%service%">
in mail/components/im/messages/Incoming/Content.html and mail/components/im/messages/Incoming/Context.html

Comment 14

[Florian Quèze [:florian]]

(In reply to Richard Marti [:paenglab] from comment #10)

> For the color it exists on the bubbles a sendercolor="color: hsl(113, 100%,
> 40%);" etc. We could code like:
> 
> .message[sendercolor="color: hsl(113, 100%, 40%);"] .sender {
>   color: hsl(113, 100%, 40%);
> }

You can just move the %sendColor% replacement in Content.html and Context.html to wherever you need it.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: patch

Florian, thank you for the tips.

On the contrary to the mockup every message is in their border. This is because the IRC attribute is on every message and not global and the events have no attribute and the topic could be never differently styled. But with this patch the IRC don't look fully different to the other protocols.

I gave the nick names a maximal length of 100px. Longer nicks have a ellipsis (crisccoulson is fully shown but on the limit). I can't do this width dynamically because of the structure of this messages.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: Patch in action

Screenshot for easier ui-review (Blake I saw your tweet ;) )

Comment 17

[Richard Marti (:Paenglab)]

Attachment: patch v2

After the patch upload I tried it with live IRC and found the attribute has changed from "irc" to "IRC". This patch checks now both (historic conversations are using lowercase and live uppercase). I also removed a calc() because TB 15 and 16 knows only -moz-calc(). With the again smaller font for the pseudo where is also no calculation needed. The pseudo is now centered to first line's text.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: Screenshot with patch v2

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 647936 [details] [diff] [review]
patch v2

This looks like a giant step forward to me.  I do wonder if we should do everything like IRC, and only have Twitter be the different one, but I'm going to say ui-r=me, and we can fix that later, if other people agree.

Thanks,
Blake.

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 647936 [details] [diff] [review]
patch v2

Sorry, wrong Florian…  If only fqueze added "[:florian]" to his name, this wouldn't have happened…  :(

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 647936 [details] [diff] [review]
patch v2

I tested the patch. To compare, I took a screenshot of the same conversation both with and without the patch.
Without: http://i.imgur.com/Bk5Hn.png
With patch: http://i.imgur.com/XOptp.png

One of the goals of this bug was to reduce the wasted space so that more messages could be displayed at once, which is especially important on IRC as busy channels may have lots of messages arriving quickly.

The patch seems to improve things a bit for this situation as there's one more line of content displayed with the patch, but I think we can still do much better.

A few random ideas to save space:
- reduce or remove the margins on the left side and right sides of the bubbles
- reduce the spacing between the text of system messages and their borders.
- Without the patch it seems the second line of text can go under the time stamp (not very visible on my screenshot), with the patch it doesn't, this creates a blank column on the right side and the timestamps end up taking a lot of space.
- I'm not a fan of the column on the left side for the nicknames that also takes a lot of space, but that's a bit a matter of taste as I know some people would like it.

Comment 22

[Andreas Nilsson (:andreasn)]

I discussed this bug with florian on IRC earlier and it feels like in order to get this bug right, we need to get rid of the bubbles since it's eating too much space. I'm going to create a patch that reuses a bunch of code from the Instantbird theme Simple and will aim at something that is closer to the initial mockup. Still need to figure out what to do in the Twitter case, since I agree that's the one place where we need avatars for sure.

Comment 23

[Blake Winton (:bwinton) (:☕️)]

I don't think anyone would object too heavily to leaving the bubbles for the Twitter theme, and I believe we can use this patch to filter for [prpl="twitter"] (or something like that) to only apply those styles for that type…

Comment 24

[Andreas Nilsson (:andreasn)]

Attachment: simple irc v1

Works ok for IRC, but needs work to fit well for Twitter

Comment 26

[Andreas Nilsson (:andreasn)]

Attachment: simple irc v2

Patch in progress. Some stupid alignment issues still, but totally usable.

Comment 27

[Andreas Nilsson (:andreasn)]

Attachment: simple irc v3

Blake suggested doing double twitter messages as one, but I'm sceptical to that as each little tweet is it's own thing, so I ended up not doing that.

Still does some ugly overflow if your irc nick is too long, but apart from that all should work well.

Comment 28

[Richard Marti (:Paenglab)]

Attachment: simple irc v3.1

I added a overflow and ellipsis to the sender name and gave a 3px gap to separate the message.

What I'm seeing always after the messages is the .lastMessage with the accumulating elapsed time I'm connected. Shouldn't this only show the time since the last message?

Comment 29

[Patrick Cloke [:clokep]]

(In reply to Richard Marti [:paenglab] from comment #28)
> What I'm seeing always after the messages is the .lastMessage with the
> accumulating elapsed time I'm connected. Shouldn't this only show the time
> since the last message?

I think this code is based off of (Time) Bubbles from Instantbird, which shows the elapsed time between any two messages (that is over 5 minutes). It doesn't look like that part of the code was changed (comparing [1] to [2]).

So it seems to be expected (by the code), but maybe that's not the design you're going for. :)

[1] http://mxr.mozilla.org/comm-central/source/mail/components/im/messages/Footer.html?force=1
[2] http://lxr.instantbird.org/instantbird/source/instantbird/themes/messages/bubbles/Footer.html?raw=1

Comment 30

[Richard Marti (:Paenglab)]

Attachment: simple irc v3.1

Sorry to spam this bug but the in my last patch added span isn't needed. The '#chat .pseudo' is enough to override the '#chat *' overflow rule. So my change is only in CSS.

Comment 31

[Andreas Nilsson (:andreasn)]

Comment on attachment 652426 [details] [diff] [review]
simple irc v3.1

Setting up for review.

Comment 32

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 652426 [details] [diff] [review]
simple irc v3.1

Getting closer, but not quite there yet.

1) http://dl.dropbox.com/u/2301433/Screenshots/NonElvis.png
   The usericons need explicit width and height, I think.

2) mail/components/im/messages/main.css
   >@@ -101,78 +93,43 @@
   >+.incoming:-moz-any([prpl="Twitter"], [prpl="twitter"]) .pseudo:after,
   >+.incoming:-moz-any([prpl="Twitter"], [prpl="twitter"]) .pseudo:after {
   >+  content: "none";
   That should be "content: none", without the quotes around "none".  (But Paenglab tells me it should be removed entirely.)

3) http://dl.dropbox.com/u/2301433/Screenshots/StrangeMargins.png
   It looks like the first line is wrapping incorrectly.  We should do something better there.

4) The "12 minutes" at the end of irc channels should be a lighter shade.  Perhaps use the one from the timestamps?
   For that matter, what is that 16 minutes anyways?  Should we just hide it for IRC?

5) http://dl.dropbox.com/u/2301433/Screenshots/Yellow.png
   Could we get some colours that aren't yellow?

6) http://dl.dropbox.com/u/2301433/Screenshots/CrazyIndent.png
   I have no idea what happened here, but that's terrible.

Okay, okay, I've complained a bunch, but I want to end off by saying that I really do like the new style, and am looking forward to ui-reviewing the next version!

Thanks,
Blake.

Comment 33

[Richard Marti (:Paenglab)]

Attachment: simple irc v3.3

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #32)
> Comment on attachment 652426 [details] [diff] [review]
> simple irc v3.1
> 
> Getting closer, but not quite there yet.
> 
> 1) http://dl.dropbox.com/u/2301433/Screenshots/NonElvis.png
>    The usericons need explicit width and height, I think.

Fixed

> 2) mail/components/im/messages/main.css
>    >@@ -101,78 +93,43 @@
>    >+.incoming:-moz-any([prpl="Twitter"], [prpl="twitter"]) .pseudo:after,
>    >+.incoming:-moz-any([prpl="Twitter"], [prpl="twitter"]) .pseudo:after {
>    >+  content: "none";
>    That should be "content: none", without the quotes around "none".  (But
> Paenglab tells me it should be removed entirely.)

Fixed

> 3) http://dl.dropbox.com/u/2301433/Screenshots/StrangeMargins.png
>    It looks like the first line is wrapping incorrectly.  We should do
> something better there.

Fixed, it looks like Andreas planned for Twitter the old behavior of pseudo/ date on first line and text on second line. It works now like this.

> 4) The "12 minutes" at the end of irc channels should be a lighter shade. 
> Perhaps use the one from the timestamps?

Gave the same color as on old theme and centered it.

>    For that matter, what is that 16 minutes anyways?  Should we just hide it
> for IRC?

It looks now it shows the time since connect. Before it showed the time since the last message.

> 5) http://dl.dropbox.com/u/2301433/Screenshots/Yellow.png
>    Could we get some colours that aren't yellow?
> 
> 6) http://dl.dropbox.com/u/2301433/Screenshots/CrazyIndent.png
>    I have no idea what happened here, but that's terrible.

Could you check with DOMi what happened?

Comment 34

[Richard Marti (:Paenglab)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #32)
> Comment on attachment 652426 [details] [diff] [review]
> 
> 5) http://dl.dropbox.com/u/2301433/Screenshots/Yellow.png
>    Could we get some colours that aren't yellow?

It looks the color is computed based on the nick: http://mxr.mozilla.org/comm-central/search?string=Compute+the+color+based+on+the+nick

I don't know when each one is used of this two files.

Comment 35

[Blake Winton (:bwinton) (:☕️)]

I wonder if it would look nicer to just pick 25-ish fairly different colours, and use those.  i.e. https://github.com/bwinton/LimeChat-Themes/blob/master/Sample.css#L42 or https://github.com/bwinton/LimeChat-Themes/blob/master/Whisper.css#L163

Comment 36

[Mike Conley (:mconley) (:⚙️)]

With this patch, my tweets don't appear to be styled...

See: http://i.imgur.com/PPw2n.png

Comment 37

[Andreas Nilsson (:andreasn)]

(In reply to Mike Conley (:mconley) from comment #36)
> With this patch, my tweets don't appear to be styled...
> 
> See: http://i.imgur.com/PPw2n.png

I haven't been able to duplicate that behaviour, but I'll do some more testing and see why that is.

Comment 38

[Richard Marti (:Paenglab)]

Attachment: simple irc v4

This patch should address all issues except the sender color (bug 783759). I'll let Andreas test this patch before I'm asking for review.

This patch is using again the class bubble and the HRs to be compatible with the JS making the lastMessage and the show/hide events. I think it's better now to play with CSS instead of changing the JS and maybe introduce some regressions when TB15 will be released in some days. If needed this can be changed in later versions.

Comment 39

[Richard Marti (:Paenglab)]

Attachment: simple irc v4

No negative feedback from Andreas so asking for review. The only change to previous patch is hiding the interval messages. This includes the lastMessage.

Comment 40

[Andreas Nilsson (:andreasn)]

Comment on attachment 647936 [details] [diff] [review]
patch v2

Marking as review minus as we have newer patches.

Comment 41

[Florian Quèze [:florian]]

(In reply to Richard Marti [:paenglab] from comment #39)
> Created attachment 653076 [details] [diff] [review]

> The only change to
> previous patch is hiding the interval messages. This includes the
> lastMessage.

Why?
(and if you remove that, you should also remove the JS code generating these messages)

Also, attachment 653076 [details] [diff] [review] and attachment 653046 [details] [diff] [review] are identical.

Comment 42

[Florian Quèze [:florian]]

Comment on attachment 653076 [details] [diff] [review]
simple irc v4

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

::: mail/components/im/messages/Info.plist
@@ +6,5 @@
> +	<string>%message%</string>
> +	<key>CFBundleDevelopmentRegion</key>
> +	<string>English</string>
> +	<key>CFBundleGetInfoString</key>
> +	<string>Instantbird Minimal Message Style</string>

This should be changed.

@@ +8,5 @@
> +	<string>English</string>
> +	<key>CFBundleGetInfoString</key>
> +	<string>Instantbird Minimal Message Style</string>
> +	<key>CFBundleIdentifier</key>
> +	<string>org.instantbird.minimal.message.style</string>

And this too.

org.mozilla.thunderbird.message.style maybe?

Comment 43

[Richard Marti (:Paenglab)]

(In reply to Florian Quèze [:florian] [:flo] from comment #41)
> (In reply to Richard Marti [:paenglab] from comment #39)
> > Created attachment 653076 [details] [diff] [review]
> 
> > The only change to
> > previous patch is hiding the interval messages. This includes the
> > lastMessage.
> 
> Why?
> (and if you remove that, you should also remove the JS code generating these
> messages)

As I wrote, because TB15 will be made tomorrow I don't want to introduce regressions.

> Also, attachment 653076 [details] [diff] [review] and attachment 653046 [details] [diff] [review]
> [details] [diff] [review] are identical.

Correct, I'll add the right one this evening.

Comment 44

[Florian Quèze [:florian]]

(In reply to Richard Marti [:paenglab] from comment #43)
> (In reply to Florian Quèze [:florian] [:flo] from comment #41)
> > (In reply to Richard Marti [:paenglab] from comment #39)
> > > Created attachment 653076 [details] [diff] [review]
> > 
> > > The only change to
> > > previous patch is hiding the interval messages. This includes the
> > > lastMessage.
> > 
> > Why?
> > (and if you remove that, you should also remove the JS code generating these
> > messages)
> 
> As I wrote, because TB15 will be made tomorrow I don't want to introduce
> regressions.

Keeping JS code that calls scrollIntoView on an element that you are hiding seems quite dangerous to me ;).

Comment 45

[Richard Marti (:Paenglab)]

(In reply to Florian Quèze [:florian] [:flo] from comment #44)
> Keeping JS code that calls scrollIntoView on an element that you are hiding
> seems quite dangerous to me ;).

I'm not a JS programmer and would more break than improve ;)

You know the code best. Could you remove it, then we are sure the correct code is removed?

Comment 46

[Florian Quèze [:florian]]

(In reply to Richard Marti [:paenglab] from comment #45)
> (In reply to Florian Quèze [:florian] [:flo] from comment #44)
> > Keeping JS code that calls scrollIntoView on an element that you are hiding
> > seems quite dangerous to me ;).
> 
> I'm not a JS programmer and would more break than improve ;)
> 
> You know the code best. Could you remove it, then we are sure the correct
> code is removed?

Yes, I'm going to attach a patch soon. I'm reviewing the CSS code in main.css now.

Comment 47

[Florian Quèze [:florian]]

Attachment: Patch v5

Check pointing. I edited Info.plist, removed the JS code related to time intervals from Footer.html, cleaned up the usage of "#chat " from main.css, removed the CSS related to time intervals, and removed the bubble class from Status.html

Comment 48

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 653076 [details] [diff] [review]
simple irc v4

It looks like you're still iterating on the patch, so I'm clearing out these requests…

Comment 49

[Florian Quèze [:florian]]

Attachment: Patch v6

- Removed the <div class="message"> and <span class="message"> elements that were redundant with the <span class="ib-msg-txt"> that's already added by imThemes.jsm.

- Removed the rules related to the .action class that were left overs from the Simple Instantbird theme, and are now irrelevant.

- Avoided the expensive descendant selector wherever possible.

If we weren't so close to releasing, I would also try to get rid of the hr tags that are currently hidden, but still referenced by some JS code, and I would try to get rid of the "p.event" override for a rule in ".event" (Both of these things are leftover from the Bubbles theme from Instantbird).

But as we are running out of time, I think I'm done editing this and it should be ready for review.

Comment 50

[Andreas Nilsson (:andreasn)]

Works most excellent! We can file followup bugs about the remaining issues.

Comment 51

[Florian Quèze [:florian]]

Attachment: Patch v6.1

Removed leftover time= attributes in Status.html and NextStatus.html, and fixed the Context.html file to match Content.html + the "context" class.

Comment 52

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 653362 [details] [diff] [review]
Patch v6.1

From what I've seen so far, I like it.  ui-r=me, and even more so if we show the username on all irc actions (as discussed in irc).

Comment 53

[Florian Quèze [:florian]]

Attachment: Patch v6.2

Just changed the .next selector to message:not(.action) > .next to display the username on all action messages (as mentioned in comment 52).

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 653377 [details] [diff] [review]
Patch v6.2

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

Drive-by - just a few comments / questions.

::: mail/components/im/messages/Incoming/Content.html
@@ +1,1 @@
> +<div class="%messageClasses%" prpl="%service%"><img src="%userIconPath%" class="usericon"/><div class="date">%time%</div><div class="pseudo" style="%senderColor%">%sender%</div>%message%</div>

I'd prefer if this was formatted properly, as opposed to a single line. Or are we optimizing for something here?

::: mail/components/im/messages/Info.plist
@@ +2,5 @@
>  <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
>  <plist version="1.0">
>  <dict>
> +	<key>ActionMessageTemplate</key>
> +	<string>%message%</string>

These need to be indented properly.

::: mail/components/im/messages/main.css
@@ +126,5 @@
>  }
>  
> +/* We style Twitter differently from other types of chat */
> +
> +.message:-moz-any([prpl="Twitter"], [prpl="twitter"]) {

Hrm. So, in some cases, %service% will get passed twitter, and other times, it's passed Twitter?

That doesn't sound right. If we have time, I'd want to make it so that we have consistent output coming out of %service%...

Comment 55

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #54)

> ::: mail/components/im/messages/Incoming/Content.html
> @@ +1,1 @@
> > +<div class="%messageClasses%" prpl="%service%"><img src="%userIconPath%" class="usericon"/><div class="date">%time%</div><div class="pseudo" style="%senderColor%">%sender%</div>%message%</div>
> 
> I'd prefer if this was formatted properly, as opposed to a single line. Or
> are we optimizing for something here?

This line will be parsed and inserted in the HTML document once for each message, so avoiding the whitespace saves a significant number of DOM Text nodes. I'm not sure if it's a significant memory saving or not.

> 
> ::: mail/components/im/messages/Info.plist
> @@ +2,5 @@
> >  <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
> >  <plist version="1.0">
> >  <dict>
> > +	<key>ActionMessageTemplate</key>
> > +	<string>%message%</string>
> 
> These need to be indented properly.

What's wrong with the indentation? Or do you just mean you would like spaces instead of the tabs?

> ::: mail/components/im/messages/main.css
> @@ +126,5 @@
> >  }
> >  
> > +/* We style Twitter differently from other types of chat */
> > +
> > +.message:-moz-any([prpl="Twitter"], [prpl="twitter"]) {
> 
> Hrm. So, in some cases, %service% will get passed twitter, and other times,
> it's passed Twitter?

It will be Twitter for ongoing conversations and twitter when displaying a logged conversation.

The value of service is defined at http://hg.mozilla.org/comm-central/annotate/8e502ee509af/chat/modules/imThemes.jsm#l418 to aMsg.conversation.account.protocol.name

The problem is that the only value that is stored in the log files is account.protocol.normalizedName and getting the protocol's name from the protocol's normalized name isn't easy.

Possible hacks:
- call toLowerCase in the service: function of imThemes.jsm. This isn't a correct fix as there's no guarantee that the normalized name is just the lowercase version of the name (the name is intented to be displayable for the user. For example for Google Talk the name is "Google Talk" and the normalized name is "gtalk"). It would be enough to simplify our CSS here though.
- make service: return the protocol's normalized name all the time. I think this would be abusing the original intent of the %service% replacement (I think it was designed to let theme authors show to the user which protocol a message has transiting through), but I don't mind too much as I'm not aware of any message theme in the wild actually doing something useful with the current %service% replacement. [I think I would suggest following this route, but not in comm-central, not in the last beta before we release.]
- a "fix" could have been to store the protocol name in addition to the normalized name in the meta data of the logs, but I'm afraid it's too late for that, as there's already been several betas producing logs with protocol name.

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Hm - I'm seeing some formatting weirdness in some cases on Windows 7.

STR:

1) In IRC chat or GTalk chat, put a smiley on its own line
2) Then type a line of text

What happens?

http://i.imgur.com/Rt7QP.png

What's expected?

Not the above.

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Note that the smiley bug only seems to appear if the smiley is by itself. If the smiley appears after text, the bug doesn't manifest.

Comment 58

[Florian Quèze [:florian]]

Attachment: Patch v6.3

This should fix the issue reported in comment 56. Thanks for catching this Mike!

The only change is:
+.message {
+  clear: both;
+}

Comment 59

[Mike Conley (:mconley) (:⚙️)]

(In reply to Florian Quèze [:florian] [:flo] from comment #55)
> 
> > 
> > ::: mail/components/im/messages/Info.plist
> > @@ +2,5 @@
> > >  <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
> > >  <plist version="1.0">
> > >  <dict>
> > > +	<key>ActionMessageTemplate</key>
> > > +	<string>%message%</string>
> > 
> > These need to be indented properly.
> 
> What's wrong with the indentation? Or do you just mean you would like spaces
> instead of the tabs?

Yes, please. :)

> 
> > ::: mail/components/im/messages/main.css
> > @@ +126,5 @@
> > >  }
> > >  
> > > +/* We style Twitter differently from other types of chat */
> > > +
> > > +.message:-moz-any([prpl="Twitter"], [prpl="twitter"]) {
> > 
> > Hrm. So, in some cases, %service% will get passed twitter, and other times,
> > it's passed Twitter?
> 
> It will be Twitter for ongoing conversations and twitter when displaying a
> logged conversation.
> 
> The value of service is defined at
> http://hg.mozilla.org/comm-central/annotate/8e502ee509af/chat/modules/
> imThemes.jsm#l418 to aMsg.conversation.account.protocol.name
> 
> The problem is that the only value that is stored in the log files is
> account.protocol.normalizedName and getting the protocol's name from the
> protocol's normalized name isn't easy.
> 
> Possible hacks:
> - call toLowerCase in the service: function of imThemes.jsm. This isn't a
> correct fix as there's no guarantee that the normalized name is just the
> lowercase version of the name (the name is intented to be displayable for
> the user. For example for Google Talk the name is "Google Talk" and the
> normalized name is "gtalk"). It would be enough to simplify our CSS here
> though.
> - make service: return the protocol's normalized name all the time. I think
> this would be abusing the original intent of the %service% replacement (I
> think it was designed to let theme authors show to the user which protocol a
> message has transiting through), but I don't mind too much as I'm not aware
> of any message theme in the wild actually doing something useful with the
> current %service% replacement. [I think I would suggest following this
> route, but not in comm-central, not in the last beta before we release.]
> - a "fix" could have been to store the protocol name in addition to the
> normalized name in the meta data of the logs, but I'm afraid it's too late
> for that, as there's already been several betas producing logs with protocol
> name.

Yeah, that's fair. I figured it'd be too deep a fix for this late in the game. No worries. But maybe this is something that should be filed for later...

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 653400 [details] [diff] [review]
Patch v6.3

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

For what it's worth, this patch looks pretty good to me.

Comment 61

[Florian Quèze [:florian]]

Attachment: Patch v6.4

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Same patch without the tabs that Mike disliked.
This patch has already been reviewed by Mike and me, and ui-reviewed by Blake.

Comment 62

[Florian Quèze [:florian]]

https://hg.mozilla.org/comm-central/rev/7af51ca03270
https://hg.mozilla.org/releases/comm-aurora/rev/d59b478123db
https://hg.mozilla.org/releases/comm-beta/rev/c039df929fc8

Comment 63

[bastien.rene]

I didn't read all the comments, but I think there is still a problem. I read that the new theme was designed for IRC, but i have that theme in Gmail conversations, and IMHO the older one was better for Gmail ;)

Is there any problem to put the older theme in Gmail conversations?

Comment 64

[Blake Winton (:bwinton) (:☕️)]

(In reply to bastien.rene from comment #63)
> Is there any problem to put the older theme in Gmail conversations?

Not for this release, but I suspect someone (Florian, perhaps?) could release a theme add-on which was more in the previous style.

Thanks,
Blake.

Comment 65

[Benedikt Pfeifer]

(In reply to bastien.rene from comment #63)
> I didn't read all the comments, but I think there is still a problem. I read
> that the new theme was designed for IRC, but i have that theme in Gmail
> conversations, and IMHO the older one was better for Gmail ;)
> 
> Is there any problem to put the older theme in Gmail conversations?

Maybe it also helps to know that Instantbird (the program that Thunderbird got its instant-messaging core from) offers a wide variety of themes at http://addons.instantbird.org (in category "Message Styles") and might have something (maybe different but also suitable) for you. Once you picked the theme you like, we can guide you through making it compatible with Thunderbird. (Easiest thing to try different themes will be to use Instantbird for that, since it has a preferences page to manage both message style- and emoticon-themes).

Comment 66

[bastien.rene]

(In reply to Benedikt P. [:Mic] from comment #65)
> (In reply to bastien.rene from comment #63)
> > I didn't read all the comments, but I think there is still a problem. I read
> > that the new theme was designed for IRC, but i have that theme in Gmail
> > conversations, and IMHO the older one was better for Gmail ;)
> > 
> > Is there any problem to put the older theme in Gmail conversations?
> 
> Maybe it also helps to know that Instantbird (the program that Thunderbird
> got its instant-messaging core from) offers a wide variety of themes at
> http://addons.instantbird.org (in category "Message Styles") and might have
> something (maybe different but also suitable) for you. Once you picked the
> theme you like, we can guide you through making it compatible with
> Thunderbird. (Easiest thing to try different themes will be to use
> Instantbird for that, since it has a preferences page to manage both message
> style- and emoticon-themes).

Ok, i'll get one in the day, but i'll need help because i never worked on thunderbird add-on. Yesterday i took a look at the package .xpi of a theme and it seems to be easy to understand, but i need to know what to change to fit the new instant messaging core. 

Thanks for your help, maybe you can contact me by mail for that, it doesn't need to be on the bug tracker because it's not really related to that bug.