Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#23901 closed defect (bug) (fixed)

Revisions: revisions.js clean up

Reported by: ocean90's profile 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 12 years ago.
23901.2.patch (48.5 KB) - added by ocean90 12 years ago.
23901.3.patch (56.3 KB) - added by ocean90 12 years ago.
23901.diff (56.3 KB) - added by adamsilverstein 12 years ago.
corrects left handle reload issue
23901.2.diff (56.3 KB) - added by adamsilverstein 12 years ago.
corrects not reloading data on left handle stop no logging
23901.4.patch (58.2 KB) - added by ocean90 12 years ago.
23901.3.diff (58.5 KB) - added by adamsilverstein 12 years ago.
adds left_handle_add to rind handle data call, corrects data return
23901.4.diff (58.5 KB) - added by adamsilverstein 12 years ago.
autonaming sequence update upload
23901.5.patch (58.5 KB) - added by ocean90 12 years ago.
23901.5.diff (2.3 KB) - added by kovshenin 12 years ago.
23901.6.diff (2.4 KB) - added by ocean90 12 years ago.
23901-7.patch (1.5 KB) - added by azaozz 12 years ago.
restoring-autosave.png (12.3 KB) - added by azaozz 12 years ago.
23901.7.diff (1.3 KB) - added by adamsilverstein 12 years 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 12 years ago.
corrected with post_date
23901-9.patch (2.3 KB) - added by azaozz 12 years ago.
23901.10.patch (9.4 KB) - added by ocean90 12 years ago.
23901.10-b.diff (10.6 KB) - added by adamsilverstein 12 years ago.
23901.9.diff (679 bytes) - added by adamsilverstein 12 years ago.
reduce min width of tick marks to 6px

Download all attachments as: .zip

Change History (56)

#1 @ocean90
12 years 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.

@ocean90
12 years ago

#2 @adamsilverstein
12 years 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

@ocean90
12 years ago

#3 @ocean90
12 years ago

23901.2.patch addresses the issues mentioned by adamsilverstein.

Moved wp_ajax_revisions_data() changes to #23920.

#4 @adamsilverstein
12 years ago

thanks, you beat me to it. will test.

@ocean90
12 years ago

@adamsilverstein
12 years ago

corrects left handle reload issue

@adamsilverstein
12 years ago

corrects not reloading data on left handle stop no logging

@ocean90
12 years ago

#5 @ocean90
12 years ago

#23920 was marked as a duplicate.

@adamsilverstein
12 years ago

adds left_handle_add to rind handle data call, corrects data return

@adamsilverstein
12 years ago

autonaming sequence update upload

#6 @adamsilverstein
12 years 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.

#7 @azaozz
12 years 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 12 years ago by azaozz (previous) (diff)

#8 follow-up: @azaozz
12 years 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].

@ocean90
12 years ago

#9 @ocean90
12 years ago

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

#10 @markjaquith
12 years ago

In 23898:

Further cleanup of revisions code. Probably not the last.

see #23901. props adamsilverstein, azaozz, ocean90.

#11 @markjaquith
12 years 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.

@kovshenin
12 years ago

#12 in reply to: ↑ 8 @kovshenin
12 years 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.

#13 @adamsilverstein
12 years ago

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

@ocean90
12 years ago

#14 @markjaquith
12 years ago

In 23904:

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

@azaozz
12 years ago

#15 @azaozz
12 years ago

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

#16 follow-up: @azaozz
12 years 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.

#17 in reply to: ↑ 16 ; follow-up: @adamsilverstein
12 years 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.

#18 in reply to: ↑ 17 ; follow-ups: @azaozz
12 years 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 12 years ago by azaozz (previous) (diff)

#19 in reply to: ↑ 18 @adamsilverstein
12 years 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

#20 @adamsilverstein
12 years 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

@adamsilverstein
12 years ago

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

#21 in reply to: ↑ 18 ; follow-up: @adamsilverstein
12 years 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?

@adamsilverstein
12 years ago

corrected with post_date

#22 in reply to: ↑ 21 ; follow-up: @adamsilverstein
12 years 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​

@azaozz
12 years ago

#23 in reply to: ↑ 22 ; follow-up: @azaozz
12 years 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 12 years ago by azaozz (previous) (diff)

#24 in reply to: ↑ 23 @adamsilverstein
12 years 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.

#25 @azaozz
12 years 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

@ocean90
12 years ago

#26 @ocean90
12 years ago

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

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

#27 @adamsilverstein
12 years 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

#28 @ocean90
12 years ago

In 24019:

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

#29 follow-up: @a.hoereth
12 years ago

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

#30 in reply to: ↑ 29 ; follow-up: @adamsilverstein
12 years 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?

#31 in reply to: ↑ 30 @a.hoereth
12 years 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).

@adamsilverstein
12 years ago

reduce min width of tick marks to 6px

#32 follow-up: @adamsilverstein
12 years 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

#33 @ocean90
12 years ago

In 24253:

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

#34 @ocean90
12 years ago

In 24254:

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

#35 @ocean90
12 years ago

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

#36 in reply to: ↑ 32 ; follow-up: @a.hoereth
12 years 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 12 years ago by a.hoereth (previous) (diff)

#37 in reply to: ↑ 36 @adamsilverstein
12 years 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.