[Zeppelin-3307] - Improved shared browsing/editing for the note #2848
Conversation
# Conflicts: # zeppelin-server/pom.xml # zeppelin-web/src/app/notebook/notebook.controller.js # zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js # zeppelin-web/src/components/websocket/websocket-event.factory.js # zeppelin-web/src/components/websocket/websocket-message.service.js
|
@prabhjyotsingh @r-kamath |
|
Zeppelin has websockets and two users can view a note simultaneously, but there is a bug.
Thus paired coding is painful. |
|
I think this PR might be not bad for a small number of users, but, I'm not sure users increase. What do you think of it? It looks like calling websockets everytime whenever all users make changes |
# Conflicts: # zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java # zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/socket/Message.java
|
@jongyoul, yes sending any change to the server is a big load. But this will happen only if the notebook is opened by several users. The current implementation is subject to errors in consequence of which users may lose some of the code. Even if the second user just open the tab in the browser. This is very annoying. |
|
@prabhjyotsingh @r-kamath Could you help take a look at it ? It seems a necessary feature. |
|
@Savalek I agree with unexpected ignorances feel bad. Can you add test cases both of backend and frontend? And please make sure the relationship with This is an idea but needs to think of how we improve this feature? If this mode is disabled, all writers can write in the same way? otherwise, prohibit it except owner? When |
| @@ -460,6 +468,20 @@ private void removeConnectionFromAllNote(NotebookSocket socket) { | |||
| } | |||
| } | |||
|
|
|||
| private void collaborativeStatusCheck(String noteId, List<NotebookSocket> socketList) { | |||
There was a problem hiding this comment.
To fit method name schema, checkCollaborateiveStatus looks more proper.
| @@ -460,6 +468,20 @@ private void removeConnectionFromAllNote(NotebookSocket socket) { | |||
| } | |||
| } | |||
|
|
|||
| private void collaborativeStatusCheck(String noteId, List<NotebookSocket> socketList) { | |||
| if (!collaborativeModeList.containsKey(noteId)) { | |||
| collaborativeModeList.put(noteId, false); | |||
There was a problem hiding this comment.
Basically, it looks enough to make the type of collaborativeModeList to Set<String>. It's exactly same usage for now.
| @@ -1284,6 +1306,42 @@ private void updateParagraph(NotebookSocket conn, HashSet<String> userAndRoles, | |||
| } | |||
| } | |||
|
|
|||
| private void patchParagraph(NotebookSocket conn, HashSet<String> userAndRoles, | |||
| Notebook notebook, Message fromMessage) throws IOException { | |||
| String paragraphId = (String) fromMessage.get("id"); | |||
There was a problem hiding this comment.
I, absolutely, know there're many cases like these kinds of usages but we need to fix all of them. It might occur ClassCaseException
|
|
||
| String noteId = getOpenNoteId(conn); | ||
| if (noteId == null) { | ||
| noteId = (String) fromMessage.get("noteId"); |
| Paragraph p = note.getParagraph(paragraphId); | ||
|
|
||
| DiffMatchPatch dmp = new DiffMatchPatch(); | ||
| String patchText = (String) fromMessage.get("patch"); |
| (LinkedList<DiffMatchPatch.Patch>) dmp.patchFromText(patchText); | ||
|
|
||
| String paragraphText = p.getText(); | ||
| paragraphText = (String) dmp.patchApply(patches, paragraphText)[0]; |
|
|
||
| // javascript add "," if change contains several patches | ||
| patchText = patchText.replace(",@@", "@@"); | ||
| LinkedList<DiffMatchPatch.Patch> patches = |
There was a problem hiding this comment.
dm.patchFromText() returns List<DiffMatchPathc.Path> type. Is there any specific reason to use LinkedList?
There was a problem hiding this comment.
patchApply(LinkedList<DiffMatchPatch.Patch> patches, String text)
First argunment requires LinkedList<DiffMatchPatch.Patch>
| } | ||
| setParagraphMode(session, dirtyText, editor.getCursorPosition()); | ||
| if ($scope.cursorPosition && $scope.cursorPosition && $scope.cursorPosition) { |
There was a problem hiding this comment.
I don't know well about front, but what is this for?
| @@ -1263,6 +1282,11 @@ function ParagraphCtrl($scope, $rootScope, $route, $window, $routeParams, $locat | |||
| $route.current.pathParams.noteId); | |||
| }; | |||
|
|
|||
| const patchParagraph = function(paragraph, patch) { | |||
| const id = paragraph.id; | |||
There was a problem hiding this comment.
It looks unnecessary as id is used only once. and this function is only used inside sendPatch. Isn't it ok to merge this function into that?
| if (newPosition && newPosition.row && newPosition.column) { | ||
| $scope.cursorPosition = $scope.editor.getCursorPosition(); | ||
| } else { | ||
| console.log('NOOOOOOO Save'); |
There was a problem hiding this comment.
Can you change the level of the log with a specific comment?
|
@jongyoul, I have corrected. |
|
@Savalek Thanks for this new feature. IMHO it will be nice to have an option to turn on/off the collaborative mode and a way to show the number of users collaborating. |
|
@r-kamath, what do you mean by turn off/on option? |
|
@jongyoul, I do not like it when |
Right, a property in zeppelin-site.xml to control this feature will be good to have option. Just in case if anyone wants to turn it off. |
| <button type="button" | ||
| class="btn btn-default btn-xs" | ||
| ng-show="collaborativeMode" | ||
| tooltip-placement="bottom" uib-tooltip="Users who watch this note: {{collaborativeModeUsers.join(', ')}}" |
|
@Savalek Thanks!! That’s what I want to do exactly. I also don’t like to use |
|
@jongyoul, I added tests. |
There was a problem hiding this comment.
Can you add a document to this feature as well?
ref: https://github.com/apache/zeppelin/blob/master/docs/setup/operation/configuration.md
conf/zeppelin-site.xml.template
Outdated
| <property> | ||
| <name>zeppelin.notebook.collaborative.mode.enable</name> | ||
| <value>true</value> | ||
| <description>Enable basic opportunity for collaborative editing. Does not change the logic of operation if the note is used by one person.</description> |
There was a problem hiding this comment.
MINOR: Ideal to have a long description in the documentation. "Enable collaborative mode feature" is should be sufficient here.
|
@Savalek Thanks for adding e2e test but can you add also functional test for your backend code? It would be better to make a mock for |
|
@r-kamath, @prabhjyotsingh, i fixed the description |
|
@jongyoul, I added tests for backend. |
|
@Savalek BTW, https://travis-ci.org/Savalek/zeppelin/builds/380644746 Can you check your Travis? Zeppelin has a lot of tricky tests but, in your case, it's a bit different. Do you know the reason? |
|
@jongyoul, I run two tests. "master" and "zeppelin-3307 merge master." Travis shows the same result. Errors in this tests identical.
|
|
Thanks. Checked. I’ve saw that all results were error except the first one. LGTM |
|
Will merge it if there's no further discussion |
# Conflicts: # zeppelin-web/src/app/notebook/notebook.css
|
Fixed conflicts. What about merge? |
|
This feature could replace "personalized mode". I will create PR to remove "personalized mode". Thank you @Savalek again. |
|
@jongyoul As I understand personal mode allows users run paragraphs and have different views and different results due user chosen values in dynamic forms. |
|
@mebelousov The main purpose of "personalized mode" is to keep the current user's view. But if the user refreshes the browser, it changes the newest one. Do you think it's enough? |
|
@jongyoul I see next usecase. 10 users open the note, put client ID in dynamic form, refresh note, get and process data result. In this case getting of default note view is OK. |
|
Git it. Thanks. I thought it might not be useful. but I might be wrong. Thank you. |
### What is this PR for? Now if the note is opened in several tabs (or several users are watching or editing it), then there may be problems. Loss of code entered by the user, reset the cursor position. This PR adds a basic opportunity for collaborative editing. For the organization of joint editing, the library diff-match-patch is used. PR does not change the logic of operation if the note is used by one person. Also, maybe this will solve the problem with [ZEPPELIN-3131](https://issues.apache.org/jira/browse/ZEPPELIN-3131). ### What type of PR is it? Improvement ### What is the Jira issue? [ZEPPELIN-3307](https://issues.apache.org/jira/browse/ZEPPELIN-3307) ### Screenshots (if appropriate)  ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: Savalek <def113@mail.ru> Closes apache#2848 from Savalek/ZEPPELIN-3307 and squashes the following commits: 2347eb5 [Savalek] Merge remote-tracking branch 'apache/master' into ZEPPELIN-3307 3688d4b [Savalek] [ZEPPELIN-3307] added description in documentation. add tests. 566f844 [Savalek] [ZEPPELIN-3307] add tests for collaborative mode 88a9640 [Savalek] [ZEPPELIN-3307] checkstyle fix 32beb3d [Savalek] [ZEPPELIN-3307] add collaborative mode enable/disable option to zeppelin-site.xml 47e7db4 [Savalek] [ZEPPELIN-3307] small refactoring 8fad55a [Savalek] [ZEPPELIN-3307] small refactoring b4c5b20 [Savalek] [ZEPPELIN-3307] add collaborative users list to tooltip. refactoring. b131246 [Savalek] [ZEPPELIN-3307] - refactoring d8e6cde [Savalek] [ZEPPELIN-3307] resolve merge conflicts fbaa809 [Savalek] Merge remote-tracking branch 'apache/master' into ZEPPELIN-3307 8651f76 [Savalek] delete debug a1700d5 [Savalek] codestyle fix d7f449c [Savalek] Merge branch 'master' into ZEPPELIN-3307 4463ff0 [Savalek] coop icon add f87ab50 [Savalek] coop_raw_1
What is this PR for?
Now if the note is opened in several tabs (or several users are watching or editing it), then there may be problems. Loss of code entered by the user, reset the cursor position.
This PR adds a basic opportunity for collaborative editing. For the organization of joint editing, the library diff-match-patch is used.
PR does not change the logic of operation if the note is used by one person.
Also, maybe this will solve the problem with ZEPPELIN-3131.
What type of PR is it?
Improvement
What is the Jira issue?
ZEPPELIN-3307
Screenshots (if appropriate)
Questions: