Closed Bug 1523606 Opened 7 months ago Closed 7 months ago

conv-info-large attributes Typed and Typing can be converted into one

Categories

(Thunderbird :: Instant Messaging, enhancement)

Unspecified
All
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 67.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 4 obsolete files)

As mentioned in the comment https://bugzilla.mozilla.org/show_bug.cgi?id=1521610#c6 typed and typing are mutually exclusive. No need for both the attributes here: https://searchfox.org/comm-central/source/mail/components/im/content/imconversation.xml#1696-1697

Assignee: nobody → khushil324
Attached patch Bug_1523606.patch (obsolete) — Splinter Review
Attachment #9039802 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9039802 [details] [diff] [review]
Bug_1523606.patch

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

::: mail/components/im/content/imconversation.xml
@@ -1362,5 @@
>              cti.setAttribute("statusTooltiptext", typingMsg);
>              cti.setAttribute("statusMessage",
>                               this.bundle.GetStringFromName("chat.isTyping"));
>            } else if (typingState == Ci.prplIConvIM.TYPED) {
> -            cti.setAttribute("typed", "true");

Wouldn't we still need to set some sort of attribute here?

These are mutually exclusive, but do we do the same styling for them? Is anything else based on this attribute?

(In reply to Patrick Cloke [:clokep] from comment #2)

Wouldn't we still need to set some sort of attribute here?

These are mutually exclusive, but do we do the same styling for them? Is
anything else based on this attribute?

There was no styling related to these two attributes.

We can even remove both the attributes. But I think we should keep one attribute to have some idea about status change on typing feature.

OK. I'd be curious if Florian has any opinion on this.

Flags: needinfo?(florian)
Comment on attachment 9039802 [details] [diff] [review]
Bug_1523606.patch

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

Sure there is css for this:
https://searchfox.org/comm-central/source/mail/components/im/themes/chat.css#356-362 no?

What I was suggesting was that we either do the webby thing and use a class, like cti.classList.add("typing-now") + cti.classList.add("typing-done")
Or, continue to use an attribute, but not a *boolean* attribute. Like typing=now + typing=done
Attachment #9039802 - Flags: review?(mkmelin+mozilla) → review-
Attached patch Bug_1523606.patch (obsolete) — Splinter Review
Attachment #9039802 - Attachment is obsolete: true
Attachment #9040058 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9040058 [details] [diff] [review]
Bug_1523606.patch

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

typing=now and typing=done are confusing (to me at least).

The meaning of "typing" was "the remote person is typing or has been typing recently". The meaning of "typed" was "the remote person typed something, which hasn't been sent yet, and the person is not typing right now". I don't think "done" describes this correctly. Some other implementations use the word "paused" in that case. And you could use "active" instead of "now".
Attachment #9040058 - Flags: review-
Attached patch Bug_1523606.patch (obsolete) — Splinter Review

Also, updated the icon behavior as discussed on IRC.

Attachment #9040058 - Attachment is obsolete: true
Attachment #9040058 - Flags: review?(mkmelin+mozilla)
Attachment #9040084 - Flags: review?(mkmelin+mozilla)
Attachment #9040084 - Flags: review?(florian)
Comment on attachment 9040084 [details] [diff] [review]
Bug_1523606.patch

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

Looks good by code inspection, I haven't tested this locally.
Attachment #9040084 - Flags: review?(florian) → review+
Comment on attachment 9040084 [details] [diff] [review]
Bug_1523606.patch

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

::: mail/components/im/themes/chat.css
@@ +358,4 @@
>  }
>  
> +.statusTypeIcon[typing="paused"] {
> +  list-style-image: url('chrome://chat/skin/typed-16.png') !important;

you don't need the !importants, so please remove those
Attachment #9040084 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED

(In reply to Magnus Melin [:mkmelin] from comment #11)

you don't need the !importants, so please remove those

Without !importants, the status icon will stay and typing and typed icons will not be shown on typing. I talked to Florian on IRC, he agreed to have those icons on typing. Something like this: https://imgur.com/a/m1AGojX. So should I keep this or not?

(In reply to Khushil Mistry [:khushil324] from comment #12)

Without !importants, the status icon will stay and typing and typed icons will not be shown on typing. I talked to Florian on IRC, he agreed to have those icons on typing. Something like this: https://imgur.com/a/m1AGojX. So should I keep this or not?

You can also try to move the typing rules to line 449. Then they are the last in the row and they should override the normal status icon.

Well it worked correctly for me when I tried without !important.
We're talking about the little bubble in the corner of the image, right? When typing there is ... and then when paused there is an icon with __.

(In reply to Magnus Melin [:mkmelin] from comment #14)

Well it worked correctly for me when I tried without !important.
We're talking about the little bubble in the corner of the image, right? When typing there is ... and then when paused there is an icon with __.

Yes. It didn't work for me. I have shifted the CSS and it worked. So I guess its the best option without using !important.

Sure, go ahead.

Attached patch Bug_1523606.patch (obsolete) — Splinter Review
Attachment #9040084 - Attachment is obsolete: true
Attachment #9040362 - Flags: review+

Actually, please add a commit message to the patch. It should be like
"Bug 1523606 - convert conv-info-large boolean attributes typed and typing into typed=active or typed=paused, r=florian,mkmelin"

Then add checkin-needed to get someone to check this in for you

Flags: needinfo?(florian)
Keywords: checkin-needed
Attached patch Bug1523606.patchSplinter Review
Attachment #9040362 - Attachment is obsolete: true
Attachment #9040369 - Flags: review+
Keywords: checkin-needed
Blocks: 1521610

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/58f55abe8c75
convert conv-info-large boolean attributes typed and typing into typed=active or typed=paused. r=florian,mkmelin

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
You need to log in before you can comment on or make changes to this bug.