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

Paragraph-level commenting #1

Open
7 of 9 tasks
boonebgorges opened this issue Jul 1, 2015 · 29 comments
Open
7 of 9 tasks

Paragraph-level commenting #1

boonebgorges opened this issue Jul 1, 2015 · 29 comments
Assignees
Milestone

Comments

@boonebgorges
Copy link
Member

Ray, I know you've started work on this. Let's use this ticket for any technical discussion. cc @christianwach

Todos:

  • Save Inline Comments with a custom comment type (commits 794cd32, 26fc64e)
  • Remove Inline Comments from main comment loop on a Social Paper page (commit b6bc22a)
  • Inline comment permalinks (commit 74c4eff)
  • Comment AJAX support (commit c862518) (see "Notes" below)
  • Keeping track of comment position during document edits (more info here and here) (christianwach)

UX:

  • When toggling an inline comment, highlight the current paragraph or add a header to the inline comment block to let people know what paragraph the inline comment block belongs to. (commit @bad6382)
  • Inline comment timestamps (this might replace permalink icon)
  • Update paragraph number counter after a new inline comment is added. (submitted PR, fixed upstream in IC 2.1.3)
  • Add easier way to copy inline comment permalink (tooltip popup?) (may not implement)

Notes by Ray:

Comment AJAX support - WP Ajaxify Comments works well with Inline Comments and I have implemented support for this in commit c862518.

Christian notes the following however:

WP Ajaxify Comments... does not support the nextpage quicktag. See commentpress_comment_post_redirect() for more details.

I do not think this is a problem for us since WP Ajaxify Comments only supports one type of comment form on the same page. This means that AJAX only works for Inline Comments and not regular post comments, which is fine for our needs.

I don't think Inline Comments requires comment pagination, so we should be fine. (I could be wrong and haven't tested this yet.)

r-a-y added a commit that referenced this issue Jul 3, 2015
Proof of concept of letting the site admin choose the plugin they want to
use.

Could do more with CSS and javascript to integrate things better into our
full-width template.

See #1.
@r-a-y
Copy link
Member

r-a-y commented Jul 6, 2015

So in our spec doc on the Commons, I've outlined a few plugins that are currently available.

I think for our needs, we will let those plugins handle paragraph commenting as much as possible, Our Social Paper plugin will simply tweak things to better suit our full-width template. This will also allow site admins to choose which paragraph plugin they prefer.

I've already added support for WP Side Comments in a previous commit and just recently added preliminary support for the Inline Comments plugin (this took less than 15 minutes!).

As mentioned in other discussions, WP Side Comments (and Inline Comments) fits what we want to do UI-wise, but does not do any logic to modify the paragraph commentmeta position when a post is updated. This means that comments tied to a specific paragraph can display at the wrong paragraph after a post is edited. I have not reached out to the plugin authors about this, though it might make sense to do so.

CommentPress is further along in this respect, but CP requires changing the current theme in order to function properly. This doesn't work for us, so I spent some time late last week trying to override CP templates to make things gel for our needs. I got this far (comment form doesn't show yet). To go all the way, we'd need to copy CP's JS over and adapt it to our needs. This will take time, so before I dived further, I thought I'd stop and assess things.

Here is a table breakdown comparing the three plugins:

WP Side Comments Inline Comments CommentPress
Saves comment with comment_type set to side-comment. Could be useful to grab just side comments instead of regular comments. No changes to wp_comments DB row Adds an extra, custom DB column - comment_signature to the wp_comments DB table. Used to tie a comment to the paragraph.
Saves paragraph number as commentmeta - side-comment-section Saves paragraph number as commentmeta - data_incom Paragraph ID not saved as commentmeta (see above). However, saves comment page number as commentmeta - _cp_comment_page. Probably used for comment permalinks when comment pagination is used.
In the regular comments block at the bottom of the page, side comments are hidden from regular post comments on the page In the regular comments block at the bottom of the page, side comments are shown alongside regular post comments on the page. Side comments and regular post comments are separated in an accordion-style sidebar
Comments are posted via AJAX AJAX'd comments requires the WP Ajaxify Comments plugin Comments are posted via AJAX
Supports threading Supports threading
Attempts to update paragraph comment position after post edits
Adds a "Reference" anchor to the paragraph but in the regular comments block, not the side comment block Supports comment permalinks
Supports highlight comments
Has small icon linking to author's site when inline comments are toggled on the page (test the demo). Requires premium version to remove. Requires switching theme
No demo. Test locally! Demo URL Demo URL

tl;dr - Should we make adjustments to support CommentPress? Or should we add support for updating the paragraph position for one of the other two plugins using a version of CP's paragraph diff code?

Supporting either WP Side Comments or Inline Comments means we'll need to also add in comment permalinks of some kind.

@christianwach
Copy link
Collaborator

Good morning - I'm finally back after chronic HD ailments.

Here are some notes on the comparisons above, in the light of testing the plugins locally:

Adds an extra, custom DB column - comment_signature to the wp_comments DB table. Used to tie a comment to the paragraph.

Yep, CommentPress predates the existence of commentmeta :-) There are advantages and disadvantages to this: it means the comment_signature data is always efficiently loaded, but on the other hand I've had support requests from academics where their WordPress install is locked down such that it won't create the column.

The "hash" that is saved is composed of the first letter of each unique word in the paragraph (or commentable block) plus an auto-incremented paragraph identifier for cases where the content of two blocks produces hashes that are considered identical (or similar enough, at a 90% confidence level) by PHP's similar_text() function. The hash is restricted to 250 chars to allow a few chars for the paragraph identifier. None of this is entirely foolproof. Consider the following content when the content is parsed by line for poetry (rather than by auto-detecting p, ol or ul tags for prose, or arbitrarily by detecting the custom commentblock quicktag):

This is a commentable block
This is another commentable block
There is a commentable block

Every unmodified hash would be Tiacb, though they end up being Tiacb-1, Tiacb-2 and Tiacb-3. This is fine until the content is modified to the following, which would still fool the parser:

This is a commentable block
There is a commentable block
This is another commentable block

I've never come across this in practice, but then perhaps people just haven't let me know.

Paragraph ID not saved as commentmeta (see above). However, saves comment page number as commentmeta - _cp_comment_page. Probably used for comment permalinks when comment pagination is used.

Not quite. Comment pagination is disabled for the site by CommentPress since it's far from clear what it means in a paragraph-level commenting context. The _cp_comment_page commentmeta identifies the "page" (as defined by the nextpage quicktag) inside a post. It modifies the comment permalink and ensures that it points to the correct "page" of a multipage post without parsing the content every time. Also modifies the comment permalink for BuddyPress activity items. Lastly, it also allows only comments for that "page" to be displayed.

Comments are posted via AJAX

I think this needs a bit more analysis and research. CommentPress implements wp_editor() for comments, allowing for media-rich, rich-text-formatted commentary. It uses the standard WordPress process of submitting via 'wp-comments-post.php', as does Inline Comments. WP Side Comments implements its own AJAX comment submission process, which bypasses some of the checks that exist in 'wp-comments-post.php', as well as bypassing the set_comment_cookies action. I'm not sure yet what the consequences of this might be.

Neither Inline Comments nor WP Side Comments accounts for possible browser caching issues on the returned data. WP Ajaxify Comments, however, does include the necessary cache-busting, but does not support the nextpage quicktag. See commentpress_comment_post_redirect() for more details.

Adds a "Reference" anchor to the paragraph but in the regular comments block, not the side comment block

It seems odd to me that comments are duplicated below the content and to the side of it. Clicking the "reference" link only scrolls to the appropriate selector and does not reveal the comments. Neither Inline Comments nor WP Side Comments generate "paragraph" (or whatever commentable blocks are called) permalinks.

I notice that WP Side Comments has been extensively modified by others and that those changes have not been pushed upstream. I would say that an audit of these forks is probably worthwhile.

Finally, it looks as though Inline Comments is no longer supported by the author. I presume that he means he only offers support via the premium plugin.

@christianwach
Copy link
Collaborator

CommentPress is further along in this respect, but CP requires changing the current theme in order to function properly. This doesn't work for us, so I spent some time late last week trying to override CP templates to make things gel for our needs.

What theme are you using, @r-a-y ?

@r-a-y
Copy link
Member

r-a-y commented Jul 7, 2015

Thanks for the feedback, Christian.

CommentPress predates the existence of commentmeta :-)

Whoa!

The "hash" that is saved is composed of the first letter of each unique word in the paragraph (or commentable block) plus an auto-incremented paragraph identifier for cases where the content of two blocks produces hashes that are considered identical (or similar enough, at a 90% confidence level) by PHP's similar_text() function.

Thanks for summarizing how the hash is calculated. Might come in handy if we decide to port over your hashing / similar_text() comparison to one of the other plugins.

Comment pagination is disabled for the site by CommentPress since it's far from clear what it means in a paragraph-level commenting context. The _cp_comment_page commentmeta identifies the "page" (as defined by the nextpage quicktag) inside a post. It modifies the comment permalink and ensures that it points to the correct "page" of a multipage post without parsing the content every time. Also modifies the comment permalink for BuddyPress activity items. Lastly, it also allows only comments for that "page" to be displayed.

Thanks for clarifying. Makes sense. I wonder if we'll need to support the <!--nextpage--> tag though. I'm guessing for academic papers, it would be important.

CommentPress implements wp_editor() for comments, allowing for media-rich, rich-text-formatted commentary.

I would actually prefer that we do not use WP Editor for comments as it clutters up the interface. I'm not sure if I saw an option in CP to disable this.

[CommentPress] uses the standard WordPress process of submitting via 'wp-comments-post.php', as does Inline Comments. WP Side Comments implements its own AJAX comment submission process, which bypasses some of the checks that exist in 'wp-comments-post.php', as well as bypassing the set_comment_cookies action. I'm not sure yet what the consequences of this might be.

Good point. I didn't get far into my analysis here.

What theme are you using, @r-a-y ?

I'm using twentytwelve. But the Social Paper plugin bundles a custom full-width template and loads it only on single social paper pages to ensure that it stays separate from the current theme.

I haven't pushed any of my CP support mods to this repo yet. I'll most likely push my mods to a separate branch so you can see what I'm doing. (Disclaimer: Parts of it isn't pretty 😄)

@boonebgorges
Copy link
Member Author

This conversation is extremely helpful. Thanks @christianwach and @r-a-y.

CommentPress's signature + similar_text() method sounds pretty neat. I'm looking through the CommentPress codebase and it looks like it's not easily abstracted as-is, but I bet we could bundle the same logic into a relatively standalone little library. Using commentmeta :)

I think that the canonical way of keeping track of comment position is going to have to be something along the lines of the CP approach. Because the WPSC and IC both use paragraph numbers in commentmeta, we'll need to have a shim for each. I'd imagine they'll work something like this: For each paper, store an array that associates paragraph numbers with paragraph signatures. Every time the paper is saved, generate an updated array, compare against the old array, and update all commentmeta as necessary. (The alternative would be to intercept commentmeta queries by these plugins, which would require no extra database overhead, but is bound to be trickier.)

In any case: It is cool to provide out-of-the-box support for various commenting plugins, but for the Commons, we need to pick one and go with it. My opinion, having looked at the three options described by Ray, is that Inline Comments is the best option. It seems like it's better maintained than wp-side-comments (see @christianwach's comment above), it has (IMHO) a much better out-of-the-box interface than wp-side-comments, and it's far more lightweight than CommentPress. The main shortcoming of inline-comments seems to be that it doesn't natively post via AJAX, but we can pretty easily shim that ourselves (and pass upstream, if the author wants it).

I see that there is some complication with using wp-comment-post.php plus AJAX - this is the browser-cache-busting stuff that Christian talks about. This is a pretty lousy situation. My initial thought is that wp-comment-post.php but one of many possible for WP's Comments API. It happens to be one that expects a POST request, which means that it does some browser redirects and other stuff like that. It's not well-suited at all for AJAX requests, and I don't think there's any reason we should feel obligated to use it. If there are hooks there that may be important, we can reproduce them in our own AJAX callback. But 'comment_post_redirect' is one thing that would rightly be left out of a custom client. wp-comment-post.php is 12 years old, and should not be used as a model for separation-of-concerns :) As for 'set_comment_cookies': I think that for our first rev, we're only allowing authenticated users to comment, and those comments will be immediately approved, so there's no real need for wp_set_comment_cookies() - though maybe we'll want it in the future.

How do others feel about going forward with Inline Comments as our primary UI for now?

@boonebgorges boonebgorges mentioned this issue Jul 9, 2015
@christianwach
Copy link
Collaborator

How do others feel about going forward with Inline Comments as our primary UI for now?

