Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collaborative Editing using WebRTC (only 2 peers supported) #3741

Closed
wants to merge 2 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Nov 30, 2017

Implements: #1930.

Opened in place of #1877 to move development inside the upstream repository.

collab editing

The PR is experimental. Lacks some changes but a starting point.
Please note this is incomplete and is currently being updated.

Detailed Description:

  1. We create a Cediting instance inside middleware.

  2. Middleware is used to listen to all the actions being dispatched so that we can send those to P2P data channel. Few states are filtered in middleware that we don't send.

  3. For peer colors, we use a different action which is always dispatched when you select a block shown on the basis of peerID which is unique to each peer.

  4. Locking also happens on the basis of borders. If border for collaboration is visible on one peer side it is locked.

The module used is: https://github.com/abhishekgahlot/gutenberg-rtc
How GRTC works: https://github.com/abhishekgahlot/gutenberg-rtc/blob/master/DESIGN.md

TODO tasks

Blockers

Improvements

@gziolo gziolo added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress [Type] Task Issues or PRs that have been broken down into an individual action to take labels Nov 30, 2017
@gziolo gziolo changed the title Collaborative Editing using WebRTC ( Only 2 peers support ) Collaborative Editing using WebRTC (only 2 peers supported) Nov 30, 2017
@gziolo gziolo force-pushed the try/coediting branch 5 times, most recently from a1f201f to db71cc9 Compare December 1, 2017 21:41
@codecov
Copy link

codecov bot commented Dec 1, 2017

Codecov Report

Merging #3741 into master will decrease coverage by 4.61%.
The diff coverage is 46.08%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3741      +/-   ##
=========================================
- Coverage   42.41%   37.8%   -4.62%     
=========================================
  Files         307     284      -23     
  Lines        7295    6849     -446     
  Branches     1348    1252      -96     
=========================================
- Hits         3094    2589     -505     
- Misses       3521    3586      +65     
+ Partials      680     674       -6
Impacted Files Coverage Δ
editor/reducer.js 90.37% <ø> (ø)
editor/actions.js 47.16% <ø> (ø)
editor/store.js 83.33% <ø> (ø)
editor/selectors.js 93.46% <ø> (ø)
blocks/editable/index.js 10.1% <0%> (-0.65%) ⬇️
editor/edit-post/sidebar/post-settings/index.js 0% <0%> (ø) ⬆️
editor/edit-post/sidebar/coediting-panel/index.js 0% <0%> (ø)
editor/edit-post/sidebar/collaboration/index.js 0% <0%> (ø)
element/index.js 100% <100%> (ø) ⬆️
components/higher-order/with-filters/index.js 100% <100%> (ø) ⬆️
... and 216 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9486f1...35df0a5. Read the comment docs.

@@ -0,0 +1,42 @@
/**
Copy link
Member Author

@gziolo gziolo Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file. Leftover from rebase.

);
}

return <OriginalComponent { ...originalProps } />;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would read better when we flip the condition and return this early.

}

// Generate a class name for the block's editable form
const generatedClassName = hasBlockSupport( blockType, 'className', true ) ?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth @youknowriad can we move it to EditBlock? I think it's internal thing which shouldn't be part of this and the original component.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved in #4009.

const { attributes, name, isValid, uid } = block;
const blockType = getBlockType( name );

// Determine whether the block has props to apply to the wrapper.
Copy link
Member Author

@gziolo gziolo Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth @youknowriad I copied it from the original component. Isn't wrapperProps something that should be part of the block object? If I follow properly the flow is as follows:

  • in a completely different place use registered block type to create block
  • her fetch block and its attributes
  • get block type again to generate wrapperProps based on block's attributes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't wrapperProps something that should be part of the block object? If I follow properly the flow is as follows:

I'd rather see this move to a block supports. Originally I'd intended for the block list to be unaware of alignment features, but I think it's such a base functionality that it's fine. Might be even better implemented as a hook.

@gziolo
Copy link
Member Author

gziolo commented Dec 8, 2017

Rebased with master and squashed commits to make future rebases less painful.

@gziolo
Copy link
Member Author

gziolo commented Mar 7, 2018

I will rebase it next week. It's still work in progress put aside because it's not planned as a core feature for 5.0.

@aduth
Copy link
Member

aduth commented Jul 31, 2018

Should we close this until it's given proper focus to be renewed? I can't imagine a rebase at this point will be easy / possible.

@gziolo
Copy link
Member Author

gziolo commented Jul 31, 2018

Yes, let’s close it 👍

@gziolo gziolo closed this Jul 31, 2018
@gziolo gziolo deleted the try/coediting branch August 30, 2018 11:44
@gziolo gziolo added the [Feature] Real-time Collaboration Phase 3 of the Gutenberg roadmap around real-time collaboration label Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Real-time Collaboration Phase 3 of the Gutenberg roadmap around real-time collaboration [Status] In Progress Tracking issues with work in progress [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants