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

Introduce Commenting Feature and Simultaneous Coding Environment. #551

Open
wants to merge 31 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@parvmor
Copy link
Contributor

parvmor commented Sep 8, 2017

Sorry for being so late in sending the PR. Some of the key highlights of the changes made are:

Note: Please do merge this right now. I would post a script here to migrate existing users files to the new file structure. Or else if you want users to be migrated online as and when required I can make the required changes.

  1. Splits up the server code into multiple modules(like Folder, Comment, Collaboration etc) and removes several avoidable GHC warnings.
  2. The file structure of storing the user data is changed completely so as to accommodate multiple users to code together by referencing a single file and simultaneously allowing users to comment on each others code. (More on this later)
  3. The save as button is removed and instead of that copy button is introduced which does the same job as save as but also handles folders.
  4. Users will now not be able to type straight out into the editor. They would always be required to create a file whenever they want to code.
  5. Users will be required to add a user identifier with each project(can vary with projects) that would appear in the comments. (Question: What to do about existing files? Possible Solution: Online migration instead of offline)

The new file structure:

  • For each user we maintain a separate folder, namely commentables, which contains all the projects user can comment on but cannot edit. Note that user will not be able to make changes so in such a case I have a introduced a Test Code button which will generate a test environment.
  • One can copy the projects/folders present in commentables into the editable category of project. Such copying will only preserve source codes. One cannot copy in to commentables directory.
  • One can move the projects/folders present in commentables into commentables and not outside of that.
  • To make multiple users to share a common file I introduced the following structure:
    1. Store all of the projects in data/mode/projectContents independent of which user they belong to. The project would be stored with a filename which is a hash of data/mode/projects/userId/userPath. Here userId will be chosen randomly from one of the many users that can edit this file.
    2. To link the users to this project we store the path of that file at user's path and also the user's path at the common location. We can visualize this as a star graph with undirected edges.
    3. All the data of comments will be stored at this new common path and any new user who is added only needs to be associated with this location.

Simultaneous coding:

  • By default every user will always be in a collaborative coding mode. To collaborate share the url generated by Collaborate button and whoever accesses this link will be given complete access to edit.
  • Deleting self from a collaborated will not delete it for others. Only the link between the user and project will be deleted.
  • One can see current and active owners by clicking Current Owners button.

Commenting:

  • One can comment on a project by clicking on any line in the gutter of the editor.
  • Handling line comments becomes a bit complicated when the file is changed. After discussing with Michael, I started doing versioning whenever needed.
  • Previous versions will not be editable but can be accessed and tested out whenever required.
  • Current commenting system is not interactive in the sense that it does not gives real time updates. But since the backend is set up it should be easy to integrate that using sockets. I would do that in my free time.

Note: For funblocks I have created a new server as it had conflicting handlers due to different file structures.

If I have missed anything new I will comment it below. Suggesstions are welcome. If any problems occur with this PR I will be glad to fix them.

parvmor added some commits Jun 27, 2017

Rearranges the code to make it more readable. Fixes the warnings of G…
…HC i.e. remove unused libraries and overshadowing of variables

Rearranges the code to make it more readable. Fixes the warnings of GHC i.e. remove unused libraries and overshadowing of variables
Changes the comment handlers to support versioning of code. frontend …
…and backend are not compatible.(Comment and Folder modules may not compile. Yet to complete certain handlers)
@cdsmith

This comment has been minimized.

Copy link
Collaborator

cdsmith commented Sep 10, 2017

Parv, I started to take a look, but before I get much further, let me address a few high-level questions:

  1. The changes to data/X/projects sound fine, but we will need a migration scripts.
  2. Do you have a running demo server somewhere? It would help tremendously to see the UI changes and be able to play around with them. Otherwise, I can try to set up a second server myself.
@cdsmith
Copy link
Collaborator

cdsmith left a comment

Just a start to the review. I will need to look more at this later, but I don't think I'm able to understand it until I can see it running. So let's prioritize getting a test server running (whether it's yours or mine)

build.sh Outdated
@@ -43,9 +43,11 @@ run codeworld-api cabal haddock --hoogle

# Build codeworld-server from this project.

run . cabal_install ./codeworld-server \
run . cabal_install ./third_party/ot.hs \

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

What does this even do? You're passing an individual Haskell source file to cabal_install?

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

Wait which file? That is a directory. It contains the source code for operational transformations.

@@ -24,20 +24,30 @@ Executable codeworld-server
cryptonite,
data-default,
directory,
engine-io,

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

In the multi-player gaming API, we decided to separate the game server from the web server, because it was less critical and more susceptible to denial-of-service or other failures. I believe that the same thing applies here, as well. Relaying real-time messages to other clients is dangerous, and if collaborative editing fails for a bit, it's better than the web site being down.

Instead of adding yet another server, can you rename codeworld-game-server, and add this functionality there, instead?

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

Sure would make the required changes!

filepath,
filesystem-trees,
funblocks-server,

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

It's not clear to me why this is a separate library. What's up here?

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

The file structure for funblocks and codeworld are different. So, the handlers for handling files in both have different logic. I thought it would be best to have a separate library for the same. Any other suggestions over how to accommodate this?


module Collaboration (module Collaboration__) where

