annotation tools

[danielcebrian]

To add these components to your course in Studio:

Installing Modules:

1. Add "notes", "textannotation" and "videoannotation" to advanced_modules in Studio's Advanced Settings. Now on the unit page you can see the Advanced components.

2. In the same advanced settings. It is necessary to add the annotation backend url and the annotation consumer key.
   2.1.) Type a url with the annotation external storage under “annotation_storage_url”
   2.2.) Type a consumer key for the annotation external storage under “annotation_token_secret”

Using Modules:

1. When creating new unit, you can find Text and Video annotation modules under “Advanced” component
2. Make sure you have either Text or Video in one unit, but not both.
3. Annotations are only allowed on Live/Public version and not Studio.
4. The video can not be opened in the cms (only in the lms) with youtube video.

Will also need to specify an annotation_storage_url and token_secret in Advanced Settings. @danielcebrian @lduarte1991 can provide this information.

[cahrens]

Is there a date by which this cleanup PR will need to be reviewed/merged?

[cahrens]

@danielcebrian Is this PR ready to review? What is the timeline for all the cleanup requested in #2188? If there are PRs to be reviewed in the next 2 weeks I need to get something on the Studio backlog.

[lduarte1991]

@cahrens This PR is ready to review. We've included a mixture of PR fixes from #2188 as well as bug fixes for the tool that we've found during live testing. We have on our own productivity tools a copy of the PR comments and will continue to tackle them, but we are definitely ready for a new push.

[cahrens]

So you would like a review from me and @cpennington? Anyone else?

[cahrens]

And is there a deadline?

[lduarte1991]

When is your next scheduled merge?

[cahrens]

I know that we went back and forth before on whether or not Studio needs the annotator code. What made you decide that Studio does need it?

[lduarte1991]

It was on the original plans to include this feature and I was told to try to include it as much as possible. I've actually been running into an issue getting TinyMCE to work with studio (works fine on the LMS) whenever I try to create an annotation, so if you happen to know why that would be, I'd greatly appreciate it. Other than that, it seems like it should work as intended.

[cahrens]

See my note below about 2 versions of tinyMCE being declared. When you are testing your changes, please also try opening an HTML component on the unit page to make sure that our existing usage of tinyMCE has not broken.

[cahrens]

BTW, you might be interested in https://github.com/edx/edx-platform/pull/2588. We plan to review that PR in the next 2 weeks, and it will upgrade the version of TinyMCE used by Studio.

[lduarte1991]

I think that might solve our issue, but that's @danielcebrian territory. Daniel do you know if having the version from #2588 will be okay for us?

[danielcebrian]

we are currently using tinymce 4.0.5 and this version should work well, better than the current 3 version.

[lduarte1991]

Great! so @cahrens I guess the proper step we could remove all instances of our tinymce and point it to the version in #2588 whenever that goes out, right? Hopefully that means that our PR and their PR goes out at the same merge (or ours after theirs).

[lduarte1991]

@cahrens Will this latest version be good enough to be pushed this week? I've taken out references in the CMS about tinymce and use your old one (it throws an error for now) but I'm guessing once the other pull goes through it will be referenced correctly since we are using requirejs, am I right?

[cahrens]

@lduarte1991 We still need to review the PR (and I imagine @cpennington needs to as well), so it will not be included in this week's push.

I don't expect the new version of TinyMCE to magically fix your issue (I haven't had a chance yet to see what it is). But if the usage in CMS is not important, you can wait until after the other PR is merged to debug.

After the new version of TinyMCE is merged, do you plan to use that for LMS as well and remove your copy of TinyMCE?

[lduarte1991]

@cahrens that's unfortunate, but understandable. Totally agree on the TinyMCE issue, just letting you know that the reason we included our version was mainly because the one currently in requirejs is old and daniel gave the okay above, but of course we would need to test it. In that case I can probably add a jasmine test for our notes app as well.

In terms of using the new version of TinyMCE, if it works for the CMS there's no reason why it cannot be used in the LMS as well, so absolutely yes.

[lduarte1991]

@cahrens and @cpennington can this version be included in the merge next week or is there anything that would still need to be changed?

If it is merged can the PR remain open for the next push with more fixes or will it have to be closed and a new PR opened (of course we'd still reference #2188 like we did above).

[cahrens]

Is this the same license that @jtauber was aware of?

[lduarte1991]

Yes, all the Plugins for annotator have the same license (including the new one added this time around called diacritic-annotator.js) and that was conveyed in the email sent to him originally. Both of these were created by me.

[jtauber]

This is acceptable because of the "(at your option) any later version". We can't accept GPLv2 but we can GPLv3.

[cahrens]

A TinyMCE update. It turns out that merging the upgrade PR was not completely straightforward, and we will have to schedule the additional work into a future sprint. At this time, there is no estimate of when the work will be completed.

[lduarte1991]

So does that mean I can't leave it as is for now and just bookmark any problems that may still be in place after the update takes place?

[cahrens]

As long as you are satisfied with the current behavior and there is no negative impact for our TinyMCE usages. I'm hoping to get back to this PR on Monday.

[lduarte1991]

Sounds good. I removed all offensive items in regards to your TinyMCE usage so it should all be okay. Will @cpennington also have time to look over this PR and thus be part of the merge next week?

[cahrens]

You forgot to remove this tinymce reference.

[lduarte1991]

Will commit fix in 1 minute. EDIT: Should be fixed now.

[cahrens]

Were more changes added to this PR? It's impossible for me to tell what has changed since I last looked at the PR because you squashed the commits together.

Here are some suggestions for making the review process easier:

1. Once someone has begun reviewing a PR, do not squash commits so the reviewer can just look at subsequent commits. The commits can be squashed again after the review is complete.

2. If you are trying to get a set of functionality submitted as soon as possible, don't keep adding additional functionality to the same PR. Instead, you can create a branch based off the branch that is being used for the first PR. You can even open a pull request against that first branch to show just the differences relative to the first PR. This allows the first PR to merge earlier.

[danielcebrian]

@cahrens I have added new functionality to this PR (new image annotation). Sorry if this way was not the best. I can go back to the last commit before the new functionalities. Should I do it?

[lduarte1991]

@cahrens @sarina Not sure if either of you is able to help, but the latest commit has an error with iconmoon.eot and I'm assuming that has to do with the fact that I tried to load my own skin (not from the pipeline but from the sass = [] inside of textannotation_module.py) and it cannot find the font file. The issue I did this was because when I was loading the tinymce without that it would show me missing fonts.

I think that the .module_display.xmodule_TextAnnotationModule .annotator-widget * class overrides the items in skin.min.css.

Any idea how to deal with that?

EDIT: I should clarify that I don't think this was an issue in studio because your tinymce is in a floating box that appears outside the module's area.

[cahrens]

Hi Luis, I don't have a computer handy (at a conference), but I can give some pointers. We spent a lot of time figuring out how to force TinyMCE to use pipelined CSS and font files. This is necessary to get things to behave properly on the CDN on FireFox as well as Chrome. As part of this work, Nimisha disabled TinyMCE's default skin loading code. So I think you will need to check in your skin into the TinyMCE directory and follow the same pattern that we did (using the pipeline).

[lduarte1991]

Well, actually I am trying to use your skin and not the one we had chosen before and that's what's causing the issues.

[lduarte1991]

But like i said the .module_display.xmodule_TextAnnotationModule .annotator-widget * overrides it.

[singingwolfboy]

@lduarte1991 said in #3466 that he is turning this pull request into a series of smaller pull requests. Does that mean that this pull request can be closed?

[singingwolfboy]

@lduarte1991 @danielcebrian can this pull request be closed?

[lduarte1991]

Yes, I'm sorry. Didn't know the first question was directed at me instead of about me. Absolutely.