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

[Obsolete] Introduce Commenting Feature and Simultaneous Coding Environment. #551

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

Conversation

parvmor
Copy link
Contributor

@parvmor 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.

…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
…and backend are not compatible.(Comment and Folder modules may not compile. Yet to complete certain handlers)
@cdsmith
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.

Copy link
Collaborator

@cdsmith cdsmith left a comment

Choose a reason for hiding this comment

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

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 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure would make the required changes!

filepath,
filesystem-trees,
funblocks-server,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@parvmor parvmor Sep 11, 2017

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@parvmor parvmor Sep 11, 2017

Choose a reason for hiding this comment

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

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
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)

@cdsmith
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
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 mentioned this pull request Dec 18, 2017
@cdsmith cdsmith changed the title Introduce Commenting Feature and Simultaneous Coding Environment. [Obsolete] Introduce Commenting Feature and Simultaneous Coding Environment. Aug 23, 2020
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.

None yet

2 participants