I agree with this. It looks the better of the lightweight plugins. My primary concern would be the developer's statement that he's no longer supporting it. My secondary concern (but one which could be fixed during development) is that Inline Comments creates duplicate comments - both below the content and to the side of it - which confuses the UI in my opinion. Lastly, does CUNY have any accessibility requirements for CAC? If so, then it opens a number of issues with a JS-only solution.

My preference (on DRY grounds and because Inline Comments already includes support for it) would be to combine it with WP Ajaxify Comments rather than duplicate the functionality of that plugin within Inline Comments. Though I have to say that the WP Ajaxify Comments code is, um, not poetry. I'll test the combination today and comment further when I've done so.

@r-a-y
Copy link
Member

r-a-y commented Jul 10, 2015

How do others feel about going forward with Inline Comments as our primary UI for now?

I'm good with it.

My primary concern would be the developer's statement that he's no longer supporting it.

I believe this is only regarding free support on the wp.org forums.

My secondary concern (but one which could be fixed during development) is that Inline Comments creates duplicate comments - both below the content and to the side of it - which confuses the UI in my opinion.

I agree. One thing I like about WP Side Comments is it marks its comments with a unique 'comment_type'. Inline Comments does not. If IC did use a unique comment type when recording the comment, it might be possible to filter out the comments that way. Not sure if filtering out these comments will affect the side comment from showing up. At the very least, we could use CSS to hide the side comments from the regular comment block.

Lastly, does CUNY have and accessibility requirements for CAC? If so, then it opens a number of issues with a JS-only solution.

Right, I think we would need some form of comment permalink support at the very least.

I see that there is some complication with using wp-comment-post.php plus AJAX

The WP Ajaxify Comments plugin, which Inline Comments relies on, posts with wp-comment-post.php:
https://github.com/wp-plugins/wp-ajaxify-comments/blob/master/js/wp-ajaxify-comments.js#L349

But has to do some workarounds on the 'comment_post_redirect' filter:
https://github.com/wp-plugins/wp-ajaxify-comments/blob/master/wp-ajaxify-comments.php#L666

It also uses PHP sessions, but doesn't close the session:
https://github.com/wp-plugins/wp-ajaxify-comments/blob/master/wp-ajaxify-comments.php#L630

This could lead to trouble with other plugins in the future, however it's the most popular wp.org plugin supporting AJAX comments...

@christianwach
Copy link
Collaborator

I've just noticed that the WP Ajaxify Comments plugin devs charge $19 per support request. Which I suspect we can all understand the motivation for ;-) but nevertheless would appear to be a hindrance to developing with it.

@boonebgorges
Copy link
Member Author

I agree. One thing I like about WP Side Comments is it marks its comments with a unique 'comment_type'. Inline Comments does not. If IC did use a unique comment type when recording the comment, it might be possible to filter out the comments that way. Not sure if filtering out these comments will affect the side comment from showing up. At the very least, we could use CSS to hide the side comments from the regular comment block.

Oh, right, we should definitely address this. Alternative strategy: we could intercept the comment query and add a meta_query clause like this:

'key' => 'data_incom',
'compare' => 'NOT EXISTS',

Lastly, does CUNY have any accessibility requirements for CAC? If so, then it opens a number of issues with a JS-only solution.

These days, reliance on JS does not, alone, pose an issue for accessibility. See http://webaim.org/techniques/javascript/ for an overview. (Requiring no-JS support is important for other reasons, but I don't think we need to hamstring ourselves about it for the purposes of CAC.) That said, I agree that we need comment permalink support at least, even if it's JS-based (an anchor + opening the relevant comment thread).

Though I have to say that the WP Ajaxify Comments code is, um, not poetry.

I dealt with it a few years ago, and if it hasn't changed since then, then 👍 If WPAC looks pretty bad, or like it's going to take any work at all to make it compatible with Social Paper, let's just ditch it and build our own AJAX compatibility. It is not difficult to do when you control the theme, which we will, at least on CAC.

I've just noticed that the WP Ajaxify Comments plugin devs charge $19 per support request.

Heh. Yeah, I don't blame them. I expect we won't need technical support from any plugin we choose, so it doesn't matter for us, but it's good to consider this when we are thinking about the viability of a plugin for being the default Social Paper interface.

@christianwach
Copy link
Collaborator

These days, reliance on JS does not, alone, pose an issue for accessibility.

Indeed. I was enquiring about CUNY/CAC guidelines, if any. Thanks for clarifying.

we need comment permalink support

Agreed. And ideally "commentable block" permalinks, but that may be less important.

I expect we won't need technical support from any plugin we choose

I was thinking about whether we could upstream any improvements, rather than asking for help :-)

@christianwach
Copy link
Collaborator

Anyone want to share their Inline Comments settings?

Mine aren't working that well, but I haven't had time to refine them:

body.single:not(.buddypress) .entry-content p,
body.single:not(.buddypress) .entry-content ul,
body.single:not(.buddypress) .entry-content ol,
body.page:not(.buddypress) .entry-content p,
body.page:not(.buddypress) .entry-content ul,
body.page:not(.buddypress) .entry-content ol

And with the current setup of the theme, the ""Slide Site" Selector" needs to be:

.entry-content, .entry-footer, #comments

r-a-y added a commit that referenced this issue Aug 14, 2015
Currently, only allows for paragraphs.  Might potentially expand to other
selectors like <blockquote>.

See #1.
r-a-y added a commit that referenced this issue Aug 15, 2015
This will allow us to separate inline comments from the main comment
loop.

This commit also:
- Fetches inline comments with the new 'incom' comment type
- Alters the CSS comment class to bring back the 'comment' class so IC's
  comment toggling will still work.

See #1.
r-a-y added a commit that referenced this issue Aug 15, 2015
… a single Social Paper page.

See corresponding commit 794cd32.

See #1.
r-a-y added a commit that referenced this issue Aug 15, 2015
r-a-y added a commit that referenced this issue Aug 18, 2015
FEE duplicates the content for use with TinyMCE.  So to avoid IC's comment
bubbles rendering twice, add the ':visible' selector.

See related commit cbcabcf.

See #1.
r-a-y added a commit that referenced this issue Aug 18, 2015
- Alters the inline comment permalink to add the paragraph number.
- Adds some JS to watch for our inline comment permalink and triggers
  IC's click action to open the inline comment block.

See #1.
r-a-y added a commit that referenced this issue Aug 21, 2015
r-a-y added a commit that referenced this issue Aug 21, 2015
If the WP Ajaxify Comments (WPAC) plugin is activated, automatically load
IC's WPAC module, so WPAC uses IC's custom settings.

WPAC has its own settings routine, which bypasses WP's options API, so
what this commit does is manually merge in IC's WPAC settings into the
$wpac_options global before WPAC writes its inline JS.  Oh joy!

See #1.
r-a-y added a commit that referenced this issue Aug 25, 2015
We must also hook onto the 'default_option_X' filter in certain instances.

See #1.
r-a-y added a commit that referenced this issue Aug 25, 2015
- Display Permalinks - checked
- Display References - set to 'nowhere'

See #1.
@christianwach
Copy link
Collaborator

I've been looking at "Keeping track of comment position during document edits" and I now see why Google Docs doesn't offer paragraph-level (or other DOM element-level) commenting. It is possible to keep track of selection-based comments because the selection-start and selection-end changes can be detected as they happen if some kind of polling of their position is done or if temporary invisible markers are set when the insertion point is inside the selection range.

However I am having a hard time getting my head around how DOM element-based changes can be similarly tracked since the elements can change in arbitrary ways including generating new elements - for example by hitting 'return' to create new paragraphs, or 'delete' to join previously separate ones. I'll keep plugging away.

I'm also part way through a dependent task, which is to regenerate a functional Inline Comments environment when a post is updated. I've successfully intercepted the FEE save process and stripped out the Inline Comments data-incom attributes before the content is sent to the server (these attributes should not be saved in the post_content) but regenerating the Inline Comments bubbles and data-incom attributes is proving problematic without some major rewriting of 'inline-comments.js' to expose its methods to the outside world.

@christianwach
Copy link
Collaborator

Commit e0cc589 allows Inline Comments to remain functional after editing the content. It does not address the possibility of new relationships between the comments and the content. Nor does it address the possibility of orphaned comments when the number of selectors is reduced. However, Inline Comments itself doesn't check this either; i.e. if the corresponding element for the data-incom-comment value for a comment does not exist, the comment cannot be viewed. This can be replicated by commenting on the final paragraph of a post and then deleting that final paragraph. CommentPress handles this by making such comments "orphans" and assigning them to the "whole page", but without the means to reassign them to a paragraph, this is an incomplete solution for Social Paper. I'll keep digging...