import Collaboration_ as Collaboration__ hiding (getFrequentParams)

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

Huh? Can you explain what you're doing here? Why not just put the code in Collaboration instead of aliasing a second (and hideously named) module?

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

This was used as a way to get around the fact that haskell requires you to explicitly export functions you want to export. This allows to hide the functions you do not want to export. But now I can see that looks kind of a hack and ugly. So, I will change that.

user <- getUser clientId
mode <- getBuildMode
case getType of
1 -> do

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

Please, no! At least, use the type system. This is Haskell. But better yet, decompose the problem in a way that's less arbitrary.

file = userProjectDir mode (userId user) </> finalDir </> projectFile projectId
case length path' of
0 -> return (user, mode, file)
_ -> case path' !! 0 of

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

Why not capture the first element in the top-level case?

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

Actually I don't why I didn't do that. Lazy evaluation should allow that. Will update that.


funblockRoutes :: ClientId -> [(B.ByteString, Snap ())]
funblockRoutes clientId =
[ ("floadProject", loadProjectHandler clientId)

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

This is extremely unfortunate. Why should all the blocks UI stuff be so separate?

I feel like if I merge this change, that will be the day that Blocks starts to bitrot. All of a sudden, it will share almost no code with the main site, and so its code will get further behind over time, until it's an unmaintainable mess. I am not ready to give up on it yet.

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

I agree with you on this but new file structure for collaboration would require this to happen. A solution would be to let even funblocks use that but then it will only create some unnecessary files. Any suggestions for this?

This comment has been minimized.

@cdsmith

cdsmith Sep 13, 2017

Collaborator

It seems like most of the file manipulation should be the same. The blocks UI should have no trouble supporting folders and such. The only change should be disabling collaborative editing. (In theory, that could eventually be implemented too; it would just require a different OT model and integration with Blockly!) My hope would be that they would mostly share code, with a few buttons being disabled. But I don't understand the functionality yet, so it's hard for me to propose a specific answer.

@@ -0,0 +1,4 @@
:set -fwarn-unused-binds -fwarn-unused-imports

This comment has been minimized.

@cdsmith

cdsmith Sep 10, 2017

Collaborator

I cannot review your changes to the ot library, because I have no way to tell what's yours, and what is part of the original. And, sorry, I can't do a complete code review for the third party ot library.

Is it possible to see a diff for your changes somehow? Have you submitted your changes upstream, so that CodeWorld isn't maintaining a fork of this library indefinitely?

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

Sorry for this. There were no changes made by me over here. I will remove this and add a dynamic installation of ot.hs.

This comment has been minimized.

@parvmor

parvmor Sep 11, 2017

Author Contributor

Wait as a matter of fact I did made changes to the cabal file. The dependencies specified were not being resolved.

Resolving dependencies...
cabal: Could not resolve dependencies:
trying: ot-0.2.1.0 (user goal)
next goal: ghc (dependency of ot-0.2.1.0)
rejecting: ghc-8.0.1/installed-8.0... (conflict: ghc =>
binary==0.8.3.0/installed-0.8..., ot => binary>=0.5.1.1 && <0.8)
Dependency tree exhaustively searched.

Resolving this still gave more dependency errors. So I removed the versions. The only change is the following:

25,32c25,32
<   Build-depends:       base,
<                        text,
<                        aeson,
<                        attoparsec,
<                        QuickCheck,
<                        binary,
<                        either,
<                        mtl,
---
>   Build-depends:       base >= 4 && < 5,
>                        text >= 1.0 && < 1.3,
>                        aeson >= 0.7 && < 0.11,
>                        attoparsec >= 0.10.1.1 && < 1,
>                        QuickCheck >= 2.7 && < 2.9,
>                        binary >= 0.5.1.1 && < 0.9,
>                        either >= 4.1.2 && < 5,
>                        mtl >= 2.1.3.1 && < 3,

Using --allow-newer should resolve this.

@parvmor

This comment has been minimized.

Copy link
Contributor Author

parvmor commented Sep 11, 2017

As I mentioned the migration script we cannot(or preferable not to) add an offline migration script because user identifiers are need to be associated which each file. So, one way would be ask a common user identifier once and for all or to prompt for a user identifier whenever a file that doesn't has a user identifier associated with it. Which one do you prefer?

Also, I do not have a demo server that you would be able to access. I agree you should test them before merging. As of right now I do not have access to any server on which I can host. Can you try to setup one?(I will see if I can arrange one if possible. Having exams next week)

@parvmor parvmor force-pushed the parvmor:master branch from e75446d to 03fd99f Sep 12, 2017

@cdsmith

This comment has been minimized.

Copy link
Collaborator

cdsmith commented Sep 13, 2017

Not being able to migrate users to the new directory structure is definitely a problem. This dooms the project to maintaining code for the legacy file structure, basically forever. If this is just about the user choosing a nickname to identify themselves in comments, then let's assign one ("anonymous"?) that's always used, and they can change it later if desired.

@cdsmith

This comment has been minimized.

Copy link
Collaborator

cdsmith commented Sep 13, 2017

I still need to set up a test server and try this out. Unfortunately, the last week has been very busy for me. I'll try to get to this ASAP.

@cdsmith cdsmith referenced this pull request Dec 18, 2017

Open

Avatars #573

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment