Last Comment Bug 779976 - Set priority with XMPP
: Set priority with XMPP
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 15 Branch
: All All
: -- enhancement with 2 votes (vote)
: Thunderbird 18.0
Assigned To: Cyril Glapa
:
Mentors:
http://xmpp.org/rfcs/rfc3921.html#rfc...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-02 13:52 PDT by Cyril Glapa
Modified: 2012-09-11 03:12 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.58 KB, patch)
2012-08-31 09:28 PDT, Cyril Glapa
no flags Details | Diff | Review
Patch v2 (2.62 KB, patch)
2012-08-31 09:47 PDT, Cyril Glapa
no flags Details | Diff | Review
Patch v3 (1.98 KB, patch)
2012-08-31 10:30 PDT, Cyril Glapa
no flags Details | Diff | Review
Patch v4 (1.98 KB, patch)
2012-08-31 10:44 PDT, Cyril Glapa
florian: review+
clokep: review+
Details | Diff | Review
Patch v5 (3.27 KB, patch)
2012-09-04 11:10 PDT, Florian Quèze [:florian] [:flo]
clokep: review+
Details | Diff | Review

Description Cyril Glapa 2012-08-02 13:52:59 PDT
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.
Comment 1 Cyril Glapa 2012-08-31 09:28:22 PDT
Created attachment 657323 [details] [diff] [review]
Patch
Comment 2 Cyril Glapa 2012-08-31 09:47:10 PDT
Created attachment 657329 [details] [diff] [review]
Patch v2
Comment 3 Patrick Cloke [:clokep] 2012-08-31 10:04:20 PDT
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...)
Comment 4 Cyril Glapa 2012-08-31 10:30:39 PDT
Created attachment 657348 [details] [diff] [review]
Patch v3
Comment 5 Cyril Glapa 2012-08-31 10:44:42 PDT
Created attachment 657356 [details] [diff] [review]
Patch v4
Comment 6 Florian Quèze [:florian] [:flo] 2012-08-31 10:48:03 PDT
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 :).
Comment 7 Florian Quèze [:florian] [:flo] 2012-08-31 12:21:10 PDT
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.
Comment 8 Ludovic Hirlimann [:Usul] 2012-09-03 04:23:33 PDT
I don't see any test with this :(
Comment 9 Florian Quèze [:florian] [:flo] 2012-09-04 11:10:51 PDT
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).
Comment 10 Patrick Cloke [:clokep] 2012-09-04 11:34:34 PDT
Comment on attachment 658157 [details] [diff] [review]
Patch v5

Review over IRC.
Comment 11 aleth [:aleth] 2012-09-04 11:53:23 PDT
Landed for IB as http://hg.instantbird.org/instantbird/rev/d83296ace3cd - Thanks!
Comment 12 Florian Quèze [:florian] [:flo] 2012-09-11 03:12:16 PDT
https://hg.mozilla.org/comm-central/rev/8100c66adb6a

Note You need to log in before you can comment on or make changes to this bug.