@christianwach
Copy link
Collaborator

I've been tackling this issue, so I'll quote the relevant passage to refresh the discussion...

I think that the canonical way of keeping track of comment position is going to have to be something along the lines of the CP approach. Because the WPSC and IC both use paragraph numbers in commentmeta, we'll need to have a shim for each. I'd imagine they'll work something like this: For each paper, store an array that associates paragraph numbers with paragraph signatures. Every time the paper is saved, generate an updated array, compare against the old array, and update all commentmeta as necessary. (The alternative would be to intercept commentmeta queries by these plugins, which would require no extra database overhead, but is bound to be trickier.)

So far, my tests have led me to the opinion that this is not going to work unless similar_text() generates a match between sections of the old and new content. When content changes beyond the limit of the Levenshtein similarity test, I cannot see a way to discover which "block" a comment should now be associated with. I faced a similar dilemma with CommentPress and never found a way to resolve this issue to my satisfaction there either. My alternative plan with CommentPress was going to be that a comment should be associated with all the post revisions up to the point at which it became orphaned. I was then going to offer a means by which readers could step back through revisions to reveal the historical comments. In the meantime I implemented a UI to manually drag-and-drop "orphaned" comments to the element they were supposed to be associated with, which works quite well. This could perhaps be considered for Social Paper if all else fails.

So instead, I've been playing with a Javascript approach that attempts to track content changes so that I can trigger code when, for example, a new <p> node is created. I've added an extra trigger() to WP FEE"s tinymce.init() so that I can add code to the editor on setup() and I can now intercept TinyMCE's change and keyup events. But this is where things get pretty complex... i.e. figuring out when to re-parse the content without placing too much overhead on every key press. There are a number of ways in which new paragraphs can be generated:

  • hitting the ENTER key
  • from text that's pasted in
  • any others I've missed?

or deleted:

  • BACKSPACE or DELETE key
  • could also be done via a selection that spans two or more paragraphs followed by any other key (seems like the hardest to trap at the moment)
  • any others I've missed?

@boonebgorges @r-a-y I'd appreciate any thoughts you may have on this before I continue down this route.

@r-a-y
Copy link
Member

r-a-y commented Sep 18, 2015

So far, my tests have led me to the opinion that this is not going to work unless similar_text() generates a match between sections of the old and new content.

When I first started looking into the changing nature of paragraphs at the PHP level, the following is one of the libraries I found:
https://github.com/gorhill/PHP-FineDiff

I think this is better than using similar_text() and it does look more granular and controllable, but you would know more than me :) Check out the demo page here to test the various granularity levels:
http://www.raymondhill.net/finediff/viewdiff-ex.php

In the meantime I implemented a UI to manually drag-and-drop "orphaned" comments to the element they were supposed to be associated with, which works quite well. This could perhaps be considered for Social Paper if all else fails.

I think this works well if we can't figure out how to do this automatically.

So instead, I've been playing with a Javascript approach that attempts to track content changes

I've been observing what Medium does and it appears this is what they are doing as well. They make an AJAX POST containing the relevant changes and new hash markers for new paragraphs during specific key-related events. It also appears that they are referencing the revision IDs as well.

Orphaned inline comments appear to be deleted, so it looks like Medium doesn't support this either. We could do what CommentPress does and just move orphaned comments to the page comments if we do not want to lose this content.

@christianwach
Copy link
Collaborator

When I first started looking into the changing nature of paragraphs at the PHP level, the following is one of the libraries I found:
https://github.com/gorhill/PHP-FineDiff

I think this is better than using similar_text() and it does look more granular and controllable, but you would know more than me :) Check out the demo page here to test the various granularity levels:
http://www.raymondhill.net/finediff/viewdiff-ex.php

Thanks for this, Ray. Whilst it looks like a fine way of displaying diffs, unfortunately (as far as I can tell) it doesn't help so much in analysing or measuring changes. At least Levenshtein provides a way to analyse and assess similarity, which in turn allows us to make a decision on the results. The real problem is that once PHP is involved (i.e. the updated post content has reached the server) the text could have changed so substantially that no amount of analysis will give us enough data to make a decision.

I'm going to keep pressing ahead with the Javascript approach since I'm now persuaded that the only feasible way of tracking which comments should remain where is by keeping a eye on the changes as they happen.

