Closed Bug 1003200 Opened 10 years ago Closed 10 years ago

Add throbber to chat tabs that are in the process of being joined

Categories

(Instantbird Graveyard :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently confusingly marked as 'left'.
Depends on: 1002621
Attached patch throbber.patch (obsolete) — Splinter Review
Straightforward, just with lots of moving parts.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attachment #8416426 - Flags: review?(florian)
Attachment #8416428 - Flags: review?(florian)
Comment on attachment 8416426 [details] [diff] [review]
throbber.patch

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

Seems reasonable. You may want clokep to have a quick look at the IRC changes. I would also like to see what the PNG images look like (maybe show a screenshot?) :-).

::: im/content/conv.xml
@@ +73,5 @@
>               aTopic == "update-conv-title" ||
>               aTopic == "update-buddy-status" ||
>               aTopic == "update-buddy-status" ||
>               aTopic == "update-conv-chatleft" ||
> +             aTopic == "update-conv-chatjoining" ||

I was wondering if it wouldn't make more sense to change the "chatleft" notification name so that it applies in both cases... but then there's potential for breaking add-ons, so I guess you made the right decision here. Don't forget to update the wiki page listing all the notifications we send :-).

::: im/themes/status.css
@@ +59,5 @@
>  }
>  
> +.protoIcon[status="joining"] {
> +  display: none;
> +}

Can you explain why this is needed?
Attachment #8416426 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Seems reasonable. You may want clokep to have a quick look at the IRC
> changes. I would also like to see what the PNG images look like (maybe show
> a screenshot?) :-).

The png images come from Firefox (which is why they have the filenames they do), so you already know what they look like ;) 
But more specifically, the throbber is coloured in the tabs (OS-dependent) and monochrome in the contact list.

> Don't forget to update the wiki page listing all the notifications we send
> :-).

Thanks for the reminder!
 
> ::: im/themes/status.css
> @@ +59,5 @@
> >  }
> >  
> > +.protoIcon[status="joining"] {
> > +  display: none;
> > +}
> 
> Can you explain why this is needed?

In the buddy list, the status icon overlays the protocol icon (e.g. the offline icon for left). The throbber on top of the protocol icon looks bad though.
Attachment #8416426 - Flags: review?(clokep)
Comment on attachment 8416426 [details] [diff] [review]
throbber.patch

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

It looks good overall, I have a couple of questions below and one nit. (And agree with Florian's question.) Thanks!

::: chat/modules/jsProtoHelper.jsm
@@ +584,5 @@
>      this._left = aLeft;
> +    if (this._left) {
> +      // Also unset joining flag when leaving a room, just in case
> +      // the room was never joined properly in the first place.
> +      this._joining = false;

delete this._joining;

::: chat/protocols/irc/irc.js
@@ +1488,5 @@
>                       "JOIN " + channel + (key ? " <key not logged>" : ""));
>      // Open conversation early for better responsiveness.
> +    let conv = this.getConversation(channel);
> +    conv.joining = true;
> +    return conv;

I wonder if it would make more sense to joining to true in the constructor, but I don't really see why one would be better than the other. So works for me.

::: chat/protocols/irc/ircBase.jsm
@@ -78,5 @@
>        if (aAccount.normalize(nick) == aAccount.normalize(aAccount._nickname)) {
>          msg = __(nick, true);
>          // If the user left, mark the conversation as no longer being active.
>          conversation.left = true;
> -        conversation.notifyObservers(conversation, "update-conv-chatleft");

Nice clean up, thanks!

::: chat/themes/jar.mn
@@ +13,5 @@
>  	skin/classic/chat/mobile-16.png
>  	skin/classic/chat/mobile.png
>  	skin/classic/chat/offline-16.png
>  	skin/classic/chat/offline.png
> +	skin/classic/chat/connecting.png

Isn't this called joining everywhere else?

::: im/themes/instantbird.css
@@ +97,5 @@
>    list-style-image: url("chrome://chat/skin/chat-16.png") !important;
>  }
> +.tabbrowser-tab[chat][status="joining"],
> +.alltabs-item[chat][status="joining"] > .menu-iconic-left > .menu-iconic-icon {
> +  list-style-image: url("chrome://instantbird/skin/tabbrowser/loading.png") !important;

Why loading and not joining? I also find it a little bit weird that the other ones are in chat/ and this is in instantbird/, but I guess it makes sense.
Attachment #8416426 - Flags: review+
> ::: chat/modules/jsProtoHelper.jsm
> @@ +584,5 @@
> >      this._left = aLeft;
> > +    if (this._left) {
> > +      // Also unset joining flag when leaving a room, just in case
> > +      // the room was never joined properly in the first place.
> > +      this._joining = false;
> 
> delete this._joining;

I disagree. A protocol could in principle override the value this._joining would have after deleting it. And as the comment explains, we want to set it to a particular value here. For the same reason, we don't use "delete this.left" anywhere.

> I wonder if it would make more sense to joining to true in the constructor,
> but I don't really see why one would be better than the other. So works for
> me.

Setting it to true in joinChat is better because it also works for rejoins.
 
> ::: chat/themes/jar.mn
> @@ +13,5 @@
> >  	skin/classic/chat/mobile-16.png
> >  	skin/classic/chat/mobile.png
> >  	skin/classic/chat/offline-16.png
> >  	skin/classic/chat/offline.png
> > +	skin/classic/chat/connecting.png
> 
> Isn't this called joining everywhere else?

The filenames are inherited from m-c along with the contents to avoid future confusion.

> Why loading and not joining? I also find it a little bit weird that the
> other ones are in chat/ and this is in instantbird/, but I guess it makes
> sense.

The status icons are all in /chat. I'm just following the existing conventions here.
Attachment #8416426 - Flags: review?(clokep)
(In reply to aleth [:aleth] from comment #6)
> > Why loading and not joining? I also find it a little bit weird that the
> > other ones are in chat/ and this is in instantbird/, but I guess it makes
> > sense.
> 
> The status icons are all in /chat. I'm just following the existing
> conventions here.

I'm confused, it looks to me like you're NOT following the convention by putting this into instantbird/ instead of chat/ chrome area.

You had already answered the rest of my questions, but please add a comment above the protoIcon display: none before committing.
(In reply to Patrick Cloke [:clokep] from comment #7)
> I'm confused, it looks to me like you're NOT following the convention by
> putting this into instantbird/ instead of chat/ chrome area.

I'm doing both, hence the confusion. The tab version is in instantbird/tabbrowser along with the other OS-dependent tab stuff. The blist version is in chat/.

Maybe that's not the right thing to do and I should just go with chat/. The blist throbber would then be coloured just like the tab one is (currently it's https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/tabbrowser/connecting.png). I'm willing to be convinced either way.
OK. That's fine w/ me. Let's see how it looks in the nightlies.
Attachment #8416428 - Flags: review?(florian) → review+
Attached patch throbber.patch 2Splinter Review
After Mic pointed out that the greyscale connecting icon is animated in the opposite direction to the coloured loading icon, I decided to only use the coloured ones and put them in chat/. This removes the potential for confusion from comment 7.

I've used the new chat/themes/<os> folder convention for this as these directories don't already exist.
Attachment #8416426 - Attachment is obsolete: true
Attachment #8416616 - Flags: review?(florian)
Comment on attachment 8416616 [details] [diff] [review]
throbber.patch 2

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

I have a feeling you need to modify mail/components/im/themes/imStatus.css too?

::: chat/themes/jar.mn
@@ +32,5 @@
>  	skin/classic/chat/icons/insecure.png		(icons/insecure.png)
> +#ifdef XP_UNIX
> +#ifdef XP_MACOSX
> +	skin/classic/chat/loading.png			(osx/loading.png)
> +	skin/classic/chat/loading@2x.png			(osx/loading@2x.png)

The spacing here seems wrong.
(In reply to Patrick Cloke [:clokep] from comment #11)
> Comment on attachment 8416616 [details] [diff] [review]
> throbber.patch 2
> 
> Review of attachment 8416616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a feeling you need to modify mail/components/im/themes/imStatus.css
> too?

It won't exist anymore after the dependency lands.

> ::: chat/themes/jar.mn
> @@ +32,5 @@
> >  	skin/classic/chat/icons/insecure.png		(icons/insecure.png)
> > +#ifdef XP_UNIX
> > +#ifdef XP_MACOSX
> > +	skin/classic/chat/loading.png			(osx/loading.png)
> > +	skin/classic/chat/loading@2x.png			(osx/loading@2x.png)
> 
> The spacing here seems wrong.

I think that's due to the fixed number of tabs (it's common in jar.mn files), but I can fix it.
Attachment #8416616 - Flags: review?(florian) → review+
https://hg.mozilla.org/comm-central/rev/396fbef06517
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Depends on: 1009284
Depends on: 1009504
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: