Closed Bug 779976 Opened 12 years ago Closed 12 years ago

Set priority with XMPP

Categories

(Thunderbird :: Instant Messaging, enhancement)

15 Branch
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: cyril.glapa, Assigned: cyril.glapa)

References

()

Details

Attachments

(1 file, 4 obsolete files)

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
Attached patch Patch (obsolete) — Splinter Review
Attachment #657323 - Flags: review?(florian)
Attached 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...)
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #657329 - Attachment is obsolete: true
Attachment #657329 - Flags: review?(florian)
Attachment #657348 - Flags: review?(florian)
Attached 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 :(
Attached 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
Closed: 12 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.