#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)
Change History (56)
#2
@
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
#3
@
12 years ago
23901.2.patch addresses the issues mentioned by adamsilverstein.
Moved wp_ajax_revisions_data()
changes to #23920.
#6
@
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
@
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 beif ( ! $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
#8
follow-up:
↓ 12
@
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].
#11
@
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.
#12
in reply to:
↑ 8
@
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.
#15
@
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:
↓ 17
@
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:
↓ 18
@
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:
↓ 19
↓ 21
@
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?
#19
in reply to:
↑ 18
@
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
@
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:
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:
@
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:
↓ 22
@
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?
#22
in reply to:
↑ 21
;
follow-up:
↓ 23
@
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
#23
in reply to:
↑ 22
;
follow-up:
↓ 24
@
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().
#24
in reply to:
↑ 23
@
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.
#26
@
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
@
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:
after:
#29
follow-up:
↓ 30
@
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:
↓ 31
@
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
@
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).
#32
follow-up:
↓ 36
@
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 -
#36
in reply to:
↑ 32
;
follow-up:
↓ 37
@
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 -
This does not adapt when resizing the browser window. Recognized it when I moved chrome from 1920 to 1366 - it breaks the 100% width.
#37
in reply to:
↑ 36
@
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 -
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?
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.