Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Allow guests to collaborate on multiple remote buffers (Part 1) #262

Merged
merged 65 commits into from
Dec 11, 2017

Conversation

jasonrudolph
Copy link
Contributor

@jasonrudolph jasonrudolph commented Dec 4, 2017

Demo

demo

Description of the Change

This pull request implements the core parts of the functionality described in RFC-001:

If you continue to follow the host [after first joining the portal], any time they switch to a new buffer, a new editor for that remote buffer is automatically added to your workspace and focused. Existing editors for previous remote buffers are not automatically closed when this switch occurs.

When a host closes a buffer, it will be removed from all guest portals. ...

You can follow any other guest participating in the host's workspace in the exact same way. If they move between buffers, you will follow them.

Coming in future pull requests

The following features of RFC-001 will be addressed in future pull requests:

  • Enhance the fuzzy finder to allow guests to navigate to any buffer that the host currently has open in their workspace.
  • If a guest is working in a buffer when the host attempts to close the buffer, before removing the buffer from the portal, ask the host to confirm their intention to remove the buffer.

Alternate Designs

See RFC-001

Benefits

See RFC-001

Possible Drawbacks

See RFC-001

Verification Process

  • Multiple guests can join portal
  • Guest can edit host's active buffer
  • Guest can edit inactive buffer shared by host
  • Guest sees empty portal pane item when host closes all active editors
  • Host closing portal changes guest's remote editors to untitled, unsaved local editors
  • Guest leaving portal removes the portal's editors from guest's workspace

Applicable Issues

#231: Introduced RFC-001.
#211: Requests sharing entire project. This pull request doesn't allow you to automatically share an entire project/folder, but we anticipate that this will be a useful interim improvement for many people that are interested in sharing a whole project.

TODO


🍐'ed with @as-cii

Antonio Scandurra and others added 30 commits November 30, 2017 15:02
This is a temporary stopgap measure until Atom supports batched marker
update events.
As of 4b3379c, this functionality lives
in GuestPortalBinding.
Antonio Scandurra added 5 commits December 5, 2017 15:01
This will prevent "Workspace can only contain one instance of object"
errors if the leader switches to a remote editor that has been moved
into a pane that is inactive when `updateTether` gets called.
This will prevent "Workspace can only contain one instance of object"
errors if the leader switches to a remote editor that has been moved
into a pane that is inactive when `updateTether` gets called.
This prevents a bug that was previously causing followers to disconnect
their tether when the leader rapidly switched among tabs and destroyed
them before followers had the chance to download the remote
editors/buffers associated with them.

In particular, followers would receive two messages indicating that the
leader wanted to switch to a new tab and that they wanted to close the
previous one. When trying to download the editor associated with the new
tab, the guest would find out that such editor did not exist anymore
and, therefore, would skip that message and only process the second
message indicating that the old editor needed to be removed. In doing
so, it would remove the old editor from the workspace and call
`portal.activateEditorProxy` with the editor proxy associated with the
newly active pane item in the workspace. In turn, this would cause the
follower to disconnect their tether, even if the two peers did not
really want to start browsing tabs on their own.

With this commit we are explicitly preventing this problem from
happening by not relaying active editor changes to `portal` when such
changes happen as a result of an update to the workspace caused by the
leader while the tether is retracted.
@jasonrudolph
Copy link
Contributor Author

jasonrudolph commented Dec 6, 2017

Exploratory testing revealed a handful of issues for us to address:

  1. In the RFC, we noted that bottom-right corner will show the avatars for participants that are in a different pane item not associated with the portal. Presently, when a guest focuses a local pane item, their avatar disappears entirely from participants' portals. When the host focuses a non-shareable pane item (e.g., the "settings" tab), the host's avatar disappears from other participants' portals.

    UPDATE 2017-12-08: We plan to address this in a follow-up pull request.

  2. The empty portal pane item sometimes remains open for the guest even when the host resumes editing a portal file. To reproduce:

    1. Host shares portal
    2. Guest joins portal
    3. Host opens a.txt
    4. Guest sees a.txt
    5. Host opens b.txt
    6. Guest sees b.txt
    7. Guest focuses a.txt, which causes guest to stop following host
    8. Host closes a.txt and b.txt
    9. a.txt and b.txt close in guest's workspace, and guest sees empty portal pane item
    10. Host opens c.txt
    11. Guest still sees empty portal pane item

    At this point, the guest can click on the host's avatar to start following the host again, but I think we should look for ways to improve this experience.

    UPDATE 2017-12-08: Fixed in 0d96a82.

  3. When the host splits panes, it opens duplicate tabs for the guest. To reproduce:

    1. Host shares portal
    2. Guest joins portal
    3. Host opens a.txt
    4. Guest sees a.txt
    5. Host runs Pane: Split Right command, resulting in a.txt open in the left pane and the right pane
    6. Guest sees a new tab open with a.txt

    Following step 5, I don't think we should open a new tab on the guest.

    UPDATE 2017-12-08: @as-cii and I discussed this behavior. This behavior provides a way for the guest to get multiple editors for the same buffer, which may be beneficial in some workflows. We're satisfied with the behavior for now.

  4. I saw this error in the console when rapidly switching back and forth between following one participant and then following the other. I'm not yet sure of exact steps to reproduce it.

    Uncaught (in promise) Error: Adding a pane item with URI '' that has already been destroyed
        at Pane.addItem (/Applications/Atom.app/Contents/Resources/app/src/pane.js:608:19)
        at Pane.activateItem (/Applications/Atom.app/Contents/Resources/app/src/pane.js:574:18)
        at Workspace.<anonymous> (/Applications/Atom.app/Contents/Resources/app/src/workspace.js:1178:20)
        at Generator.next (<anonymous>)
        at step (/Applications/Atom.app/Contents/Resources/app/src/workspace.js:11:279)
    

    UPDATE 2017-12-08: The following steps reproduce this error:

    1. Host opens a.txt and b.txt

    2. Host shares portal

    3. Guest joins portal

    4. Host navigates to a.txt

    5. Guest now sees a.txt and b.txt, and a.txt is the active pane item

    6. Guest closes b.txt

    7. Host closes a.txt

    8. Guest now sees b.txt but it's untitled

      At this point, a.txt should have been removed from the portal, and the guest should see the empty portal pane item.

    UPDATE: Resolved in 2905e67.

  5. I saw this error in the console once, but I haven't yet been able to reproduce it:

    - Uncaught (in promise) TypeError: Cannot read property 'dispose' of undefined
        at lastEditorProxyChangePromise.lastEditorProxyChangePromise.then (~/github/teletype/lib/guest-portal-binding.js:74)
    

    UPDATE 2017-12-08: Fixed in recent commits.

