Closed
Bug 954988
Opened 11 years ago
Closed 11 years ago
Download the user's vCard before uploading an avatar
Categories
(Chat Core :: XMPP, defect)
Chat Core
XMPP
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: florian, Assigned: florian)
References
Details
(Whiteboard: [1.2-blocking])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1556 at 2012-06-27 16:37:00 UTC ***
http://xmpp.org/extensions/xep-0153.html#bizrules-vcard says:
"A client MUST NOT advertise an avatar image without first downloading the current vCard. Once it has done this, it MAY advertise an image. However, a client MUST advertise an image if it has just uploaded the vCard with a new avatar image. In this case, the client MAY choose not to redownload the vCard to verify its contents."
The code I added in bug 954816 (bio 1381) doesn't respect this.
Assignee | ||
Comment 1•11 years ago
|
||
*** Original post on bio 1556 at 2012-06-27 16:42:22 UTC ***
See also http://xmpp.org/extensions/xep-0054.html#sect-id229678 (3.1 Retrieving One's vCard) and http://xmpp.org/extensions/xep-0054.html#sect-id229776 (3.2 Updating One's vCard)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [1.2-blocking?]
Assignee | ||
Comment 2•11 years ago
|
||
*** Original post on bio 1556 as attmnt 1755 at 2012-07-25 18:00:00 UTC ***
I haven't tested this at all (the unindented debug code is meant to be removed as soon as I'll have verified that things work as expected).
Patrick, I'm requesting feedback now so that you know what to expect in your review queue tomorrow ;). (and of course so that you have a chance to tell me if I'm doing something stupid before I waste more time on it :))
Attachment #8353516 -
Flags: feedback?(clokep)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → florian
Comment 3•11 years ago
|
||
*** Original post on bio 1556 at 2012-07-25 18:06:50 UTC ***
Comment on attachment 8353516 [details] [diff] [review] (bio-attmnt 1755)
WIP
>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>@@ -584,20 +584,22 @@ const XMPPAccountPrototype = {
>+ this._sendVCard(false, true);
> }
> else if (aTopic == "user-display-name-changed")
>- this._sendVCard();
>+ this._forceUserDisplayNameUpdate = true;
>+ this._sendVCard(true);
>@@ -1118,35 +1138,86 @@ const XMPPAccountPrototype = {
> _sendVCard: function() {
Am I missing something or does this function not take any parameters?
Assignee | ||
Comment 4•11 years ago
|
||
*** Original post on bio 1556 at 2012-07-25 20:33:51 UTC ***
(In reply to comment #3)
> Comment on attachment 8353516 [details] [diff] [review] (bio-attmnt 1755) [details]
> >+ this._sendVCard(false, true);
> Am I missing something or does this function not take any parameters?
Until a few minutes before I attached this patch, the function was taking 2 parameters aForceDisplayNameUpdate and aForceUserIconUpdate.
I changed to using member variables (eg. this._forceUserDisplayNameUpdate) because keeping the values of these 2 parameters so that the code fetching the vCard from the server and fetching the icon from the disk could provide them when calling _sendVCard back was challenging.
I'll clean this up for the next attachment, thanks for pointing it out!
Assignee | ||
Comment 5•11 years ago
|
||
*** Original post on bio 1556 as attmnt 1758 at 2012-07-26 16:33:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353519 -
Flags: review?(clokep)
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 8353516 [details] [diff] [review]
WIP
*** Original change on bio 1556 attmnt 1755 at 2012-07-26 16:33:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353516 -
Attachment is obsolete: true
Attachment #8353516 -
Flags: feedback?(clokep)
Comment 7•11 years ago
|
||
Comment on attachment 8353519 [details] [diff] [review]
Patch
*** Original change on bio 1556 attmnt 1758 at 2012-07-26 16:40:17 UTC ***
>+ // Send the vCard only if it has really changed.
>+ if (this._userVCard.getXML() != existingVCard)
>+ this._connection.sendStanza(Stanza.iq("set", null, null, this._userVCard));
>+ else
>+ LOG("Not sending the vcard to the server because it already has the same");
Maybe "Not sending the vcard to the server because the server vcard is identical." Or something...it took me a few times to understand what this meant. :) r+ with this changed to be readable.
Attachment #8353519 -
Flags: review?(clokep) → review+
Assignee | ||
Comment 8•11 years ago
|
||
*** Original post on bio 1556 at 2012-07-26 17:36:21 UTC ***
https://hg.instantbird.org/instantbird/rev/4cdab92b4cc0
Status: NEW → RESOLVED
Closed: 11 years ago
OS: Other → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [1.2-blocking?] → [1.2-blocking]
Updated•11 years ago
|
Target Milestone: --- → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•