@christianwach
Copy link
Collaborator

Before I commit anything related to change-tracking, I should probably post the code that allows additional setup to be done. Beginning at line 373 of fee.js:

        tinymce.init( _.extend( wp.fee.tinymce, {
            setup: function( editor ) {
                contentEditor = editor;
                window.wpActiveEditor = editor.id;

                registerEditor( editor );

                // Remove spaces from empty paragraphs.
                editor.on( 'BeforeSetContent', function( event ) {
                    if ( event.content ) {
                        event.content = event.content.replace( /<p>(?:&nbsp;|\s)+<\/p>/gi, '<p><br></p>' );
                    }
                } );

                $(document).trigger( 'fee-editor-setup', [ editor ] ); // CMW: new trigger code here

            }
        } ) );

This'll be required unless someone has an alternative idea of how to add the extra code to the TinyMCE instance.

@r-a-y
Copy link
Member

r-a-y commented Sep 21, 2015

Try putting this in hooks-wp-fee.js:

    $(document).on( 'fee-editor-init', function( event ) {
        editor = tinyMCE.get( window.wpActiveEditor );
        console.log( editor );
    } );

Will this work, @christianwach?

@christianwach
Copy link
Collaborator

Thanks @r-a-y that's more elegant.

@christianwach
Copy link
Collaborator

FYI, I'm working on change tracking in branch 'delta'. So far, it's looking like complex project in its own right.

@christianwach
Copy link
Collaborator

There's a new-ish change tracking branch which I've been working on to preserve comment associations and prevent the orphaning of comments during the editing process when the paper has existing comments.

What seems to be working (both with and without a selection):

  • Enter key
  • Delete key
  • Any key that could overwrite content
  • Paste key combination
  • Cut key combination

When paragraphs are added (e.g. with the enter key), comments on subsequent paragraphs should remain associated with their paragraphs. The rule that I'm working with when paragraphs are removed is that their comments get attached to the previous paragraph in the editor.

What's not yet been done:

  • when one or more entire paragraphs have been cut - I would like to re-associate the comments to the clipboard data if it's pasted back in to the editor.

@boonebgorges @r-a-y Feedback greatly appreciated as it would be really nice to put this out to wider testing via cdev.

@boonebgorges
Copy link
Member Author

Hi @christianwach - Sorry it's taken a while, but I wanted to get a chance to give this the attention it deserves. I spent a bunch of time today going through the functionality and the code, and it is quite clever and really nicely done. I'm saddened by the fact that the TinyMCE implementation requires these weird workarounds (like the clipboard node handling, and the PastePostProcess dance that you're doing). But I'm impressed with how you've made it work :)

The ENTER functionality seems to work well.

One bit of weirdness with DELETE. When backing out a paragraph one character at a time, using BACKSPACE, the unorphaning seems to take one extra keystroke. That is, I back out all the text in a para, until I get the "Add a block" placeholder, then press Backspace one more time so that I'm on the previous line (call this press "Alpha"). I'd expect the reshuffle to happen at this point, but missing = SocialPaperChange.tracker.get_missing( current_items ); is an empty array at this point. It's only on the next keypress ("Beta") - deleting the last character of the previous paragraph - that the reshuffle happens. I assume this is because the para isn't marked as "missing" until "Alpha", so nothing can be done about it until "Beta"? Seems like a small thing, but I did mess up my document a few times in this way until I realized what was going on :)

I found the CUT/PASTE process to be a little finicky. Most of this is FEE's fault. If you highlight the text of a paragraph and cut it, the paragraph block is emptied, but it remains in the document. So, given your "change" logic, the comments don't come with. I don't know how serious a problem this will be in practice, but it seems like an upstream bug: all empty elements probably ought to disappear, not just n-1 empty elements.

It's sort of a side-question, but a significant problem in my testing is that once paragraph comments have been combined, there's no way to manually uncombine them. So if SocialPaperChanges decides that comments from paras A, B, and C will all be added to A, and that's not correct for some reason, there's no way for me to fix the problem manually. I remember that there was a discussion between you and @r-a-y about the granularity of manual comment resyncing - is it related to this idea?

Thanks for your work on this! It's really impressive.

@r-a-y
Copy link
Member

r-a-y commented Oct 23, 2015

One bit of weirdness with DELETE. When backing out a paragraph one character at a time, using BACKSPACE, the unorphaning seems to take one extra keystroke.

I noted this on Gitter. Here's a GIF displaying the problem:
GIF


When I delete the This is what I would do if I was writing a paper paragraph, the cursor lands on the Blah Blah and Blah. paragraph. Notice that if I delete the trailing "." period from the "Blah Blah and Blah." paragraph, the JS then recognizes the deleted P1 paragraph.

I've found that switching handle_DELETE() to the 'keyup' event instead of 'keydown' fixes this use-case:
https://gist.github.com/r-a-y/994c86d119e9c213e735

But I'm not sure if this will break other use-cases.

Like Boone said, the JS is really impressive! ⭐ 🌟 🌠


So if SocialPaperChanges decides that comments from paras A, B, and C will all be added to A, and that's not correct for some reason, there's no way for me to fix the problem manually.

You can drag the paperclip icon for a top-level inline comment to another paragraph to move that inline comment and its children. I just noticed that I removed the paperclip icon on cdev (groan!), so I'll have to reinstate that back over there!

Here's a GIF that describes this:
GIF 2

We just need to document this to end-users.

@christianwach
Copy link
Collaborator

@boonebgorges @r-a-y Thank you both for the thorough testing. I will respond in full on Monday.

@boonebgorges boonebgorges added this to the 1.0 milestone Oct 25, 2015
@christianwach
Copy link
Collaborator

One bit of weirdness with DELETE. When backing out a paragraph one character at a time, using BACKSPACE, the unorphaning seems to take one extra keystroke

It looks to me as though it's a TinyMCE issue since I can see the issue in Gecko but not in Webkit. Webkit behaves properly (i.e. it doesn't require the extra DELETE keystroke) but Gecko does not rejig the paragraphs when it is supposed to. Switching to keyup as @r-a-y suggests won't be a solution, because keyup doesn't fire when the DELETE key is held down to produce multiple deletes. I'm not sure what the solution is right now, but I will have a play.

As @r-a-y points out, the comment-reassignment system should allow comments (that have been merged into paragraphs that they shouldn't be attached to) to be reassigned to their correct paragraphs.

I found the CUT/PASTE process to be a little finicky.

Yes indeed. The goal would obviously be to make the comments stick with entire paragraphs that have been cut, but I can't quite see how to identify when that happens. As my comments in the code point out, the presence of a trailing <p data-incom="Pn">&nbsp;</p> in the clipboard content is not a reliable guide to this because it is also present in the following situation, i.e. cutting between [ and ]:

<p data-incom="P0">Fo[o</p>]
<p data-incom="P1">Bar</p>

The only solution that I can think of is that the code keeps track of the content after each key press in order to discover whether a complete paragraph has been cut. This seems really clunky to me, but may be the only way forward.

@christianwach
Copy link
Collaborator

On a related note, I notice that Medium has abandoned paragraph-level commenting. I wonder if their decision is related to the problems associated with paragraph-tracking (as opposed to selection-tracking, where key presses inside a selection are easy to track)?

@mkgold
Copy link
Member

mkgold commented Oct 26, 2015

Very interesting to see that Medium has gone with selection-based
commenting. . . .

On Mon, Oct 26, 2015 at 9:36 AM, Christian Wach [email protected]
wrote:

On a related note, I notice that Medium has abandoned paragraph-level
commenting. I wonder if their decision is related to the problems
associated with paragraph-tracking (as opposed to selection-tracking, where
key presses inside a selection are easy to track)?


Reply to this email directly or view it on GitHub
#1 (comment)
.

@sheesh
Copy link
Contributor

sheesh commented Oct 26, 2015

Interesting, indeed!

On Mon, Oct 26, 2015 at 11:37 AM, mkgold [email protected] wrote:

Very interesting to see that Medium has gone with selection-based
commenting. . . .

On Mon, Oct 26, 2015 at 9:36 AM, Christian Wach [email protected]
wrote:

On a related note, I notice that Medium has abandoned paragraph-level
commenting. I wonder if their decision is related to the problems
associated with paragraph-tracking (as opposed to selection-tracking,
where
key presses inside a selection are easy to track)?


Reply to this email directly or view it on GitHub
<
#1 (comment)

.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@christianwach
Copy link
Collaborator

Also interesting is that Genius (was RapGenius) are offering their annotation system to other sites. Comments become theirs, however.

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

No branches or pull requests

5 participants