Revisions: revisions.js clean up

[ocean90]

Follow-up: #23497

Yes it works, but it can be prettier.

I'm starting with a clean up and koopersmith will step in later.

[ocean90]

I have created a ​branch on GitHub (Yes I did).

I attached a patch of the current state, but I still want to get rid of some things and waiting for some feedback from koop.

[adamsilverstein]

thank you for your effort at cleaning up the code, its definitely getting better!

i did a little testing and found a few problems, not sure of the source but i can try to dig a little deeper into the code to see whats happening.

here is what i saw:

• in single handle mode, the Restore button should be hidden when the handle is in the right most position - the current revision; it doesn't make sense to restore the current revision so the button is hidden. everything else in the single handle mode seems fine.

• in the two handled mode there is an issue with the left handle which seems to be stuck showing [Current Revision]; when dragging the left handle the 'To:' part changes when only the 'From:' part should change; it looks like the ajax data calls are off, i see right_handle_at=0 in both initial two handled calls, one should not have this variable at all, one should have right_handle_at=[wherever the handle is at]; something is broken with the url. if i discover more i will contribute to your branch on github

[ocean90]

23901.2.patch​ addresses the issues mentioned by adamsilverstein.

Moved wp_ajax_revisions_data() changes to #23920.

[adamsilverstein]

thanks, you beat me to it. will test.

[adamsilverstein]

corrects left handle reload issue

[adamsilverstein]

corrects not reloading data on left handle stop no logging

[ocean90]

#23920 was marked as a duplicate.

[adamsilverstein]

adds left_handle_add to rind handle data call, corrects data return

[adamsilverstein]

autonaming sequence update upload

[adamsilverstein]

latest patch adds the left_handle_at variable to the right handle data call in two handle mode. this limits data calculations to possible comparisons. please ignore my duplicate uploads.

[azaozz]

Looking at 23901.4.patch (not 23901.4.diff):

• There is a typo on line 2185 in the patched ajax-actions.php, if ( $show_autosaves && wp_is_post_autosave( $revision ) ) should be if ( ! $show_autosaves && wp_is_post_autosave( $revision ) ).
• Still seeing two &compare_to=... in the url, now sometimes two &post_id=... as well:

  /wp-admin/admin-ajax.php?
  action=revisions-data
  &compare_to=9077
  &post_id=9077
  &show_autosaves=true
  &show_split_view=true
  &right_handle_at=0
  &nonce=159dedc409
  &single_revision_id=9140
  &compare_to=9133
  &post_id=9077
  

[azaozz]

Another bug: when there is a notice "There is an autosave of this post that is more recent than..." the autosave is not listed in the revisions postbox. Clicking the "View the autosave" link doesn't let you restore that autosave. There is a "Restore This Revision" button for the [Current Revision] but not for the [Autosave].

[ocean90]

23901.5.patch fixes the typo. Looking into the ID mess now.

[markjaquith]

In 23898:

Further cleanup of revisions code. Probably not the last.

see #23901. props adamsilverstein, azaozz, ocean90.

[markjaquith]

Got the ID mess. It was overwriting the model's urlRoot with the collection's url() output. And then the model's url() function was tacking on redundant stuff.

[kovshenin]

Indeed, Diff.rightDiff === Diff.revisions.length is not the best way to toggle the "Restore" button visibility, since the last revision is not always the current one. 23901.5.diff​ adds a new is_current_revision property to the Revision model, and uses it to determine the "Restore" button display.

[adamsilverstein]

looks good, its helpful for me to see whats getting cleaned up, thanks!

[markjaquith]

In 23904:

A little more revisions js/php cleanup. see #23901. props kovshenin, ocean90.

[azaozz]

As we don't do theModel.urlRoot = model_collection.url; any more seems we don't need urlRoot at all.

[azaozz]

When there is a notice "There is an autosave of this post that is more recent...", clicking the "View the autosave" link shows the wrong diff: switched left/right parts, the content that is new in the autosave shows as deleted on the left instead of as added on the right.

Clicking on the Restore button restores the autosave properly.

[adamsilverstein]

Replying to azaozz:

When there is a notice "There is an autosave of this post that is more recent...", clicking the "View the autosave" link shows the wrong diff: switched left/right parts, the content that is new in the autosave shows as deleted on the left instead of as added on the right.

Clicking on the Restore button restores the autosave properly.

question:

is the id of the revision correct? that is, when you click the 'view the autosave' link, is the id in the link url the same as the stored autosave?

