-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 support ) #1877
Conversation
Nice! Let's tweak the boundaries of a locked block so they look and behave like other blocks. Like this: Let's make sure all floats and alignments work, with the tweaked markup. @youknowriad not urgent but if at some point you have time can you look at this PR? Thanks! |
blocks/editable/index.js
Outdated
@@ -387,6 +387,7 @@ export default class Editable extends Component { | |||
} | |||
|
|||
getContent() { | |||
return this.editor.getContent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad No it's being worked at, I added this because redux states had react elements which I was not able to serialize. This works fine and output string but we are working on this for proper structure.
editor/modes/visual-editor/block.js
Outdated
@@ -345,66 +348,74 @@ class VisualEditorBlock extends Component { | |||
/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */ | |||
return ( | |||
<div |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to avoid creating a new div here, and reuse the inner div?
editor/selectors.js
Outdated
export function getPeerData( state, uid ) { | ||
const { peerID, peerName, peerColor, blocksByUid } = state.collaborationMode; | ||
|
||
if ( peerColor && peerID && peerName && peerID !== window.grtcProps.peerID && blocksByUid === uid ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird to use global variables in a selector? What's the purpose of the window.grtcProps
? Can't we move this to the store maybe?
editor/state.js
Outdated
@@ -3,6 +3,7 @@ | |||
*/ | |||
import optimist from 'redux-optimist'; | |||
import { combineReducers, applyMiddleware, createStore } from 'redux'; | |||
import ReduxThunk from 'redux-thunk'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need redux thunk? We're using an alternative in Gutenberg https://github.com/aduth/refx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, where are we using thunks in this code?
This PR is not easy to review since I'm lacking a lot of information and knowledge using P2P collaboration. Some questions:
|
@youknowriad Thanks for the comments, I will check and fix them. Gutenberg RTC is not a generic library but wrapper written over simple-peer library. I didn't add that to Gutenberg because it does one thing which is maintain P2P data channel (encrypted) with the help of serverside key value store. |
@abhishekgahlot What about moving the library to a root folder in the gutenberg repository? we have several separate modules already, element, utils, components, date... |
@youknowriad Sure will do that too. Thanks for notifying. |
editor/state.js
Outdated
} ) ); | ||
|
||
const enhancers = [ applyMiddleware( refx( effects ) ) ]; | ||
const enhancers = [ applyMiddleware( refx( effects ) ), applyMiddleware( ReduxThunk, grtcMiddleware ) ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be merged into a single applyMiddleware
: applyMiddleware( refx( effects ), grtcMiddleware )
editor/state.js
Outdated
@@ -3,6 +3,7 @@ | |||
*/ | |||
import optimist from 'redux-optimist'; | |||
import { combineReducers, applyMiddleware, createStore } from 'redux'; | |||
import ReduxThunk from 'redux-thunk'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, where are we using thunks in this code?
editor/selectors.js
Outdated
'border-bottom': '2px solid ' + peerColor, | ||
}, | ||
}; | ||
grtcProps.lastPeerData = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly mutating the state, Fix this
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from 'i18n'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note with #2172, these need to be updated to prefix the dependencies with @wordpress/
. You will need to perform a rebase against the latest version of master and apply your changes:
git fetch origin
git rebase origin/master
@gziolo These are the few things that have been bottleneck till now.
|
editor/reducer.js
Outdated
grtc.on( 'peerData', onReceivedAction ); | ||
// Temporary change for testing, Not using alert. Need UI. | ||
grtc.on( 'peerConnected', function() { | ||
alert( 'Peer connected' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should display global notice instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, but it still needs some work. Moved to todo list.
editor/modes/visual-editor/block.js
Outdated
@@ -320,6 +327,11 @@ class VisualEditorBlock extends Component { | |||
// Generate a class name for the block's editable form | |||
let { className = getBlockDefaultClassname( block.name ) } = blockType; | |||
className = classnames( className, block.attributes.className ); | |||
// Don't show controls when collaboration is enabled. | |||
if ( peerShowStyle ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gziolo @abhishekgahlot wonder if the changes to the visual-block component could be abstracted in a HoC withCollabEditing
to try to clean this file cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice, I will explore this idea. Moved to todo list :)
@@ -83,6 +82,76 @@ | |||
transition: 0.2s outline; | |||
} | |||
|
|||
&.is-collaboration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should be more semantic: is-being-edited
perhaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
opacity: 1; | ||
} | ||
|
||
&.collab-red { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These classes should be is-red
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
opacity: 0.8; | ||
} | ||
|
||
&.collab-null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to support a null class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
editor/reducer.js
Outdated
@@ -16,6 +16,14 @@ import { getBlockTypes } from '@wordpress/blocks'; | |||
import { combineUndoableReducers } from './utils/undoable-reducer'; | |||
|
|||
/** | |||
* Collaboration Dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing spaces here it seems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
editor/reducer.js
Outdated
*/ | ||
import GRTC from '../grtc/app'; | ||
|
||
const isMobile = window.innerWidth < 782; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem a good place for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both constants were never used. I removed them.
editor/reducer.js
Outdated
} | ||
|
||
/** | ||
* @param {Object} action is redux action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use //
comments for these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
import { collaborationMode } from '../../actions'; | ||
|
||
function CollaborationPanel( { active = false, ...props } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasmussen had a nicer idea for placing this in an ellipsis menu. (It's ok for an initial pass, though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to the todo list.
import { collaborationMode } from '../../actions'; | ||
|
||
function CollaborationPanel( { active = false, ...props } ) { | ||
const changeMode = () => props.collaborationMode( ! active ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "enableCollaboration" or "toggleCollaborationMode"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to unify language we use for this feature. If we would go for collaborative editing
then I would go for toggleCollaborativeEditing
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
grtc/DESIGN.md
Outdated
@@ -0,0 +1,193 @@ | |||
# Design |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this "architecture" to avoid clashes with design docs on searches, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Should we keep grtc in its own package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's much smaller after encryption got removed. If we would keep it, where should we put it in the repository? Can it remain in the top level or rather should be moved to editor
top level folder?
grtc/DESIGN.md
Outdated
@@ -0,0 +1,193 @@ | |||
# Design |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @gziolo maybe some of these notes can be added as comments in the code, which is always better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the docs, they may require further iterations. Let's wait until we stabilize API.
lib/collaboration.php
Outdated
* | ||
* @param object $data as request data along with args. | ||
* @return bool else username if logged in. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions need the version since info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to todo list.
lib/collaboration.php
Outdated
* @param string base64 | ||
* @return bool if its valid | ||
*/ | ||
function base64_check( $string ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the extra sanity checks needed over the strict flag in base64_decode()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
grtc/crypto.js
Outdated
@@ -0,0 +1,32 @@ | |||
const forge = require( 'node-forge' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crypto packages tend to be extremely large for browser consumption. This one is approximately 266kb minified and gzipped. I'd sought similar in Automattic/wp-calypso#17356, though I'm unsure of a good alternative for RSA key generation as you need here. Do you know of other options we could evaluate? I think the size as-is is quite excessive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the support of encryption and trust the turn servers in the relay. It can be removed completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed completely with 4ebad12.
grtc/app.js
Outdated
@@ -0,0 +1,471 @@ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need 'use strict';
with Babel (it is added automatically)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to split this file up, perhaps into one-file-per-class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use strict
removed and classes moved to their own files.
grtc/app.js
Outdated
const self = this; | ||
return new Promise( ( resolve, reject ) => { | ||
const data = { peerID: self.peerID, peerName: self.peerName, type: 'initial', signal: self.signalID }; | ||
jQuery.post( self.url + '/remove', { [ self.grtcID ]: window.btoa( JSON.stringify( data ) ) }, ( resp ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jQuery is not defined as a dependency for this script (i.e. incidental that another plugin depends on jQuery, but cannot rely on this). In any case, we might want to consider if we can do without it (maybe fetch
, which is already promise based?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial iteration uses window.fetch
. We can update later id we decide on some different approach.
grtc/app.js
Outdated
clearSignal() { | ||
const self = this; | ||
return new Promise( ( resolve, reject ) => { | ||
const data = { peerID: self.peerID, peerName: self.peerName, type: 'initial', signal: self.signalID }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the following line could be made more readable by splitting into separate lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Done 👍
grtc/app.js
Outdated
* @return {promise} promise object | ||
*/ | ||
clearSignal() { | ||
const self = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arrow functions preserve this
reference, you don't need to create a separate variable for tracking this
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removed all self
occurrences.
grtc/app.js
Outdated
* signalID is peer signal used to traverse and connect P2P. | ||
*/ | ||
constructor( url, grtcID, peerID, peerName, signalID ) { | ||
const self = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With arrow functions, I think this pattern of assigning a self
variable should be largely avoidable and arguably discouraged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
@aduth @mtias I addressed almost all your comments. Those remaining are now on the todo list. I did quite a heavy refactoring, but there are a few things that can be further improved. In particular I would like to change the way we shaped Redux state. If I understood properly it can't properly handle more than 2 peers at the moment. Middleware is still a bit hard to read, so I plan to tackle this one, too. @abhishekgahlot what did you mean by "Optimization for XHR Polling". Can you elaborate on this one? |
@gziolo Right now the number of requests that goes to the server in order to check for new peers is done every 5 seconds this can become huge very soon. Maybe something can be done to optimize stop polling when both peers connect? |
Codecov Report
@@ Coverage Diff @@
## master #1877 +/- ##
=========================================
+ Coverage 31.64% 33% +1.35%
=========================================
Files 235 208 -27
Lines 6658 6039 -619
Branches 1196 1074 -122
=========================================
- Hits 2107 1993 -114
+ Misses 3824 3409 -415
+ Partials 727 637 -90
Continue to review full report at Codecov.
|
Closing in favor of #3741. |
Work continues in #3741!!!
Please note this is incomplete and is currently being updated.
Detailed Description:
We create a grtc constructor inside middleware.
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.
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.
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
@since
in PHP file ( Collaborative Editing using WebRTC ( Only 2 peers support ) #1877 (comment)).withCoediting
HOC to wrap the visual-block component (Collaborative Editing using WebRTC ( Only 2 peers support ) #1877 (comment)).Blockers
Improvements