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

Save comment annotations in Drupal-like manner #12

Closed
tanius opened this issue Jan 8, 2015 · 4 comments
Closed

Save comment annotations in Drupal-like manner #12

tanius opened this issue Jan 8, 2015 · 4 comments
Assignees
Labels
Milestone

Comments

@tanius
Copy link
Member

tanius commented Jan 8, 2015

Pull request #2 brought us storing annotations relative to content, not content display. It relies on a nid column instead of pure XPath. That's much more Drupal-like, but does not allow annotations to comments any more. (Except when using a jQuery selector that also includes the comments below a post, storing comment annotations with the post's nid, but that's semantically wrong / not clean.)

The Drupal-like way to save annotations to comments (and potentially other Drupal entity types like users, webforms etc.) is to identify them with two columns: enity_type (with values "node", "comment" etc.) and entity_id. This is the usual Drupal technique to handle this, see for example:

SELECT entity_type, entity_id FROM field_data_body LIMIT 20;

(We don't want to include a revision_id field here, yet. Formally we would have to, but revision handing is a separate headache for the future.)

In addition, this needs code that determines the proper node ID or comment ID when creating an annotation. For performance reasons, we better avoid one instance per entity, instead setting up the jQuery selector to create one Annotator instance per page, for the node and its comments in common. This may require changes to the template. (Regarding performance: Firefox easily crashes with 20+ annotator instances, while Chromium / Chrome do not have issues with this.)

With one Annotator instance for a node and its comments together, it is clear that the XPaths for the annotation range delimiters cannot be determined like now (namely, not any more by making them start at the Annotator's jQuery result, which must coincide with a node's body field content to get character indexes right in RQDA exports). Instead, XPaths must be anchored at the container nodes for node body resp. comment body content, which are somewhere below Annotator's jQuery results. The modes of how to determine the node resp. comment IDs would ideally be configurable, if they differ between Drupal themes.

@tanius tanius added this to the 1.0 milestone Jan 8, 2015
@tanius tanius changed the title Allow annotating comments and other non-node entities. Save annotations on comments in Drupal-like manner Jan 8, 2015
@tanius tanius changed the title Save annotations on comments in Drupal-like manner Save comments annotations in Drupal-like manner Jan 13, 2015
@tanius tanius changed the title Save comments annotations in Drupal-like manner Save comment annotations in Drupal-like manner Jan 16, 2015
@tanius tanius self-assigned this Jan 25, 2015
@tanius
Copy link
Member Author

tanius commented Jan 25, 2015

While the database side of implementing this feature is clear, I am still working on a proper design for the Annotator side. The proposal from the issue description is:

[…] this needs code that determines the proper node ID or comment ID when creating an annotation. […] With one Annotator instance for a node and its comments together, it is clear that the XPaths for the annotation range delimiters cannot be determined like now […]. Instead, XPaths must be anchored at the container nodes for node body resp. comment body content, which are somewhere below Annotator's jQuery results.

It now seems to me that this proposal does not fit the Annotator architecture. Determining range start and stop positions is somewhere deep in the Annotator core code, always refers to the start of the Annotator instance's DOM element, and cannot be configured. The way how annotating multiple disparate elements on one page is meant to happen in Annotator is rather: one annotator instance per such element [source].

Each such Annotator instance would get metadata to use for storing its annotations: entity type, entity ID, field name, field value index. The way to pass such metadata is shown here. This multi-instance proposal also fits well with the currently favored proposal how to implement field-level annotations in the future.

Obviously the problem with this solution is that Firefox crashes when dealing with 20+ Annotator instances. Solution options for this are proposed and implemented in #46.

@tanius
Copy link
Member Author

tanius commented Jan 26, 2015

Just tried to modify determining an annotation's start and stop XPath expressions in Annotator, as proposed in the original issue report. It is definitely not the way to go. Because, it leads to multiple more issues:

  • XPath expression building needs to stop at relatively arbitary locations (all field content tags). So a callback (event subscription) mechanism would have to be introduced to not tie Annotator to the specifics of one Drupal template.
  • In addition to determining the XPath expression, the code would also have to determine annotation metadata: entity type, entity ID, and later (when implementing Save annotations relative to fields, not just entities #14) also field name and field delta. This needs more custom code via the callback, and excessive changes to Annotator since this part of the code (src/xpath.coffee) is not at all made to determine such metadata. Instead, metadata should be provided to the Annotator instance, as proposed above.

@tanius
Copy link
Member Author

tanius commented Jan 26, 2015

Simpler way to implement multiple Annotator instances

The problem is still how to provide multiple Annotator instances each with its own set of annotation metadata like entity type, entity ID etc.. The proposal above is to use different Annotator Store plugin configurations, like here. However, that's difficult, as one has to hook into each field rendering process, create a dynamically generated JavaScript file and fitting, dynamically generated calls to create Annotator instances.

A better and still simple and effective solution is to create multiple Annotator instances by extending the "Annotator element" jQuery field in /admin/config/content/annotator to be a list for multiple jQuery expressions (separated by some character or newlines). Or possibly, URL / jQuery expression combinations, but that's enough flexibility then. The trick is now to have a generic mechanism how an annotator instance can determine the field metadata at runtime – this means that nothing has to be configured for this, making this proposal less template dependent and more robust. For example, the uuid module could help with this.

Note that this proposal does not collide with solving #42 (though handing the annotations directly to the plugin also requires custom-generated JS).

tanius added a commit that referenced this issue Jan 26, 2015
…butes to #12

Note that you have to re-install the Drupal annotation module to get the software to work after this change. Like: drush dis annotation && drush pm-uninstall annotation && drush en annotation
@tanius tanius closed this as completed in 84eb7d6 Jan 28, 2015
@tanius
Copy link
Member Author

tanius commented Jan 28, 2015

Ok, so commit 84eb7d6 implements this "annotations for comments" feature. Metadata (entity type, entity ID etc.) is determined from a data-annotator-target attribute of the tag immediately surrounding field content. So I made it like proposed in my last comment. All in-place editing plugins for Drupal do it likewise, and I was inspired by their code :)

Note ( @danohuiginn ) that in order to use the software from now on, the "Annotator element" in the module configuration has to be set to something that selects all fields that you want to annotate. The following works for me (only annotating node / comment body fields to avoid too many Annotator instances bugging my Firefox down):

[data-annotator-target*="/body/"], [data-annotator-target*="/comment_body/"]

Also note that annotating body fields and other fields with HTML content requires fixing unfixed Drupal Core bug #1940986, as I describe over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant