Set priority with XMPP

RESOLVED FIXED in Thunderbird 18.0

Status

enhancement
RESOLVED FIXED
7 years ago
7 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

7 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

7 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #657323 - Flags: review?(florian)
(Assignee)

Comment 2

7 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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

7 years ago
Posted patch Patch v3 (obsolete) — Splinter Review
Attachment #657329 - Attachment is obsolete: true
Attachment #657329 - Flags: review?(florian)
Attachment #657348 - Flags: review?(florian)
(Assignee)

Comment 5

7 years ago
Posted patch Patch v4 (obsolete) — Splinter Review
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 :(
Posted patch Patch v5Splinter Review
- 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+
Assignee: nobody → cyril.glapa
Status: NEW → ASSIGNED
https://hg.mozilla.org/comm-central/rev/8100c66adb6a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.