Add annotation tool & migrate to JS in Thyme Player #533
Conversation
|
Obsolete since I've copied the contents from the other branch over to here for transparency. |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
I'm by far not yet finished with the code review, here some more remarks as intermediate result. Please make sure you have rubocop locally installed, so that it can show you some code suggestions. Otherwise, it's tedious that I have to open my editor, see what rubocop says, and copy it in here as comment. Note it's only responsible for Ruby files, not .erb files, for those you can install an ERB linter (which is not working very well for me in VSCode, will have to experiment a bit more with that).
Locally, I've slightly changed the .rubocop.yml file, see it here. It just adds some more excludes, so that one can focus on relevant things.
There was a problem hiding this comment.
Right now, some parts a hard to review in this PR due to the conversion of coffeescript to JS (which I indeed asked you to do). However, this PR would be more manageable if just the annotation-tool-related coffeescript lines would be converted to JS. Note that I say "lines", not "files". For example thyme.js is pretty big right now and GitHub shows me that a lot of new lines have been added. It is difficult to distinguish between lines that are really new regarding the annotation tool or lines that are just "new" as they were converted by the online decaffeinater.
My proposal is as follows:
- Keep the converted JS files somewhere on your PC locally for a later PR (or just save the commit hash right now, so we can jump back to these file later on).
- ⚠Outsource your annotation-tool specific javascript code to new
.jsfiles, e.g.thyme_annotations.js. Restore the old.coffeescriptfiles likethyme.coffeescript. - In a future PR named "Decaffeinate project (Coffeescript to JavaScript)", we can then bundle the annoying "get rid of CoffeeScript" changes together and fix things that the decaffeinater does not perfect manually, but in many files at once. This "de-clutters" this PR which should only consider things related to the annotation tool and not some old stuff from other files.
I know this is a bit extra-work, but I think it's worth it.
Note I also fell prey to decaffeination, e.g. here, however there it's really just one file with 24 lines of code, not huge files.
…e native HTML player is used or not.
… ids from the HTML files can be different in different players.
…attributes in normal and feedback player.
Also fixed positioning of other elements on the bottom bar.
This is the new labeling: -1: inherit from lecture 0: disabled 1: enabled Beforehand it was: -1: disabled 0: inherit from lecture 1: enabled However, I think it makes more sense to have 0/1 for disabled/enabled and leave the -1 for the "inherit" use case. Also, the "share annotation with lecturer" feature is now enabled by default for all lectures.
There was a problem hiding this comment.
Finally, after a long ride, I approve this PR.
To be honest, +4,677 code additions and -1,116 code deletions have shown me why huge pull requests like this one are frowned upon. It's just not manageable. Even with a new feature, there's always a way to break things down into smaller pieces. And there's no need for one PR to equal one release, e.g. one strategy could have been to create smaller PRs and merge them into the annotation-tool branch one step at a time, then later have a big annotation-tool to dev PR, but you have already gone through all the code in smaller PRs.
Hindsight is easier than foresight, of course. But I hope we learn our lessons from this PR. It's a sign if even the GitHub UI takes years to load the Git diff in the Files changed tab.
Despite all this: the new annotation feature is great and I'm really thrilled to get it shipped soon. With such a beast of a PR, I wasn't able to review every single line of code. The bigger picture is fine with me, but I wouldn't stake my life on the annotation tool not including any bugs. For sure, there will be some, but I'm sure we can tackle them quickly. The new Ruby and JS linters also assure code quality and looking at the code, the new modularized thyme player is really a big step forward away from coffeescript, thanks a lot @Frodo161 for your efforts!
I will merge this PR soon into dev, the last stage before we hit production ;)
Splines
changed the title
Splines
changed the title
This branch adds the feature to add annotations to the video timeline. They can be used for giving feedback to the teacher, for getting help, for publishing something directly in the video comments or - simply - to make personal notes.
By default students can create annotations on the video timeline. Click on the button next to the speed control to open a dialogue.
Enter the text for your annotation (you can use Latex!), choose a color and select the category. You can publish the content directly as a comment and - if the teacher enabled this feature - make your annotation visible for the teacher. If the teacher entered an "emergency-link" (see below) and if you choose "content" as category and then "definition" the dialogue will give you a link for further help.
Markers will appear on the video timeline once you click on "Save". If you click on one of them their content will be shown in the interactive area on the right.
To see the annotations of students (which are marked as "visible for teacher") the teacher has to activate the emergency button on the lecture edit page (comments).
If it is (not) activated there is still the posibility to (de)activate it for each single video. By default the option "inherit from lecture" is selected.
Go again to the lecture edit page (preferences this time). There you can enter links (or select a lecture) which is shown to the students as "emergency-link", if they have problems with understanding a definition.
To see the feedback of the student, teachers now have the "feedback player" which can be accessed on the media edit page (where the video was uploaded). In this very simple player, the teacher sees all the annotations (that are marked as "visible for teacher") together with a heatmap. The teacher can (de)select the categories of annotations that are shown to him at the bottom of the video.
Mistake annotations are slightly bigger than other annotations and are not included in the calculation of the heatmap!