Closed Bug 955048 Opened 10 years ago Closed 10 years ago

Tags are never really removed from contacts

Categories

(Chat Core :: General, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [1.2-blocking])

Attachments

(1 file)

*** Original post on bio 1618 at 2012-08-05 01:40:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Blocks: 954956
Attached patch PatchSplinter Review
*** Original post on bio 1618 as attmnt 1790 at 2012-08-05 01:40:00 UTC ***

2 issues with removing tags from contacts made testing my patch from bug 954956 (bio 1524) difficult:
- we keep lots of left-over rows in the contact_tag table, causing tags removed from an account_buddy to still be kept on the contact (after a restart!, when rereading the blist from blist.sqlite).
- if a contact is removed from a tag and the contact signs on during the fading animation, the contact stays displayed in the group in the UI.
Attachment #8353549 - Flags: review?(clokep)
Severity: normal → major
Whiteboard: [1.2-blocking]
Comment on attachment 8353549 [details] [diff] [review]
Patch

*** Original change on bio 1618 attmnt 1790 at 2012-08-05 04:32:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353549 - Flags: review?(clokep) → review+
*** Original post on bio 1618 at 2012-08-05 09:16:12 UTC ***

(In reply to comment #0)

> - we keep lots of left-over rows in the contact_tag table, causing tags removed
> from an account_buddy to still be kept on the contact (after a restart!, when
> rereading the blist from blist.sqlite).

To be clear, I'm not very comfortable with the imContacts part of the patch fixing this issue, and even less with taking it that late for 1.2.
I believe this new behavior will cause problems in some corner cases, for example, if a contact c1 has two buddies b1 and b2, b1 has the t1 tag on the server, b2 has t2 on the server, the user adds the tag t3 to c1 from the UI, then for some reason (the user doing the same change from another client?) b2 has t3 on the server. With this situation, if the user detaches b2 from c1 or removes b2 from his list (by deleting the associated account for example), c1 will no longer have t3.

I think we should take the patch anyway because:
- these corner cases causing unexpected behaviors are much less likely to happen than the user removing a tag and wondering why it's back after a restart (the current behavior).
- I don't have any better solution to offer. It's the least intrusive changes I could find that fixed the main issue here. I don't think there's any "real" solution without large refactoring of our code handling tags.
*** Original post on bio 1618 at 2012-08-05 14:58:50 UTC ***

Comment on attachment 8353549 [details] [diff] [review] (bio-attmnt 1790)
Patch

>diff --git a/chat/components/src/imContacts.js b/chat/components/src/imContacts.js

>+    // aNewTag is now inherited by the contact from an account buddy, so avoid
>+    // keeping direct tag <-> contact links in the contact_tag table.
>+    ContactsById[buddy.contact.id]._removeContactTagRow(aNewTag);
>+
>     buddy.observe(aAccountBuddy, "account-buddy-moved");
>     ContactsById[buddy.contact.id]._moved(aOldTag, aNewTag);

After a night of sleep, this duplication of ContactsById[buddy.contact.id] looks awful. I'm adding a let contact = ContactsById[buddy.contact.id] before the check-in.
*** Original post on bio 1618 at 2012-08-06 01:56:12 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/428d3629a9d2
Assignee: nobody → florian
Status: NEW → RESOLVED
Closed: 10 years ago
OS: Other → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.