@ungb
Copy link

ungb commented Dec 6, 2017

good finds! me and @rsese are going to spend some time testing this tmrw! @jasonrudolph Do you think we should wait until the issues above are address before doing testing on it or would still be good to get extra 👀 on it?

@jasonrudolph
Copy link
Contributor Author

Do you think we should wait until the issues above are address before doing testing on it or would still be good to get extra 👀 on it?

@ungb: I'd love to have you test it out after we address those issues. If you want to also test it out before we address those issues, that's totally welcome. 😄

@ungb
Copy link

ungb commented Dec 7, 2017

Thanks @jasonrudolph! I'll take a look today and let you know if I find anything. I'm going to reschedule the meeting with me and @rsese to next Tuesday. I'll follow up with you guys on Tuesday if the PR isn't ready I can push the meeting out.

Prior to this change, if I host split an editor and then closed one of
the two tabs for the editor, the remote editor on the host would have
its title changed to "untitled".

With this change, we keep track of how many remote editors we have for a
remote buffer. When we deremotify the last editor for the buffer, we
deremotify the buffer as well.
@jasonrudolph
Copy link
Contributor Author

@as-cii and I discussed the rollout strategy for these changes. Here's our current plan:

  • Once we complete the task list in the pull request body, we'll merge this pull request and publish a new version of the package with these enhancements (i.e., the first part of RFC-001). These enhancements represent the majority of RFC-001, and we think developers will appreciate getting access to these enhancements without having to wait for all of RFC-001 to be implemented.
  • After publishing a release with these changes, we'll pursue the remaining parts of RFC-001 in follow-up pull requests and one or more subsequent releases:
    • Show icon on avatars for participants that are not in the current editor proxy
    • Show avatars for participants that are in non-portal pane items
    • Allow guests to use fuzzy-finder to open any remote editor shared by the host
    • Given that the fuzzy-finder integration will likely require a new release of the fuzzy-finder package, and given that the fuzzy finder package is bundled with Atom and new versions ship with the Atom release cycle, we may want to consider providing a custom Teletype fuzzy-finder for remote editors in the interim. We could deprecate this custom fuzzy-finder after implementing it properly in the fuzzy-finder package. Food for thought.
    • Once the fuzzy-finder (or a custom Teletype fuzzy-finder) is in place, we'll consider removing the logic that causes guests to automatically re-follow the host when the portal goes from n to zero remote items. (If the guest is working in a local editor when the portal goes from n to zero remote items, and we automatically re-follow the host, it will yank the guest away from their current editor. We'd like to avoid that. 😬)

When the guest closes the last remote editor or the portal's empty pane
item, leave the portal.

Prior to this change, if the guest closes the last remote editor, the
guest would be in limbo: they're still in the portal, but they don't see
any remote editors. Their only remedy was to manually leave and re-join
the portal.

In an upcoming release, we hope to integrate the guest's fuzzy finder
with the portal. Once that functionality is in place:
- If the guest closes the last remote editor, the guest could use the
  fuzzy finder to open any of the available remote editors, thus
  preventing the limbo state described above
- Because of this, we could remove the behavior implemented in this
  commit (i.e., we would no longer have to leave the portal when the
  guest closes the last remote editor)
@jasonrudolph jasonrudolph merged commit 2c461ca into master Dec 11, 2017
@jasonrudolph jasonrudolph deleted the multi-buffer-sharing branch December 11, 2017 15:38
@nathansobo
Copy link
Contributor

Nice work @as-cii and @jasonrudolph. This is going to be a huge improvement.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants