Set priority with XMPP

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
Instant Messaging
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Cyril Glapa, Assigned: Cyril Glapa)

Tracking

15 Branch
Thunderbird 18.0

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120724191344

Steps to reproduce:

I added a XMPP account.


Actual results:

I cannot set a priority for my resource.


Expected results:

I should be able to set a priority for my resource.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

5 years ago
Created attachment 657323 [details] [diff] [review]
Patch
Attachment #657323 - Flags: review?(florian)
(Assignee)

Comment 2

5 years ago
Created attachment 657329 [details] [diff] [review]
Patch v2
Attachment #657323 - Attachment is obsolete: true
Attachment #657323 - Flags: review?(florian)
Attachment #657329 - Flags: review?(florian)
Comment on attachment 657329 [details] [diff] [review]
Patch v2

>diff -r e11462508947 chat/protocols/facebook/facebook.js
>--- a/chat/protocols/facebook/facebook.js	Mon Aug 27 21:21:49 2012 -0400
>+++ b/chat/protocols/facebook/facebook.js	Fri Aug 31 18:46:38 2012 +0200
>@@ -19,6 +19,7 @@
> FacebookAccount.prototype = {
>   __proto__: XMPPAccountPrototype,
>   get canJoinChat() false,
>+  get priority() 0,
Can't this just inherit from the super class? This would allow someone to go into about:config and set the priority then. (Same for GTalk.)

>diff -r e11462508947 chat/protocols/xmpp/xmpp.js
>--- a/chat/protocols/xmpp/xmpp.js	Mon Aug 27 21:21:49 2012 -0400
>+++ b/chat/protocols/xmpp/xmpp.js	Fri Aug 31 18:46:38 2012 +0200
>@@ -29,6 +29,7 @@
>   options: {
>     resource: {get label() _("options.resource"),
>                get default() XMPPDefaultResource},
>+    priority: {get label() _("options.priority"), default: 0},
Was there thought to putting this here or was it just randomly chosen? (I'm assuming it was chosen to group it with "resource", which kind of seem related...)

Do we definitely not want this option for GTalk and Facebook exposed in the UI? (I think we don't for Facebook, but I could potentially be convinced for GTalk...)
(Assignee)

Comment 4

5 years ago
Created attachment 657348 [details] [diff] [review]
Patch v3
Attachment #657329 - Attachment is obsolete: true
Attachment #657329 - Flags: review?(florian)
Attachment #657348 - Flags: review?(florian)
(Assignee)

Comment 5

5 years ago
Created attachment 657356 [details] [diff] [review]
Patch v4
Attachment #657348 - Attachment is obsolete: true
Attachment #657348 - Flags: review?(florian)
Attachment #657356 - Flags: review?(florian)
Comment on attachment 657356 [details] [diff] [review]
Patch v4

Thanks Cyril!

This looks good to me, but as we were pair programming on it, Patrick may want to double check :).
Attachment #657356 - Flags: review?(florian)
Attachment #657356 - Flags: review?(clokep)
Attachment #657356 - Flags: review+
Attachment #657356 - Flags: review?(clokep) → review+
Comment on attachment 657356 [details] [diff] [review]
Patch v4

>diff -r e11462508947 chat/protocols/xmpp/xmpp.jsm
>--- a/chat/protocols/xmpp/xmpp.jsm	Mon Aug 27 21:21:49 2012 -0400
>+++ b/chat/protocols/xmpp/xmpp.jsm	Fri Aug 31 19:44:17 2012 +0200
>@@ -1207,6 +1207,9 @@
>     this.reportDisconnected();
>   },
> 
>+  get priority()
>+    this.prefs.prefHasUserValue("priority") ? this.getInt("priority") : 0,
>+
>   /* Set the user status on the server */
>   _sendPresence: function() {
>     delete this._shouldSendPresenceForIdlenessChange;
>@@ -1232,6 +1235,14 @@
>       let time = Math.floor(Date.now() / 1000) - this._idleSince;
>       children.push(Stanza.node("query", Stanza.NS.last, {seconds: time}));
>     }
>+    let priority = this.priority;

Now that we don't override it anymore from facebook.js and gtalk.js, I wonder if we should keep the priority getter, or just inline it.
I don't see any test with this :(
Created attachment 658157 [details] [diff] [review]
Patch v5

- inlined the priority getter
- replaced the large if/else if block with Math.max(-128, Math.min(127, this.getInt("priority"))) that we find more readable (this was discussed on IRC).
Attachment #657356 - Attachment is obsolete: true
Comment on attachment 658157 [details] [diff] [review]
Patch v5

Review over IRC.
Attachment #658157 - Flags: review+

Comment 11

5 years ago
Landed for IB as http://hg.instantbird.org/instantbird/rev/d83296ace3cd - Thanks!
Assignee: nobody → cyril.glapa
Status: NEW → ASSIGNED
https://hg.mozilla.org/comm-central/rev/8100c66adb6a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.