Ticket #4965 (closed enhancement: fixed)

Opened 6 years ago

Last modified 6 years ago

friends should be subscribed to on XMPP

Reported by: daf Owned by: gdesmott
Priority: high Milestone: Update.1
Component: presence-service Version:
Keywords: review? Cc: eben, marco, jg, kimquirk, gdesmott, robot101, morgs
Action Needed: Verified: no
Deployments affected: Blocked By:
Blocking:

Description (last modified by jg) (diff)

We should subscribe to the presence of our friends on XMPP. When we turn off the shared roster, we won't get presence for anyone unless we do this. This will at least let us see the presence of our friends until we have a long term solution.

Where is the list of friends stored? Does the presence service have any way of knowing when a friend has been added/removed? Is friendship bidirectional? I.e. if somebody friends you, do you automatically friend them?

Attachments

subscribe.diff (3.6 kB) - added by daf 6 years ago.
ps-sync-friends.patch (5.2 kB) - added by gdesmott 6 years ago.
sugar-sync-friends.patch (1.1 kB) - added by gdesmott 6 years ago.
ps-sync-friends-new.patch (6.0 kB) - added by gdesmott 6 years ago.
sugar-sync-friends-3.patch (1.3 kB) - added by gdesmott 6 years ago.
ps-sync-friends-3.patch (5.0 kB) - added by gdesmott 6 years ago.
ps-sync-friends-4.patch (5.0 kB) - added by gdesmott 6 years ago.

Change History

Changed 6 years ago by robot101

  • cc kimquirk added
  • keywords Update.1? added
  • priority changed from normal to high
  • type changed from defect to enhancement
  • milestone changed from Never Assigned to Update.1

I have found that "Make Friend" in the Sugar UI only changes a local file. Kim agrees that we should populate the roster on the server so that at least your friends and their activities will still work for everyone if we turn off the shared roster stuff (whatever other scalability is implemented).

I had a chat with Eben this morning about whether or not friendship should be bidirectional. Our gut feeling is that similarly to normal XMPP clients, and when we have a UI for it, approving a friend request should also reply with a friend request, and that incoming friend requests for people who are currently your friend should be silently accepted (although removals are unidirectional).

Currently because there is no UI for it, incoming subscription ("Make Friend") requests are automatically accepted by the presence service. We should make sure this checks that the JIDs are on our local server, and if so then this is no worse than the privacy implications the shared roster currently has.

Marking high priority for Update.1 because if we don't fix this, the current XO firmware will see a very lonely world if when we start changing the server.

Changed 6 years ago by robot101

  • cc gdesmott added

Changed 6 years ago by jg

  • description modified (diff)
  • milestone changed from Update.1 to Never Assigned

Please do not set milestones.

Adding a feature at this date for update.1 must have truly compelling reasoning.

I'd like to talk about this first.

Changed 6 years ago by robot101

  • cc robot101 added

Changed 6 years ago by robot101

Of relevance to this ticket, I'm working on documenting the server interactions we use, their problems, and our proposed replacement, on the wiki at http://wiki.laptop.org/go/XMPP_Extensions.

Changed 6 years ago by jg

Awaiting attempted patch to evaluate milestones....

Changed 6 years ago by daf

Changed 6 years ago by daf

Untested patch applied.

I think the correct approach is to have the shell service provide a D-Bus API for accessing the friends list, with signals for when friends are added and removed. However, I think this is too large a change for Update.1, so I've taken a simpler approach:

* when connected to the server, read the friends file and synchronise it to the server * do the same thing every 10 minutes thereafter

I'd like some feedback as to whether this approach sounds reasonable.

The patch has some limitations:

* it assumes that our friends are on the same server as us, which should hold for the time being * I don't think it handles being disconnected from the server properly

Another option might be to use inotify to monitor the friends file, but that might be more complicated.

Changed 6 years ago by gdesmott

Changed 6 years ago by gdesmott

Changed 6 years ago by gdesmott

These 2 patched are based on daf's intial work and add a SyncFriends method. I modified his first patches to make them actually work and changed the code to make D-Bus calls async.

Note that we can't apply them now as it automatically unsubscribes all not-friend contacts and we still don't have a way to find not subscribed buddies. http://wiki.laptop.org/go/XMPP_Extensions#Use_Cases should solve this problem.

Changed 6 years ago by robot101

Apologies as I was away last week, but this isn't really the approach I was hoping for. We should hard-code a group ("Friends"?) on the server and only do synchronisation of our friends to that group. As you point out, removing everyone else from the roster will break the shared roster functionality. This defeats the point of introducing this patch at this point. We can't rely on changing the server and the laptops at the same time, so we want the laptops to *add* the Friends to whatever the server gives them, not just do one or the other.

The aim of this change is to make sure that current laptops who rely on being subscribed to *everyone* can still function (ie still see their friends) with a server that's modified to produce some other groups such as "Nearby" (people on the same /24 or /96) and "Random" (30 other people) as described on http://lists.jabber.ru/pipermail/ejabberd/2007-November/003230.html, rather than "Everybody".

Changed 6 years ago by robot101

A further thought. We don't need to remove anybody at the moment anyway, all we're looking to achieve is making sure that all friends are subscribed to as well as anyone else the server chooses to add. Sugar already shows only people /it/ considers to be friends in the friends view, so we can reduce the size of our patch by just making it additive-only at the moment. We don't need to sync the list back or consider removals at all.

Changed 6 years ago by gdesmott

Changed 6 years ago by gdesmott

New patch attached implementing that: ps-sync-friends-new.patch

Note that we'll have to fix #5164 to make it working.

Changed 6 years ago by gdesmott

Changed 6 years ago by gdesmott

Changed 6 years ago by gdesmott

New version of patches: sugar-sync-friends-3.patch and ps-sync-friends-3.patch.

Parsing is not done by PS anymore. Instead we pass an array of friend's key to the SyncFriends D-Bus method.

Changed 6 years ago by gdesmott

  • keywords Update.1?, review? added; Update.1? removed

Changed 6 years ago by gdesmott

Changed 6 years ago by gdesmott

new version of the PS patch: ps-sync-friends-4.patch

Changed 6 years ago by jg

  • keywords Update.1?, removed
  • milestone changed from Never Assigned to Update.1

OK to review and then put in joyride; eventual destination is Update.1. It is too late to consider for ship.2.

Changed 6 years ago by morgs

  • cc morgs added
  • owner changed from morgs to gdesmott

Changed 6 years ago by robot101

Uhh... we need this for Update.1. Marco, can you commit the sugar part of it and push it to Joyride, Daf, likewise for presence-service, then we can get it tested and have it tagged into Update.1.

Jim: Am I allowed to set blocker priorities, or should you do that? :)

Changed 6 years ago by daf

There's a Presence Service build that has ps-sync-friends-4.patch applied:

http://koji.fedoraproject.org/koji/buildinfo?buildID=27609

I've committed sugar-sync-friends-3.patch to Sugar git; Marco will include it in the next release.

Changed 6 years ago by cjb

  • owner changed from gdesmott to marco

Marco will do the sugar side tomorrow.

Changed 6 years ago by marco

  • owner changed from marco to gdesmott

Built sugar-0.75.3-1, it will be in the next joyride.

Changed 6 years ago by marco

Please test in joyride and request approval if it's working.

Changed 6 years ago by marco

  • status changed from new to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.