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

[IMP] introduce collaboration feature #193

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

EmilienD
Copy link
Contributor

  • Expanded rollup configuration and changed live and dev script to allow the use of node_module dependencies.

  • add venv to .gitignore

  • Build folder for development and adapt script

  • Use uuid v4 instead of Math.random() based id

  • Add socketio cdn link and "start collaboration" button to demo

  • Remove lines that started a rollback when adding an unremovable node

  • Adapt editor and server for collaboration through websocket

  • Refactor "current step" management

    The current step was stored at the end of the historySteps array.
    This created a gotcha where there was no visible difference between a "commited" step and the current step that is not yet really part of the history.
    The last "commited" step was is the penultimate position in the array, which is counterintuitive.
    The current step is now stored in a property of the editor, and is accessed through a function in case this change proves to be wrong, it would be easy to reverse.

  • Human readable history implementation. Magic numbers are a bad practice.

  • User-based undo/redo. The _historyStepsState uses the step id instead of index for the edge case of an undo/redo step changing order because of the server reordering.

  • Better serialization of mutations. Mutations were serialized using the final state of nodes, instead of using the state of the node at the time of the mutation.
    This commit cleans the nodes of their extra children, using the heuristic that if a node is already added or removed directly in a mutation, it should not be present as a child of an other node.
    When applying the mutations, we also reuse existing nodes instead of automatically deserializing.

@EmilienD EmilienD force-pushed the master-add-collaboration-edu branch 2 times, most recently from 226f205 to 63aaf94 Compare April 16, 2021 09:43
src/editor.js Outdated
@@ -1,5 +1,7 @@
'use strict';

import { v4 } from 'uuid';
Copy link
Contributor

Choose a reason for hiding this comment

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

On le renommerait pas uuid4 en l'important ? C'est confusing quand même de voir v4() je trouve.

src/editor.js Outdated
// if not in collaboration mode, no need to serialize / unserialize
serialize(node) {
return this._isCollaborativeActive ? nodeToObject(node) : node;
serialize(node, stripFromChildren) {
Copy link
Contributor

Choose a reason for hiding this comment

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

C'est pas clair pour moi dans quel cas on veut sérialiser avec stropFromChildren et sans ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probablement jamais, stripFromChildren c'est un set de node à retirer des enfants des nodes qu'on sérialize. Le nom est un peu pourrave, si t'as une meilleur idée qui sonne moins comme le slogan d'un bar louche de Pattaya, je suis preneur.

Copy link
Contributor

Choose a reason for hiding this comment

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

Je comprends pas, si c'est une option qu'on veut jamais activer ou désactiver, pourquoi c'est une option ? ^^'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Qu'est-ce que tu veux dire par "option" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Je parle du fait que la fonction elle a un paramètre en +. C'est un malentendu en fait car je n'avais pas saisi que c'était un Set. Vu que son nom commence par un verbe, je pensais que c'était une option genre stripFromChildren: true / false, pour stripper je ne sais quoi depuis children, tu vois ce que je veux dire ? Mais en fait c'est pas une option, d'où le fait qu'on se comprenait pas :p Du coup faudrait p-e trouver un autre nom. Je propose childrenToStrip ou bien excludedChildren ? Qu'est-ce que tu en penses ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je suis vraiment d'accord avec toi qu'il faut renommer. L'idée c'est que c'est un set de node qu'il faut retirer des enfants des nodes qu'on sérialise, je vois pas trop cette idée dans childrenToStrip ou excludedChildren. Peut-être nodesToStripFromChildren ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Je pensais que c'était les children qu'on strippait. nodesToStripFromChildren ça me semble bien du coup.

src/editor.js Outdated
// if (destnode && record.node.parentNode.oid === destnode.parentNode.oid) {
// // TODO: optimization: remove record from the history to reduce collaboration bandwidth
// continue;
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Je retirerais le code commenté, à moins que le commit soit toujours un wip ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah oui j'avais oublié ce truc, faudrait que je voie si quelqu'un a une idée de ce que c'est censé être.

src/editor.js Outdated
});
this.historySetCursor(this.historyGetCurrentStep());
}
//else if (localStep && newStep.id === localStep.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not, it's there to document the explicit lack of action for that scenario. At first I left the actual code with the // do nothin' comment, but I felt it was a little confusing to actually test and branch on a condition where we do nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alors mettre un commentaire au dessus pour expliquer pourquoi on ne gère pas ce cas là. Histoire qu'on comprenne bien que c'est pas une ligne oubliée quoi parce que c'est à ça que ça ressemble pour l'instant... ^^'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est expliqué ligne 548.

Copy link
Contributor

Choose a reason for hiding this comment

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

Je suggère de distribuer le commentaire qui se trouve au dessus de la fonction. Plutot que d'avoir:

commentaire cas 1
commentaire cas 2
commentaire cas 3
code cas 1
code cas 2
code cas 3

Avoir plutot:

commentaire cas 1
code cas 1
commentaire cas 2
code cas 2
commentaire cas 3
code cas 3

Je pense que c'est mieux que de devoir switcher de l'un à l'autre pour bien comprendre.

server.py Outdated Show resolved Hide resolved
@dmo-odoo
Copy link
Contributor

Faudra rebaser la branche par contre. L'idée c'était de la merger en l'état puis de faire une autre branche pour le serveur etc ou bien tu comptes re-pusher dessus ? Je pencherais plutot pour la première option vu que t'as rien repushé sur cette branche depuis la review alors que je sais que t'as bossé, mais je préfère qu'on soit bien d'accord.

Emilien Durieu (edu) added 4 commits May 7, 2021 15:28
Changed live and dev scripts to allow the use of node_module dependencies.
This is a essentially a kludge to prepare for the dev of the collaboration feature.
@EmilienD EmilienD force-pushed the master-add-collaboration-edu branch from 63aaf94 to cf94e4d Compare May 12, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants