WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 11 months ago

Last modified 11 months ago

#23901 closed defect (bug) (fixed)

Revisions: revisions.js clean up

Reported by: ocean90 Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Revisions Keywords: has-patch
Focuses: Cc:

Description

Follow-up: #23497

Yes it works, but it can be prettier.

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

Attachments (19)

23901.patch (49.6 KB) - added by ocean90 13 months ago.
23901.2.patch (48.5 KB) - added by ocean90 13 months ago.
23901.3.patch (56.3 KB) - added by ocean90 13 months ago.
23901.diff (56.3 KB) - added by adamsilverstein 13 months ago.
corrects left handle reload issue
23901.2.diff (56.3 KB) - added by adamsilverstein 13 months ago.
corrects not reloading data on left handle stop no logging
23901.4.patch (58.2 KB) - added by ocean90 13 months ago.
23901.3.diff (58.5 KB) - added by adamsilverstein 13 months ago.
adds left_handle_add to rind handle data call, corrects data return
23901.4.diff (58.5 KB) - added by adamsilverstein 13 months ago.
autonaming sequence update upload
23901.5.patch (58.5 KB) - added by ocean90 13 months ago.
23901.5.diff (2.3 KB) - added by kovshenin 13 months ago.
23901.6.diff (2.4 KB) - added by ocean90 13 months ago.
23901-7.patch (1.5 KB) - added by azaozz 13 months ago.
restoring-autosave.png (12.3 KB) - added by azaozz 13 months ago.
23901.7.diff (1.3 KB) - added by adamsilverstein 13 months ago.
use date not ID for left->right compare; show all revisions in post meta box including most current;
23901.8.diff (1.3 KB) - added by adamsilverstein 13 months ago.
corrected with post_date
23901-9.patch (2.3 KB) - added by azaozz 13 months ago.
23901.10.patch (9.4 KB) - added by ocean90 12 months ago.
23901.10-b.diff (10.6 KB) - added by adamsilverstein 12 months ago.
23901.9.diff (679 bytes) - added by adamsilverstein 12 months ago.
reduce min width of tick marks to 6px

Download all attachments as: .zip

Change History (56)

comment:1 ocean9013 months ago

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.

ocean9013 months ago

comment:2 adamsilverstein13 months ago

  • Cc adamsilverstein@… added

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

ocean9013 months ago

comment:3 ocean9013 months ago

23901.2.patch addresses the issues mentioned by adamsilverstein.

Moved wp_ajax_revisions_data() changes to #23920.

comment:4 adamsilverstein13 months ago

thanks, you beat me to it. will test.

ocean9013 months ago

adamsilverstein13 months ago

corrects left handle reload issue

adamsilverstein13 months ago

corrects not reloading data on left handle stop no logging

ocean9013 months ago

comment:5 ocean9013 months ago

#23920 was marked as a duplicate.

adamsilverstein13 months ago

adds left_handle_add to rind handle data call, corrects data return

adamsilverstein13 months ago

autonaming sequence update upload

comment:6 adamsilverstein13 months ago

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.

comment:7 azaozz13 months ago

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
    
Last edited 13 months ago by azaozz (previous) (diff)

comment:8 follow-up: azaozz13 months ago

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].

ocean9013 months ago

comment:9 ocean9013 months ago

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

comment:10 markjaquith13 months ago

In 23898:

Further cleanup of revisions code. Probably not the last.

see #23901. props adamsilverstein, azaozz, ocean90.

comment:11 markjaquith13 months ago

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.

kovshenin13 months ago

comment:12 in reply to: ↑ 8 kovshenin13 months ago

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.

comment:13 adamsilverstein13 months ago

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

ocean9013 months ago

comment:14 markjaquith13 months ago

In 23904:

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

azaozz13 months ago

comment:15 azaozz13 months ago

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

azaozz13 months ago

comment:16 follow-up: azaozz13 months ago

  • Priority changed from normal to high

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.

comment:17 in reply to: ↑ 16 ; follow-up: adamsilverstein13 months ago

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.

comment:18 in reply to: ↑ 17 ; follow-ups: azaozz13 months ago

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?

Last edited 13 months ago by azaozz (previous) (diff)

comment:19 in reply to: ↑ 18 adamsilverstein13 months ago

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

comment:20 adamsilverstein13 months ago

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:

http://f.cl.ly/items/3g2K070j113L363W3H2K/Screen%20Shot%202013-04-08%20at%209.20.55%20AM.jpg

i did a little sort order testing:

i tried changing the sort order from date:

http://f.cl.ly/items/0b1H1j0v1N3k1E3l1k2P/Screen%20Shot%202013-04-08%20at%209.34.13%20AM.jpg

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

http://f.cl.ly/items/2B0p151C1P0G1e2I3H0x/Screen%20Shot%202013-04-08%20at%209.32.22%20AM.jpg

adamsilverstein13 months ago

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

comment:21 in reply to: ↑ 18 ; follow-up: adamsilverstein13 months ago

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?

adamsilverstein13 months ago

corrected with post_date

comment:22 in reply to: ↑ 21 ; follow-up: adamsilverstein13 months ago

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​

azaozz13 months ago

comment:23 in reply to: ↑ 22 ; follow-up: azaozz13 months ago

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().

Last edited 13 months ago by azaozz (previous) (diff)

comment:24 in reply to: ↑ 23 adamsilverstein13 months ago

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.

comment:25 azaozz12 months ago

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

ocean9012 months ago

comment:26 ocean9012 months ago

  • Keywords has-patch added
  • Priority changed from high to normal

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

comment:27 adamsilverstein12 months ago

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:

http://f.cl.ly/items/2R2J0f1C1N3f3j1V2U0v/Screen%20Shot%202013-04-17%20at%2012.49.58%20PM.jpg

after:

http://f.cl.ly/items/0z1G1K0j0E0e2S0P331q/Screen%20Shot%202013-04-17%20at%2012.50.40%20PM.jpg

comment:28 ocean9012 months ago

In 24019:

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

comment:29 follow-up: a.hoereth12 months ago

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

comment:30 in reply to: ↑ 29 ; follow-up: adamsilverstein12 months ago

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?

comment:31 in reply to: ↑ 30 a.hoereth12 months ago

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).

adamsilverstein12 months ago

reduce min width of tick marks to 6px

comment:32 follow-up: adamsilverstein12 months ago

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 -

http://f.cl.ly/items/2b1c063J3F403I3L061b/Screen%20Shot%202013-05-07%20at%204.44.35%20PM.jpg

comment:33 ocean9011 months ago

In 24253:

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

comment:34 ocean9011 months ago

In 24254:

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

comment:35 ocean9011 months ago

  • Resolution set to fixed
  • Status changed from new to closed

comment:36 in reply to: ↑ 32 ; follow-up: a.hoereth11 months ago

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 -

http://f.cl.ly/items/2b1c063J3F403I3L061b/Screen%20Shot%202013-05-07%20at%204.44.35%20PM.jpg

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

Last edited 11 months ago by a.hoereth (previous) (diff)

comment:37 in reply to: ↑ 36 adamsilverstein11 months ago

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 -

http://f.cl.ly/items/2b1c063J3F403I3L061b/Screen%20Shot%202013-05-07%20at%204.44.35%20PM.jpg

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?

Note: See TracTickets for help on using tickets.