it almost seems like the comparison direction left/right is reversed? might it be an ordering issue with the list of revisions ( ala #23923 )? would be interesting to see if changing the ordering to ID fixes this.

[azaozz]

Replying to adamsilverstein:

is the id of the revision correct? that is, when you click the 'view the autosave' link, is the id in the link url the same as the stored autosave?

Yes, it's the correct post ID.

it almost seems like the comparison direction left/right is reversed? might it be an ordering issue with the list of revisions ( ala #23923 )? would be interesting to see if changing the ordering to ID fixes this.

No, changing the sorting doesn't seem to fix it. There are 6 revisions on the post, one is autosave:

ID	post_author	post_name
9298	4	9289-revision-v1
9297	1	9289-revision-v1
9296	4	9289-revision-v1
9295	1	9289-revision-v1
9294	1	9289-autosave-v1
9290	4	9289-revision-v1

Usually autosaves are not the latest revisions as they get updated while the user is editing. Revisions are never updated.

Also noticing we don't show the autosave(s) when listing the revisions in the Revisions postbox. In some cases there may be many autosaves (for different users) and no revisions. All autosaves should be listed there imho, why have them if we don't tell the user he can restore?

[adamsilverstein]

Replying to azaozz:

Replying to adamsilverstein:

is the id of the revision correct? that is, when you click the 'view the autosave' link, is the id in the link url the same as the stored autosave?

Yes, it's the correct post ID.

it almost seems like the comparison direction left/right is reversed? might it be an ordering issue with the list of revisions ( ala #23923 )? would be interesting to see if changing the ordering to ID fixes this.

No, changing the sorting doesn't seem to fix it. There are 6 revisions on the post, one is autosave:

ID	post_author	post_name
9298	4	9289-revision-v1
9297	1	9289-revision-v1
9296	4	9289-revision-v1
9295	1	9289-revision-v1
9294	1	9289-autosave-v1
9290	4	9289-revision-v1

Usually autosaves are not the latest revisions as they get updated while the user is editing. Revisions are never updated.

Also noticing we don't show the autosave(s) when listing the revisions in the Revisions postbox. In some cases there may be many autosaves (for different users) and no revisions. All autosaves should be listed there imho, why have them if we don't tell the user he can restore?

thanks for the details, i will take a look and fix the bug.

i agree the autosaves should be shown in the post box and will fix that as well

[adamsilverstein]

attaching a patch next that corrects the left/right comparison to use date not ID, this fixes the comparison issue you were seeing

the revisions list was removing the most current revision, but i think its better to show, so i removed that. i see other users autosaves in my revision list:

i did a little sort order testing:

i tried changing the sort order from date:

to ID - note user2's Autosave is now correctly shown as the 2nd revision:

[adamsilverstein]

use date not ID for left->right compare; show all revisions in post meta box including most current;

[adamsilverstein]

please test your data with 23901.7.diff​​, it should fix the issues you describe :)

Replying to azaozz:

Replying to adamsilverstein:

is the id of the revision correct? that is, when you click the 'view the autosave' link, is the id in the link url the same as the stored autosave?

Yes, it's the correct post ID.

it almost seems like the comparison direction left/right is reversed? might it be an ordering issue with the list of revisions ( ala #23923 )? would be interesting to see if changing the ordering to ID fixes this.

No, changing the sorting doesn't seem to fix it. There are 6 revisions on the post, one is autosave:

ID	post_author	post_name
9298	4	9289-revision-v1
9297	1	9289-revision-v1
9296	4	9289-revision-v1
9295	1	9289-revision-v1
9294	1	9289-autosave-v1
9290	4	9289-revision-v1

Usually autosaves are not the latest revisions as they get updated while the user is editing. Revisions are never updated.

Also noticing we don't show the autosave(s) when listing the revisions in the Revisions postbox. In some cases there may be many autosaves (for different users) and no revisions. All autosaves should be listed there imho, why have them if we don't tell the user he can restore?

[adamsilverstein]

corrected with post_date

[adamsilverstein]

Replying to adamsilverstein:

please test your data with 23901.7.diff​​, it should fix the issues you describe :)

Replying to azaozz:

Replying to adamsilverstein:

is the id of the revision correct? that is, when you click the 'view the autosave' link, is the id in the link url the same as the stored autosave?

Yes, it's the correct post ID.

it almost seems like the comparison direction left/right is reversed? might it be an ordering issue with the list of revisions ( ala #23923 )? would be interesting to see if changing the ordering to ID fixes this.

No, changing the sorting doesn't seem to fix it. There are 6 revisions on the post, one is autosave:

ID	post_author	post_name
9298	4	9289-revision-v1
9297	1	9289-revision-v1
9296	4	9289-revision-v1
9295	1	9289-revision-v1
9294	1	9289-autosave-v1
9290	4	9289-revision-v1

Usually autosaves are not the latest revisions as they get updated while the user is editing. Revisions are never updated.

Also noticing we don't show the autosave(s) when listing the revisions in the Revisions postbox. In some cases there may be many autosaves (for different users) and no revisions. All autosaves should be listed there imho, why have them if we don't tell the user he can restore?

actually, please use 23901.8.diff​​

[azaozz]

Replying to adamsilverstein:

Looking at 23901.8.diff: changing from ID to post_date in wp_ajax_revisions_data() fixes the above problem.

Not sure about the changes in wp_list_post_revisions(). Seems the $parent arg there is not needed now as the revisions always include a copy of the current post as a "real" revision. Checking the plugins repo, this function doesn't seem used anywhere except in core, so deprecating one arg won't bring any back-compat problems.

As this function is called after the revisions have been converted, it's not necessary to check the revisions version and prepend @post.

23901-9.patch​ deprecates the $parent arg in wp_list_post_revisions().

[adamsilverstein]

Replying to azaozz:

Replying to adamsilverstein:

Looking at 23901.8.diff: changing from ID to post_date in wp_ajax_revisions_data() fixes the above problem.

Not sure about the changes in wp_list_post_revisions(). Seems the $parent arg there is not needed now as the revisions always include a copy of the current post as a "real" revision. Checking the plugins repo, this function doesn't seem used anywhere except in core, so deprecating one arg won't bring any back-compat problems.

As this function is called after the revisions have been converted, it's not necessary to check the revisions version and prepend @post.

23901-9.patch​ deprecates the $parent arg in wp_list_post_revisions().

right! perfect.

[azaozz]

In 23975:

Revisions: compare revisions by date in wp_ajax_revisions_data(), deprecate the $parent arg in wp_list_post_revisions() as now revisions always include a copy of the current post, props adamsilverstein, see #23901

[ocean90]

23901.10.patch​ takes up 23901-7.patch​ and unifies the variable naming.

[adamsilverstein]

23901.10-b.diff​​ corrects one issues from 23901.10.diff, variable names for linesAdded, linesDeleted ​needed to be updated in ajax-actions.php; otherwise the ticks are all the same width.

also, removed code that was hiding last tick in two handled mode, this was a bit of remnant code from an earlier build. now in the two handled mode the rightmost tick shows correctly.

before:

after:

[ocean90]

In 24019:

Revisions: Clean up JavaScript variable names, see #23901.

[a.hoereth]

wp.revisions.view.Diff comment wrong due to copy/paste (I reckon): ​revisions.js:606ff

[adamsilverstein]

Replying to a.hoereth:

wp.revisions.view.Diff comment wrong due to copy/paste (I reckon): ​revisions.js:606ff

is that describing the wrong code block? wasn't certain: can you create a diff that shows what needs to change?

[a.hoereth]

Replying to adamsilverstein:

Replying to a.hoereth:

wp.revisions.view.Diff comment wrong due to copy/paste (I reckon): ​revisions.js:606ff

is that describing the wrong code block? wasn't certain: can you create a diff that shows what needs to change?

​Line 533 describes the buttons and slider view (div#revisions-interact), while ​Line 609 describes the Diff view (div#revisions-diff).

[adamsilverstein]

reduce min width of tick marks to 6px

[adamsilverstein]

23901.9.diff​​ reduces min width of tick to 6 px, letting close to 100 tick marks show on 640 pixel wide screen.

looks like this -

[ocean90]

In 24253:

Revisions JS: Fix inline docs. props a.hoereth. see #23901.

[ocean90]

In 24254:

Revisions UI: Reduce the min width of a tick. props adamsilverstein. see #23901.

[a.hoereth]

Replying to adamsilverstein:

23901.9.diff​​ reduces min width of tick to 6 px, letting close to 100 tick marks show on 640 pixel wide screen.

looks like this -

This does not adapt when resizing the browser window. Recognized it when I moved chrome from 1920 to 1366 - it breaks the 100% width.

[adamsilverstein]

Replying to a.hoereth:

Replying to adamsilverstein:

23901.9.diff​​ reduces min width of tick to 6 px, letting close to 100 tick marks show on 640 pixel wide screen.

looks like this -

This does not adapt when resizing the browser window. Recognized it when I moved chrome from 1920 to 1366 - it breaks the 100% width.

would be a good idea to recalculate bar on resize; if you reload the page does it load properly?