Revisions Rewrite using JS/Backbone

[adamsilverstein]

notes:

• to install, apply the diff and extract images in wp-includes-images.zip into wp-includes/images (images from jquery-ui-slider)
• interface rewritten with JS/Backbone
• to use, click Revisions: 'View' link, in post publish box
• design cues from @karmatosed ​here and @lessbloat ​here (and developed in #23396)
• revisions are loaded asynchronously
• allows rapid review of all revisions using previous/next buttons and a slider
• always shows what each revision contains - what it removed, added and what was unchanged, compared to the preceding revision
• each diff is pre-calculated for rapid paging through revisions
• allows comparison in split view or combined view, ajax refresh
• allows user to show or hide (default) auto-saves, ajax refresh
• skips revisions which include no changes, including the first save (note - unchanged revisions are no longer saved, see #7392)
• includes link to old revisions system, for these missing features: revisions list, compare any two revisions and the easter egg :)

the attached is a proof-of-concept, with plenty of room for improvement:

• currently the diff view is not rendered until all revisions are loaded
• diffs are calculated with each load, cache these calculations?
• hook to turn off new revisions system for backwards compatibility, plugins
• only load whats changes when options changed for auto-saves, split/combined view options ( currently refreshes revisions from server and logic is there )

screenshots:

[adamsilverstein]

backbone revisions

[adamsilverstein]

images for jquery-ui-slider

[wonderboymusic]

Dang. Looks cool.

If you are going to use Backbone templates, you need to not use ASP-style tags, which are allowed by PHP config, which is lame. Koop set up a template function for media but didn't expose it globally, so you need to do something like:

template = _.memoize(function (id) {
	var compiled, options = {
		evaluate : /<#([\s\S]+?)#>/g,
		interpolate: /\{\{\{([\s\S]+?)\}\}\}/g,
		escape: /\{\{([^\}]+?)\}\}(?!\})/g,
		variable: 'data'
	};

	return function (data) {
		compiled = compiled || _.template($('#tmpl-' + id).html(), null, options);
		return compiled(data);
	};
});

I think the script tags also have to be set to text/html for some weird IE reason.

[rmccue]

Replying to wonderboymusic:

Koop set up a template function for media but didn't expose it globally

Related: #23263. May want to patch that before this one so we don't go repeating it everywhere.

[adamsilverstein]

thanks, i'll integrate that - i'm new to backbone and templates have only tested in firefox so far (like i said its a proof-of-concept); also noticed two issues, there is no 'cancel' button and the current version is not shown. maybe just need a button 'Cancel Restore keep Current Version'

Replying to wonderboymusic:

Dang. Looks cool.

If you are going to use Backbone templates, you need to not use ASP-style tags, which are allowed by PHP config, which is lame. Koop set up a template function for media but didn't expose it globally, so you need to do something like:

template = _.memoize(function (id) {
	var compiled, options = {
		evaluate : /<#([\s\S]+?)#>/g,
		interpolate: /\{\{\{([\s\S]+?)\}\}\}/g,
		escape: /\{\{([^\}]+?)\}\}(?!\})/g,
		variable: 'data'
	};

	return function (data) {
		compiled = compiled || _.template($('#tmpl-' + id).html(), null, options);
		return compiled(data);
	};
});

I think the script tags also have to be set to text/html for some weird IE reason.

[scribu]

You should transform the code that starts after // Set up the buttons into one or more Backbone views.

[scribu]

Also, the indentation for the Revision model looks screwy.

[ocean90]

• jQuery(function() { .. }(jQuery)) should be $(function() { .. }(jQuery)) and then you can just use $
• Take a look on how script/style dependencies are working.
• wp-admin/data/revisions-data.php - IMO an admin-ajax.php action would be better.

[rmccue]

Replying to ocean90:

• jQuery(function() { .. }(jQuery)) should be $(function() { .. }(jQuery)) and then you can just use $

I think you mean (function($) { ... })(jQuery); :)

• wp-admin/data/revisions-data.php - IMO an admin-ajax.php action would be better.

+1, the file itself can live in wp-admin/includes/

[adamsilverstein]

Replying to scribu:

You should transform the code that starts after // Set up the buttons into one or more Backbone views.

yes, i agree. i had it set up that way originally, but ran into problems with my slider which would get reloaded when the review refreshed. so i redid it this way because it worked... ideally i'd like to switch the code so the individual revision models are self rendering, rather than having the overall diff view doing the render. this should also allow the 1st diff to display as the rest are still loading.

[adamsilverstein]

Replying to rmccue:

Replying to ocean90:

• jQuery(function() { .. }(jQuery)) should be $(function() { .. }(jQuery)) and then you can just use $

I think you mean (function($) { ... })(jQuery); :)

• wp-admin/data/revisions-data.php - IMO an admin-ajax.php action would be better.

+1, the file itself can live in wp-admin/includes/

i will try it that way, thanks.

[adamsilverstein]

Replying to wonderboymusic:

Dang. Looks cool.

If you are going to use Backbone templates, you need to not use ASP-style tags, which are allowed by PHP config, which is lame. Koop set up a template function for media but didn't expose it globally, so you need to do something like:

template = _.memoize(function (id) {
	var compiled, options = {
		evaluate : /<#([\s\S]+?)#>/g,
		interpolate: /\{\{\{([\s\S]+?)\}\}\}/g,
		escape: /\{\{([^\}]+?)\}\}(?!\})/g,
		variable: 'data'
	};

	return function (data) {
		compiled = compiled || _.template($('#tmpl-' + id).html(), null, options);
		return compiled(data);
	};
});

I think the script tags also have to be set to text/html for some weird IE reason.

once i have that code in there, can you tell me how to change the template code as well? thanks

[adamsilverstein]

IMPORTANT - to use this code, click Revisions: 'View' link, in post publish box!

[wonderboymusic]

attachment:23497.diff​​ moves the AJAX code to admin-ajax.php and ajax-actions.php, uses the templating originally impl'd by media, and rehabilitates some whitespace / removes unused code / etc. I didn't actually change much code, just made it work in the ways I described.

[adamsilverstein]

Replying to wonderboymusic:

attachment:23497.diff​​ moves the AJAX code to admin-ajax.php and ajax-actions.php, uses the templating originally impl'd by media, and rehabilitates some whitespace / removes unused code / etc. I didn't actually change much code, just made it work in the ways I described.

Thank You! i knew i needed help, but had to start somewhere. i'll pull this diff and integrate; i've got several other changes from our Revisions meeting this morning.

[duck_]

Looks good! :)

Quick review of the patch (I looked at 23497.diff from wonderboymusic):

• XSS in revisions.php: $postid should be run through absint()
• revisions.php needs some caps checks
• $action isn't used in revisions.php, but it's reset from GET/POST [Edit: probably because it just hasn't been implemented yet, sorry]
• wp_reset_vars() in the ajax action isn't very nice as it relies on globals
• Missing $suffix on when revisions.js is registered? (Plus an unnecessary space in array ('backbone)

I know this is WIP, but I wanted to mention these things so they aren't forgotten.

[adamsilverstein]

Replying to duck_:

Looks good! :)

Quick review of the patch (I looked at 23497.diff from wonderboymusic):

• XSS in revisions.php: $postid should be run through absint()
• revisions.php needs some caps checks
• $action isn't used in revisions.php, but it's reset from GET/POST [Edit: probably because it just hasn't been implemented yet, sorry]
• wp_reset_vars() in the ajax action isn't very nice as it relies on globals
• Missing $suffix on when revisions.js is registered? (Plus an unnecessary space in array ('backbone)

I know this is WIP, but I wanted to mention these things so they aren't forgotten.

appreciate the feedback, will work on integrating. if you are testing note that the patch from wonderboymusic slightly broke a few things i am fixing

[adamsilverstein]

change notes for ​23497.2.diff

• remove 'unchanged'
• fixed split diff view css issue
• changed removed/added back so symbol to right of text
• added Cancel button, goes back to post
• added Compare two revisions button at the bottom of the diff view, as per latest mockup on #23996, ​http://core.trac.wordpress.org/attachment/ticket/23396/revisions11.png
• switched comparison action so current version is compared to revision instead of comparing revision to previous revision, as per chat
• changed ajax call so its asking for 'compareto' - possibly enable compare any two revisions with (optional) second slider
• apply absint to $postid in revisions.php
• added some caps checks to revisions.php
• left action for
• question re: wp_reset_vars call, should i just use $_POST[]?
• added correct $suffix and fixed extra space in script-loader.php

[duck_]

Replying to adamsilverstein:

• question re: wp_reset_vars call, should i just use $_POST[]?

Yeah. Remember to wp_unslash() where necessary.

[adamsilverstein]

Replying to duck_:

Replying to adamsilverstein:

• question re: wp_reset_vars call, should i just use $_POST[]?

Yeah. Remember to wp_unslash() where necessary.

will do, thanks for all the help!

[adamsilverstein]

• remove wp_reset_vars call in ajax-actions.php, * fixed broken 'show autosaves'

[adamsilverstein]

added gravatar image, as per design, date change; css diff view updates as per latest mockup ('Added +" left aligned)

[adamsilverstein]

side by side 'split' diff view

[adamsilverstein]

thanks to everyone for all the help so far on this code, especially to @wonderboymusic for his extensive fixes/refactoring.

i would like some help with a few outstanding ideas/issues from some more expert backbone ninjas

• can the render action fire as soon as the current view revision is loaded? can we do that with a list return as currently, or do we need code that returns a single revision? would work better with many/large revisions or slow internet, as it is all diffs have to load before the view renders.
• what hooks should we add to the code, did i loose any?

thanks in advance for any tips.

[adamsilverstein]

formatting updates

[adamsilverstein]

23497.5.diff changes

• clean up spacing, wording, etc. to bring design closer to latest mockup (​http://core.trac.wordpress.org/attachment/ticket/23396/comparing-one.png).
• add space between columns in split diff view as per mockup and remove extra margins left/right of div
• add checkbox for 'Compare two revisions' (not active yet) - working on that next...

heres how it looks:

[adamsilverstein]

Uploading a new version, ready for testing

Notes

• allows comparison of two revisions, using range slider
• when in compare two mode, the left most spot in the slider represents the 'current version'
• when in compare one mode, the comparison is always to the current version, the leftmost slider spot represents the 1st (most recent) revision
• slider leftmost position is most recent, rightmost oldest - is this obvious?
• currently using range slider which prevents handles from crossing automatically; a two handled slider would also work, with some code changes
• updated wp-includes images folder (gray color for range bar)
• code refactored for cleaner interactions, but still plenty of room for improvement!
• code prevents overlapping handles, eg. compare to self, bye bye easter egg? suggestions?
• current version shows info about revisions being compared on same line, in compare two revisions mode, these only fit on a wide screen; probably need to create a separate row for compare from details.
• expanded meta info, possibly needs shortening
• showing/hiding divs to keep words 'current version' translatable and in the PHP not JS side, is there a better way?
• when in compare two mode, all diffs are calculated for right and left handles whenever sliding stops - the appropriate model is loaded as soon as sliding begins again, possible to get blank diffs when comparing

TODO notes:

• currently always restores right handle position in compare two mode, should we add restore left/restore right? 2nd restore button?
• update restore action:
  • capture and store the restore event in post meta ('post_restored_from') as per ​IRC - store: revision id, date of restore, user who restored;
  • edit_last and post_author set to current user - see #20982.
• update meta box on edit post screen, show revision meta info: count, earliest, oldest; remove link from publish box
• verify all filters working, and new hooks?
• show autosaves by default when user clicks links from warning dialog 'an autosave exists for this post'

Screenshots:

Compare two:

Compare one:

[adamsilverstein]

updated gray range color

[adamsilverstein]

adds compare two revisions

[adamsilverstein]

applies cleanly to trunk

[adamsilverstein]

note: when testing please create a new post with revisions, current patch apparently broken for older posts. (fixing that!)

[adamsilverstein]

fixes compare two math

[adamsilverstein]

code cleanup

[adamsilverstein]

note, when applying current patch for the second time, make sure you delete new files (or start with a fresh svn co) -- otherwise patch duplicates content in the new files.

[adamsilverstein]

rework edit post screen revision meta box, move code into main revision.php file

[adamsilverstein]

changes:

• clean up application variables
• freeze slider interactions while models loading, also prevent multiple overlapping ajax calls
• add loading indicators while models loading, spinner + message
• added initial left/right load when switching to compare two
• consolidated model reload calls into main app
• better serialization of returned data (echo as iterating revisions)
• moved code from revisions.php to revision.php
• removed link from publish box
• reworked Revisions meta box. so far box includes: number of revisions, newest & oldest revision meta, plus a link to enter revision compare page:

note: patch includes the following new files:

wp-includes/js/template.js, wp-includes/js/revisions.js, wp-admin/css/revisions.css and wp-includes/css/jquery-ui-slider.css

these files need to be deleted before reapplying this patch if you have already applied it or the patch command will duplicate content in the files.

[westi]

I'm working through reviewing the patch and getting it to the point where I'm happy to commit it.

[adamsilverstein]

thanks. i am posting one more patch now that includes storing _post_restored_from when restoring a revision and displaying that info in the post meta box. i can integrate this later if you are already into the updates.

meta box with restore info:

[adamsilverstein]

stores _post_restored_from post meta on restore, displays in meta box

[adamsilverstein]

Replying to westi:

I'm working through reviewing the patch and getting it to the point where I'm happy to commit it.

sure you noticed, we need to enable a link into viewing a specific revision, as when users return to a page with an autosave and gets "There is an autosave of this post that is more recent than the version below. View the autosave" link in format ​http://siteurl/wp-admin/revision.php?revision=25651&action=edit

also we need to provide an accurate count of revisions, excluding duplicates, for the post meta box, so the count matches the revisions interface which already excludes duplicates

[westi]

Here are my notes from my pre-commit review and patch massaging. I focused primarily on getting the Backbone view of things into a state which I was happy to commit and in the process may have broken somethings accidentally and also removed some changes from landing in this version.

Things I removed for now:

• I reverted the metabox changes to use the old list function - I think we should discuss the best presentation we can for the meta box that still gives instant access to each revision

Fixes I made:

• Security improvements - I tried to restore all the missing capability checks but this needs a second review
• I added CSRF projection to the Ajax Request
• I tried to better scope the data we pass to the Backbone code by wrapping it in a js object
• I switch the code back to support as much as possible old entry points and not rename variables/query string args too much
• I fixed up translations which had leading or trailing whitespace - we can't do this as it just gets "lost in translation" by accident
• I noted some TODO's
• I fixed up some of the variable naming to be more WP like but it could do with a second pass so that it follows the coding style more closely
• I cleanup the enqueuing of js and css to use dependencies rather than enqueuing many scripts.
• I was scared by all the Backbone code because it is currently outside my comfort zone, I need to up-skill here :)
• I hid the "screen options" that allow you to switch between split view and save autosaves - need to loop back and fix these up

Todos:

• Move the advance option stuff that is hidden to screen options or a better place in the UI
• Fix the split translations noted in wp-admin/revisions.php to be safe for RTL translation
• There are other TODO's in the code that need reviewing and handling
• Should we put back a no-js version of the code?
• Looks like the UI slider CSS references some images and I'm not sure we have them on this ticket
• More testing of how the new UI works with plugins that use revisions.

Other things I checked:

• We preserved the _wp_post_revision_field_$field filters - they moved to the ajax action
• Plugins that use CPT and revisions - e.g Jetpack Custom CSS worked fine in my light testing

Other notes:

• This change probably breaks some of the other revisions patches, sorry ;)

I think this is good enough to commit and iterate on so we can move to smaller easier to review patches :)

[westi]

In 23506:

Revisions: First pass an implementing a new UI/UX for reviewing the revisions of posts. See #23497 props adamsilverstein for the initial patch.

This implements a new revisions ui using Backbone and preserves all the old methods of "integration" so the change should be transparent to plugins using revisi
ons with CPTs.

This is the first pass and so there are a number of things still to be resolved, more details in the ticket. Feedback welcomed.

[helen]

In 23507:

Merge revisions.css into wp-admin.css. Move misplaced block of revisions CSS in wp-admin.css into appropriate section. Some standards clean up and selector merging. see #23497.

[adamsilverstein]

correct ajax calls

[westi]

In 23508:

Revisions: Fix up some bugs I introduced while reviewing the mega revisions patch - when comparing two historical revisions only one half of the diff would load

See #23497 props adamsilverstein.

[adamsilverstein]

corrects split view variable passthru

[westi]

In 23509:

Revisions: Fix up some half renamed variables that break the view and display a mismash of split and combined views. See #23497 props adamsilverstein.

[azaozz]

Some quick notes:

• jquery-ui-slider.css references about 10 images that are missing. Apparently the slider works without them (regardless of the browser console filling with 404s). Perhaps it needs some changes to exclude these backgrounds or replace couple of them them with gradients?
• Does template.js need to load every time backbone.js is loaded (doesn't seem needed for media)? Is there a better place for this function instead of a separate file (since it's only about 10 lines).

And some nitpicking:

• Looking at ajax-actions.php: $showautosaves and $showsplitview appears to be boolean, why not check them with ! empty(). Also $postid should probably be $post_id as most other places in the admin, foreach blocks should be indented, and there seems to be a typo on ​line 2156.

[helen]

Replying to azaozz:

• jquery-ui-slider.css

karmatosed is going to look at slimming down the CSS to no images and as little as is required for the slider, along with bringing the styling more in line with the admin. I'll be watching for that and helping out as well, and will probably just merge it into wp-admin.css and remove the separate sheet.

[adamsilverstein]

Replying to azaozz:

Some quick notes:

• jquery-ui-slider.css references about 10 images that are missing. Apparently the slider works without them (regardless of the browser console filling with 404s). Perhaps it needs some changes to exclude these backgrounds or replace couple of them them with gradients?
• Does template.js need to load every time backbone.js is loaded (doesn't seem needed for media)? Is there a better place for this function instead of a separate file (since it's only about 10 lines).

And some nitpicking:

• Looking at ajax-actions.php: $showautosaves and $showsplitview appears to be boolean, why not check them with ! empty(). Also $postid should probably be $post_id as most other places in the admin, foreach blocks should be indented, and there seems to be a typo on ​line 2156.

thanks; i will correct the variable names, my mistake for not following convention. if you want to get rid of the console errors for now, the missing images are here: ​http://core.trac.wordpress.org/attachment/ticket/23497/wp-includes-images.2.zip

will also adjust boolean checks, thanks for the tip.

[ocean90]

Some results after a small review:

---

Strange behavior:

• I have a post with 3 revisions.
• I select the oldest one.
• I activate "Compare two revisions"
• I drag the right handler of the slider one step to the right. (Comparing current version to revision 2 of 3)
• I drag the left handler one step to the right. (Comparing revision 1 to revision 2 of 3)
• I get Cannot compare revision to itself.
• I drag the right handler one step to the right. (Comparing revision 1 to revision 3 of 3)
• I get nothing.

---

See ​http://cl.ly/NEKV and ​http://cl.ly/NFSN

The whole line shouldn't be crossed out when just a word or sentence has been removed.
text-decoration: line-through should be removed from table.diff .diff-deletedline and added to table.diff .diff-deletedline del.

---

From the JS console (Chrome)

Uncaught TypeError: Cannot call method 'replace' of undefined underscore.min.js:5
w.template underscore.min.js:5
(anonymous function) template.js:24
jQuery.extend.access jquery.js:866
jQuery.fn.extend.html jquery.js:6038
_.extend.Options.Backbone.View.extend.render revisions.js:314
wp.revisions.App.Backbone.Router.extend.revisionDiffSetup revisions.js:212
self._revisions.fetch.success revisions.js:189
a.success backbone.min.js:23
fire jquery.js:1037
self.fireWith jquery.js:1148
done jquery.js:8074
callback

---

wpRevisionsSettings should be added via wp_localize_script().

---

Is there a way to avoid the use of onClick attributes?

---

tmpl-revisionvinteract has missing gettext calls. (Next/Prev)

---

Would be nice, if we can rewrite the CSS class names to add some dashes.

---

Finally: I'm not a huge fan of the use of a slider here. Sliders doesn't work good if there are too few revisions/steps (the next step is 200px away) or too many revisions/steps (each pixel is a new step).

[helen]

Definitely need to do a review of HTML and CSS naming and structure - there's a lot of ID usage going on the CSS that I'm not super convinced about as well. ocean90 - do you want to look at that?

Other notes I had:

• The clearing break at the top pushes the whole wrap down. Does that div + br really need to be there? If something appears there (which I haven't managed to trigger), it would make the page jumpy, which is also not great. I don't think anything should come before or really outside of the wrap at all, anyway.
• The "Restore" button doesn't tell me what exactly is being restored, especially in compare-two mode. Not sure what a good way is to handle that, especially given translations, but perhaps adding the revision number would be helpful.

[adamsilverstein]

thanks for the feedback/testing, i will try to address your notes. just to verify, did you create a new post for testing? the existing code has known wonky behavior with existing posts that i'm fixing. i tried to reproduce what you did with a new post and did not get the problem you mention.

i agree the a slider is bit ugly with too few revisions, and that whole row should get hidden if there is only one revision. once you get to the 5-100 revisions range it works pretty well though.

the line through was added to the deleted block site to help color blind users make out the change, the changed word is also highlighted with a darker red background.

Replying to ocean90:

Some results after a small review:

---

Strange behavior:

• I have a post with 3 revisions.
• I select the oldest one.
• I activate "Compare two revisions"
• I drag the right handler of the slider one step to the right. (Comparing current version to revision 2 of 3)
• I drag the left handler one step to the right. (Comparing revision 1 to revision 2 of 3)
• I get Cannot compare revision to itself.
• I drag the right handler one step to the right. (Comparing revision 1 to revision 3 of 3)
• I get nothing.

---

See ​http://cl.ly/NEKV and ​http://cl.ly/NFSN

The whole line shouldn't be crossed out when just a word or sentence has been removed.
text-decoration: line-through should be removed from table.diff .diff-deletedline and added to table.diff .diff-deletedline del.

---

From the JS console (Chrome)

Uncaught TypeError: Cannot call method 'replace' of undefined underscore.min.js:5
w.template underscore.min.js:5
(anonymous function) template.js:24
jQuery.extend.access jquery.js:866
jQuery.fn.extend.html jquery.js:6038
_.extend.Options.Backbone.View.extend.render revisions.js:314
wp.revisions.App.Backbone.Router.extend.revisionDiffSetup revisions.js:212
self._revisions.fetch.success revisions.js:189
a.success backbone.min.js:23
fire jquery.js:1037
self.fireWith jquery.js:1148
done jquery.js:8074
callback

---

wpRevisionsSettings should be added via wp_localize_script().

---

Is there a way to avoid the use of onClick attributes?

---

tmpl-revisionvinteract has missing gettext calls. (Next/Prev)

---

Would be nice, if we can rewrite the CSS class names to add some dashes.

---

Finally: I'm not a huge fan of the use of a slider here. Sliders doesn't work good if there are too few revisions/steps (the next step is 200px away) or too many revisions/steps (each pixel is a new step).

[adamsilverstein]

that clearing break was a leftover from my options mockup (now removed) - will remove it in my next patch.

i had the same concern about the restore button, it always restores the right hand revision, but thats not obvious. thought about two retore buttons (left/right) but i think your idea is best, will try this: change button to 'Restore Revision ID:X' we can translate the words in revision.php, i think.

Replying to helen:

Definitely need to do a review of HTML and CSS naming and structure - there's a lot of ID usage going on the CSS that I'm not super convinced about as well. ocean90 - do you want to look at that?

Other notes I had:

• The clearing break at the top pushes the whole wrap down. Does that div + br really need to be there? If something appears there (which I haven't managed to trigger), it would make the page jumpy, which is also not great. I don't think anything should come before or really outside of the wrap at all, anyway.
• The "Restore" button doesn't tell me what exactly is being restored, especially in compare-two mode. Not sure what a good way is to handle that, especially given translations, but perhaps adding the revision number would be helpful.

[adamsilverstein]

more updates, fixes

[adamsilverstein]

changes/corrections

• correct variable misname: REVAPP.right_model.loading -> REVAPP.right_model_loading
• correct variable name convention across code in revisions.js/revisions.php/ajax-actions.php/pluggable.php
• didn't change css names yet, change IDs to classes? cleanup help appreciated.
• remove extraneous <br clear> above wrap
• changed checks for booleans to ! empty()
• indent foreach block
• added translations for Next/Previous buttons
• added _post_restored_from meta when restoring revision, currently a single meta entry (should this be one per restore for full audit trail?)
• added display of _post_restored_from in post revisions meta box, at bottom of list, if present
• change restore button to include revision ID, as in "Restore Revision ID xxx"
• other minor bug fixes - if you still see failures please report
• still need to optimize returned data

[karmatosed]

Changes to make the background and borders more consistent with WP UI

[karmatosed]

I just tested this and one thing crosses my mind is you may expect as a user to be able to slide the slider without clicking it to go to a place. I tested this with cache cleared in both Chrome and Safari and the behaviour is that it requires you to click and then it goes to an area or you use the next / previous.

Update : The above does seem a bug I can get it sliding on one refresh of page but it seems delayed behind the movement so it is a bit disjointed in the sliding of the button across. Maybe we can smooth this /refine?

[karmatosed]

Slider css cleanup

[karmatosed]

Colour contrast adjustments

[karmatosed]

I did a colour contrast check and came up with a patch based on ​http://contrastchecker.com/?swatch_session=lTiOATdvqriX.

​http://core.trac.wordpress.org/attachment/ticket/23497/colours.diff

[helen]

In 23581:

• Simplify jQuery UI slider CSS and bring into line with admin styles.
• Merge styles into wp-admin.css and colors-*.css.
• Scope the CSS with a class so as not to conflict with the color picker, which also utilizes jQuery UI slider. Authors wanting to use built-in styling for sliders should add a class of .wp-slider to the container to be intialized.

props karmatosed, helen. see #23497.

[helen]

Replying to karmatosed:

I did a colour contrast check and came up with a patch based on ​http://contrastchecker.com/?swatch_session=lTiOATdvqriX.

A couple of things for that patch - please also alter colors-classic.css to match, and hex values should be all lowercase, per CSS coding standards.

[karmatosed]

Contrast colours

[karmatosed]

More contrast colours

[karmatosed]

@helen : sure, sorry for the incomplete patch. I've uploaded those now.

​http://core.trac.wordpress.org/attachment/ticket/23497/colors-classic.diff
​http://core.trac.wordpress.org/attachment/ticket/23497/colors-fresh.diff

If you could possibly please look to add ​http://core.trac.wordpress.org/attachment/ticket/23497/bordersbackground.diff I think we're a bit more on track with the styling. Still a few edges but hopefully getting there.

[adamsilverstein]

bug fixes, only compare required diffs

[adamsilverstein]

updates in 23497.15.diff

• more bug fixes
• only compare diffs required, for two handle comparison, skip calculation and return blank for diffs to the right of the right handle (for left handle drags) or to the left of the left handle (for right handle drags). since its impossible to drag the right handle past the left handle or visa versa in the two handle mode, we can skip calculating these diffs

[ethitter]

​ajax-actions.patch uses admin_url() to build the URL for the Restore button, rather than assuming that WP is installed at the server root.

[ethitter]

​compare-two.patch wraps the text "Compare two revisions" in a label, so it can be clicked to manipulate the checkbox.

[helen]

Replying to karmatosed:

colors-classic.css looks like it needs the text-decoration: line-through. colors-fresh.css looks like it got some extra whitespace added somehow.

For the borders one, the colors do look better, although I'd suggest a 1px border. Also, move the color definitions into the color sheets. For borders, that means specifying border-width and border-style in wp-admin.css, and then border-color in each of the color sheets.

And, finally, please create patches from the root of your install; otherwise applying the patch requires the path to be manually defined.

[adamsilverstein]

model load optimizations

[adamsilverstein]

​23497.16.diff when dragging handles in two handled mode, only reload right models when left handle changed, only reload left models when right handle changed.

note: includes all previous updates

[karmatosed]

Background color, border color and contrast patch

[karmatosed]

@helen thank you for taking a look. I've added a new patch that hopefully solves those issues and merges things a bit. If it's not right just let me know but hopefully it is.

[adamsilverstein]

updates:

• asynchronous fetch of revision diffs:
  • initial Collection fetch gets list of revisions with meta-data, including 'needs loading' flag for possible comparisons
  • iterates thru revision models that need loading and fetches diffs for these models
  • even with many revisions, you can now start sliding almost immediately, spinners show until data loads
  • current code fetches all model data asynchronously, one call pre revision
  • revision fetches are throttled to 200ms to avoid hammering the server if there are lots of revisions, consider lazy loading?

• hide 'compare two' option when only two revisions available (possible easter egg location?)
• includes previous code patches, including ethitter's ajax-actions.patch and compare-two.patch corrections

[adamsilverstein]

asynchronous fetching, code patch rollup

[adamsilverstein]

minor JS cleanup

[adamsilverstein]

fix restore issue

[karmatosed]

Following on from the discussion in dev chat, I have done some playing around with UI's. This relates to the markers and the scroll indicator along with tooltips indication. I will continue to explore this tomorrow if time but wanted to get some ideas up and out from the chat as soon as possible.

​http://core.trac.wordpress.org/attachment/ticket/23497/revisions-01.jpg
​http://core.trac.wordpress.org/attachment/ticket/23497/revisions-02.jpg
​http://core.trac.wordpress.org/attachment/ticket/23497/revisions-03.jpg
​http://core.trac.wordpress.org/attachment/ticket/23497/revisions-04.jpg

[westi]

In 23638:

Revisions: Update the Styling for the Revisions UI props karmatosed see #23497.

[westi]

In 23639:

Revisions: Updates to the new Revisions UI.

Various Updates including:

• i18n fixes
• Added tracking of what revision ID was restored
• async fetching of diffs so that slider works sooner even with many revisions

See #23497 props adamsilverstein, ethitter

[karmatosed]

I just did an amendment to the mockup as per discussion today in WordPress dev.

​http://core.trac.wordpress.org/attachment/ticket/23497/revisions-05.jpg

[karmatosed]

Another version based on feedback

[MikeHansenMe]

I have not been following this feature closely but wanted to leave some thoughts from my recent testing. After testing I did read some of the comments on this page which explained the most recent version is on left and oldest on right side of slider. This seems backwards because below you show what was removed(old version) on the left and what was added on the right(new version). I know a lot of work has gone in already just wanted to voice some concerns. Here are some steps and thoughts I had testing.

• Create Page
• Add content "Original Content"
• Update content "Revision 1"
• Under Screen Options check revisions
• Click the bottom revision link
• Page opens, note the sliders handle to the left
• Slide it to the right "Comparing current version to revision 2 of 2" (nothing shown.. confused now "What is the purpose of the slider at this point...")
• Slide it back to the left
• Compare two revisions checkbox
• Slide middle handle to the right (result: same as 7 )
• Slide left handle to middle (left shows nothing, right shows "Original Content")
• Unclick Compare two revisions check box
• Slide from right to left ("Wait... Original content is on right and revision 1 is on left...")
• confused again or still..
• Move slider back to right side
• click compare two revisions again
• Slide the right slider to middle and uncheck Compare two revisions (Cannot compare revision to itself.... Confused again)

[adamsilverstein]

thanks for testing. a few notes:

• the direction is reversed in the (next) patch, that may make more sense. in the new setup, the oldest revisions are on the left, the newest ones on the right. i will post that patch when i have everything else addressed, you might want to test again after that...
• did you do this test using the latest trunk? the code was updated recently and some of this sounds like buggy behavior that was fixed, please try with latest updates to see if you still have the issues; i was not able to reproduce the bug you described creating a new post and revising it -- the revisions screen worked as expected (although with the direction reversed)
• the 'compare two revisions' should not be available if there are fewer than three revisions

here is a screen recording of my test trying to reproduce the error you outline: ​http://cl.ly/NQb0

again, appreciate the testing & feedback.

Replying to MikeHansenMe:

I have not been following this feature closely but wanted to leave some thoughts from my recent testing. After testing I did read some of the comments on this page which explained the most recent version is on left and oldest on right side of slider. This seems backwards because below you show what was removed(old version) on the left and what was added on the right(new version). I know a lot of work has gone in already just wanted to voice some concerns. Here are some steps and thoughts I had testing.

• Create Page
• Add content "Original Content"
• Update content "Revision 1"
• Under Screen Options check revisions
• Click the bottom revision link
• Page opens, note the sliders handle to the left
• Slide it to the right "Comparing current version to revision 2 of 2" (nothing shown.. confused now "What is the purpose of the slider at this point...")
• Slide it back to the left
• Compare two revisions checkbox
• Slide middle handle to the right (result: same as 7 )
• Slide left handle to middle (left shows nothing, right shows "Original Content")
• Unclick Compare two revisions check box
• Slide from right to left ("Wait... Original content is on right and revision 1 is on left...")
• confused again or still..
• Move slider back to right side
• click compare two revisions again
• Slide the right slider to middle and uncheck Compare two revisions (Cannot compare revision to itself.... Confused again)

[bpetty]

Replying to adamsilverstein:

• did you do this test using the latest trunk? the code was updated recently and some of this sounds like buggy behavior that was fixed, please try with latest updates to see if you still have the issues; i was not able to reproduce the bug you described creating a new post and revising it -- the revisions screen worked as expected (although with the direction reversed)

I just did some testing in (completely) up-to-date SVN trunk, and I don't know how you didn't see errors because I spotted *several* problems right away. Both in my own testing, and in your own video.

1. When creating a new post or page (with JS turned on), there are no revisions of the post, so the "Revisions" screen option isn't even available unless you have two revisions. The problem here is that if the second revision is an autosave (from JS), the revisions still don't show up on the edit screen like they do when you publish a second revision since it doesn't actually post the entire page, it's just an AJAX request (this might be an old problem though). This isn't a big deal since compare doesn't work at all with only one non-autosave revision anyway, however, this points me to a second problem...

2. (with JS off) When opening a page or post with one revision and an additional autosave revision, the "Revisions" widget on the edit page lists the revisions (properly), but with links to compare, which end up on a blank compare page with no explanation about what happened, but there's actually two problems here. First, JS was off, so it just doesn't load anything, hence the blank page. Second, if JS is actually on, it *does* load a comparison with only one revision, but you're saying that shouldn't even be possible without at least two non-autosave revisions to compare - but it happens (and in fact, it's actually correct about the revision info, and the comparison diff content for once surprisingly - hey, creation of a page/post is an edit from nothing to something and even Wikipedia and all VCS diff apps will show a diff on that).

3. As hinted to in (B), the diff content is entirely incorrect in several different situations (which I am finding tough to consistently reproduce, however, one bug or another keeps popping up with *every* single test I do, I just don't always know which one is next - so they aren't hard to spot). In one case, with 3 non-autosave revisions each only changing one line, and with only one line of content, I have been able to get the comparison to somehow end up with *two* lines of content from two different revisions on the same side of the diff, one line right after the other. That should definitely never happen. Fix the other bugs mentioned here first, and then it might be easier to reproduce and squash this bug (I'll come back again later to test again I'm sure).

4. I did one test (without autosaves) where I created a post with "one", updated with "two", reverted to "one" (which worked), and updated the post with "four" (since it's the fourth revision), and while in comparison view (no matter how I got there from any revision), if the slider is on any of the revisions, it always showed "four" as being added even though it was only added in the last revision.

• the 'compare two revisions' should not be available if there are fewer than three revisions

This could be a little hard to test since it's tough to ensure an autosave doesn't sneak in right now, maybe just disable JS while testing (and re-enable on compare). That may have happened in Mike's test. In my own testing, I never did see the "compare two revisions" option unless I had at least 3 revisions.

here is a screen recording of my test trying to reproduce the error you outline: ​http://cl.ly/NQb0

There are definitely errors right here in your own video...

5. After you save the "revise" content, you clicked on the "22:03:08" revision, which was the oldest and first revision, however, the comparison page always opens on the latest revision first every time (which in your case was the "22:03:11" revision - from "test" to "revise").

6. The first time you move the slider to the right, it says it's comparing the oldest "22:03:08" revision, which should have been the change from nothing to "test", not from nothing to "revise" - in fact, this removed/added combo *never* happens in any single revision - that's only if you compared two revisions from the non-existent post to the final "revise" post.

7. When you click "restore revision", this is super confusing... what are you restoring? If it was the oldest revision's content, shouldn't your content have been "test" and not an empty post? Why was your content completely empty, but the title wasn't? If you click "restore" with the latest revision selected, doesn't that actually mean that nothing should happen? Shouldn't the restore button be disabled on the latest revision diff?

[adamsilverstein]

Thanks for your testing and detailed feedback. Here are some responses to your specific points:

1. Thanks for catching that issue with the autosaves, not sure the best way to fix as you mentioned its an ajax save and the logic about showing the meta box happens at page load (are there x revisions for this post?).

2. 1. JS off - this is a JS/Backbone rewrite of revisions and no-js has not been addressed at all. not sure what the approach will be, either no support or fallback to old code. 2. you shouldn't even be able to get to the revisions page unless you have at least two revisions

3. that should NOT happen as you mention. if you change one line in each revision, that line change should be visible in each diff. if you can reproduce, please tell me steps.

4. reverting a post may mess with the revisions page, aware of the issue. part of the problem is that currently the most recent revision represents the previous update, not the current post data - except after a restore. we are addressing this issue in #16215 and once that patch rolls i believe the logic will be cleaner.

• from your description though, wouldn't 'four' show in any position? note that the diff view in single handle mode shows what has changed between the current version and the selected revision. if the current version contains the 'four' edit, that will show on the +Added side of the diff for every revision comparison. again: the view shows compare to current, not what happened in the revision (i tried it that way, but it wasn't liked)

5. you are correct that no matter which revision link you click on, the revisions page always loads with the most recent revision selected; this is a known issue and will be addressed in an upcoming patch

6. all comparisons are to the final revised post in one handle mode (currently actually uses post since no revision is available), thats the way the comparisons are currently set up. maybe we need to make that logic more apparent in the interface. also trying reversing the order so newest revisions are on the right

7. Agreed 'Restore Revision' button is super confusing. in my upcoming patch you will see we changed what that button says 'Restore This Revision' and placement- next to the compare to revision data, hopefully making it clearer what happens when you click the button. i think my screencast may display a bug with which revision was restored, i'm looking into that.

really appreciate the feedback and testing, obviously some kinks to work out! i should have another patch on the ticket early next week and could use more testing help if you are willing...

Replying to bpetty:

Replying to adamsilverstein:

• did you do this test using the latest trunk? the code was updated recently and some of this sounds like buggy behavior that was fixed, please try with latest updates to see if you still have the issues; i was not able to reproduce the bug you described creating a new post and revising it -- the revisions screen worked as expected (although with the direction reversed)

I just did some testing in (completely) up-to-date SVN trunk, and I don't know how you didn't see errors because I spotted *several* problems right away. Both in my own testing, and in your own video.

1. When creating a new post or page (with JS turned on), there are no revisions of the post, so the "Revisions" screen option isn't even available unless you have two revisions. The problem here is that if the second revision is an autosave (from JS), the revisions still don't show up on the edit screen like they do when you publish a second revision since it doesn't actually post the entire page, it's just an AJAX request (this might be an old problem though). This isn't a big deal since compare doesn't work at all with only one non-autosave revision anyway, however, this points me to a second problem...

2. (with JS off) When opening a page or post with one revision and an additional autosave revision, the "Revisions" widget on the edit page lists the revisions (properly), but with links to compare, which end up on a blank compare page with no explanation about what happened, but there's actually two problems here. First, JS was off, so it just doesn't load anything, hence the blank page. Second, if JS is actually on, it *does* load a comparison with only one revision, but you're saying that shouldn't even be possible without at least two non-autosave revisions to compare - but it happens (and in fact, it's actually correct about the revision info, and the comparison diff content for once surprisingly - hey, creation of a page/post is an edit from nothing to something and even Wikipedia and all VCS diff apps will show a diff on that).

3. As hinted to in (B), the diff content is entirely incorrect in several different situations (which I am finding tough to consistently reproduce, however, one bug or another keeps popping up with *every* single test I do, I just don't always know which one is next - so they aren't hard to spot). In one case, with 3 non-autosave revisions each only changing one line, and with only one line of content, I have been able to get the comparison to somehow end up with *two* lines of content from two different revisions on the same side of the diff, one line right after the other. That should definitely never happen. Fix the other bugs mentioned here first, and then it might be easier to reproduce and squash this bug (I'll come back again later to test again I'm sure).

4. I did one test (without autosaves) where I created a post with "one", updated with "two", reverted to "one" (which worked), and updated the post with "four" (since it's the fourth revision), and while in comparison view (no matter how I got there from any revision), if the slider is on any of the revisions, it always showed "four" as being added even though it was only added in the last revision.

• the 'compare two revisions' should not be available if there are fewer than three revisions

This could be a little hard to test since it's tough to ensure an autosave doesn't sneak in right now, maybe just disable JS while testing (and re-enable on compare). That may have happened in Mike's test. In my own testing, I never did see the "compare two revisions" option unless I had at least 3 revisions.

here is a screen recording of my test trying to reproduce the error you outline: ​http://cl.ly/NQb0

There are definitely errors right here in your own video...

5. After you save the "revise" content, you clicked on the "22:03:08" revision, which was the oldest and first revision, however, the comparison page always opens on the latest revision first every time (which in your case was the "22:03:11" revision - from "test" to "revise").

6. The first time you move the slider to the right, it says it's comparing the oldest "22:03:08" revision, which should have been the change from nothing to "test", not from nothing to "revise" - in fact, this removed/added combo *never* happens in any single revision - that's only if you compared two revisions from the non-existent post to the final "revise" post.

7. When you click "restore revision", this is super confusing... what are you restoring? If it was the oldest revision's content, shouldn't your content have been "test" and not an empty post? Why was your content completely empty, but the title wasn't? If you click "restore" with the latest revision selected, doesn't that actually mean that nothing should happen? Shouldn't the restore button be disabled on the latest revision diff?

[bpetty]

Replying to adamsilverstein:

• from your description though, wouldn't 'four' show in any position? note that the diff view in single handle mode shows what has changed between the current version and the selected revision. if the current version contains the 'four' edit, that will show on the +Added side of the diff for every revision comparison. again: the view shows compare to current, not what happened in the revision (i tried it that way, but it wasn't liked)

How are you supposed to know what content (in the selected revision) you are actually restoring then without opening "compare two revisions"? And if you do have that open, "restore revision" becomes even more confusing. This seems incredibly unhelpful, and definitely confusing if you come from the old revisions page or Wikipedia diffs. There's no possible way to see what the real changes were in a single revision other than the first revision without opening the "compare two revisions" feature, but I always thought that was one of the primary use-cases for revisions.

MikeHansenMe is right, this is all very confusing UI.

You open revisions and start out on a diff from the latest revision to the next oldest (MediaWiki provides "prev" links rather than making you select both revisions manually for that use case, and I'm guessing they do since it's so frequently used), but from there on out, I would *always* end up checking "compare two revisions", and painstakingly selecting each revision by moving both slider buttons one tick at a time.

Anyway, if the way it works now is how it's going to be, you're right that it needs to be significantly more apparent - like adding the exact revisions (and dates) above each column on Removed/Added.

[adamsilverstein]

i agree, its confusing! my original take on the interface had the comparison always be to the previous revision, showing 'whats changed in this revision. i think this is what you were expecting to see as well.

this idea wasn't well accepted in our revisions team meeting and the consensus was that the diff view should show the difference between the current post data and the viewed revision. i'm not entirely convinced, however - i really liked the mode that showed the change in each revision... one idea would be to enable a 'compare to previous' feature in the single slider handle mode.

once we get the code a little more polished and clear bugs, i would love to see some user testing like the tests of the menu system @lessbloat ran a few weeks ago.

thanks for the feedback and testing, stay tuned here for updates.

Replying to bpetty:

Replying to adamsilverstein:

• from your description though, wouldn't 'four' show in any position? note that the diff view in single handle mode shows what has changed between the current version and the selected revision. if the current version contains the 'four' edit, that will show on the +Added side of the diff for every revision comparison. again: the view shows compare to current, not what happened in the revision (i tried it that way, but it wasn't liked)

How are you supposed to know what content (in the selected revision) you are actually restoring then without opening "compare two revisions"? And if you do have that open, "restore revision" becomes even more confusing. This seems incredibly unhelpful, and definitely confusing if you come from the old revisions page or Wikipedia diffs. There's no possible way to see what the real changes were in a single revision other than the first revision without opening the "compare two revisions" feature, but I always thought that was one of the primary use-cases for revisions.

MikeHansenMe is right, this is all very confusing UI.

You open revisions and start out on a diff from the latest revision to the next oldest (MediaWiki provides "prev" links rather than making you select both revisions manually for that use case, and I'm guessing they do since it's so frequently used), but from there on out, I would *always* end up checking "compare two revisions", and painstakingly selecting each revision by moving both slider buttons one tick at a time.

Anyway, if the way it works now is how it's going to be, you're right that it needs to be significantly more apparent - like adding the exact revisions (and dates) above each column on Removed/Added.

[nacin]

Replying to adamsilverstein:

i agree, its confusing! my original take on the interface had the comparison always be to the previous revision, showing 'whats changed in this revision. i think this is what you were expecting to see as well.

this idea wasn't well accepted in our revisions team meeting and the consensus was that the diff view should show the difference between the current post data and the viewed revision.

Huh? This makes very little sense. Single-handle scrubber mode should show what changed in that particular revision. Comparing to current is very unexpected behavior. If you wanted to compare to current, you have the two-handle scrubber.

[adamsilverstein]

Replying to nacin:

Replying to adamsilverstein:

i agree, its confusing! my original take on the interface had the comparison always be to the previous revision, showing 'whats changed in this revision. i think this is what you were expecting to see as well.

this idea wasn't well accepted in our revisions team meeting and the consensus was that the diff view should show the difference between the current post data and the viewed revision.

Huh? This makes very little sense. Single-handle scrubber mode should show what changed in that particular revision. Comparing to current is very unexpected behavior. If you wanted to compare to current, you have the two-handle scrubber.

ok, glad you agree with the concept of the single handle slider showing whats changed in each revision compared to the previous revision instead of whats changed between each revision and the current version. thats the way i thought it should work and thats the way it was originally coded in my backbone code, but in our meeting ​IRC chat, @helen felt that was confusing to the end user and the consensus seemed to be the comparison should be to the current version...

so, should i just reverse that decision and go back to showing each revision's changes, or should we offer an option? will bring up at tomorrows office hours, appreciate any feedback.

[bpetty]

I understand what @helen was trying to explain, but I think she didn't realize that the compare page could open the "compare two revisions" mode with the current version against the revision selected by default (where applicable), and that's really what she may have meant (maybe you didn't even have that functionality finished yet at the time?).

If the slider only allows selection of one revision, the changes in just that revision are what the user wants to see. If the slider allows selection of two revisions, then we compare the range of changes. As far as the actual UI control logic, that seems entirely clear.

What you have in trunk right now is two different modes where the functionality of one of them (the single revision selection) is already covered by the second mode, and so now it doesn't really provide any advantage over the "compare two revisions" mode. And in the process of doing this, it's now also not possible to easily browse single revision changes. The two modes are there to complement each other with functionality the other mode doesn't provide, right? Otherwise there's not much point in even having the second mode.

@helen, it would be helpful to see the report of the actual survey mentioned, it might help give us some insight into what you mean.

[helen]

Replying to bpetty:

@helen, it would be helpful to see the report of the actual survey mentioned, it might help give us some insight into what you mean.

An Automattician (I think lessbloat, but not 100% sure) had done a quick survey of .com users and shared it in IRC: ​https://docs.google.com/spreadsheet/ccc?key=0Av7nMZXUOf52dDc0QVowVHRXR29QeXk0YXJaX2l3TVE#gid=0

You don't need to speculate on what I do or don't realize. Just ask instead. :)

[bpetty]

Replying to helen:

You don't need to speculate on what I do or don't realize. Just ask instead. :)

I tagged you, and explained what I thought you meant, fully expecting you to reply with a clarification, correction, or (preferably) just an acknowledgement. My experience with open source issue trackers (especially this one) has taught me to never rely on a response though, so I tend to avoid direct questions unless it's the OP in order to maintain progress on a ticket. It doesn't mean I didn't want your input though, I was asking for it.

Since you still don't appear to have any corrections though, I'm assuming you agree then. (see, no direct questions)

[adamsilverstein]

Replying to bpetty:

Replying to helen:

You don't need to speculate on what I do or don't realize. Just ask instead. :)

I tagged you, and explained what I thought you meant, fully expecting you to reply with a clarification, correction, or (preferably) just an acknowledgement. My experience with open source issue trackers (especially this one) has taught me to never rely on a response though, so I tend to avoid direct questions unless it's the OP in order to maintain progress on a ticket. It doesn't mean I didn't want your input though, I was asking for it.

Since you still don't appear to have any corrections though, I'm assuming you agree then. (see, no direct questions)

as i recall that conversation happened before the two handle slider mode was even an idea. now that we have the compare two functionality, i'm going to restore the previous method, actually a relief for me as thats the way i intuitively felt it should operate.

[bpetty]

Looking through the responses on this survey, it's actually interesting to note that a large portion of users (about 25%) looking at revisions are expecting to be able to compare the changes in an autosave from the published version to see what was saved. This actually makes a lot of sense. If someone were to restore an autosave, something interrupted them, and now they have no idea what changes were saved in the autosave, and which weren't. Some responses also indicated that some users thought that revisions could help them compare the changes they've made right there in the editor, regardless of autosave or published revisions (much like how MediaWiki has a "show changes" option from their editor).

Is there really any reason autosave revisions shouldn't also be available to compare with regular revisions (because they aren't right now)? They are a revision with changes, even if they aren't published.

If they were included, the UI would certainly have to be clear (with some kind of indicators in the slider, and in the diffs) about which revision is the current published revision, but otherwise, I don't see any reason why they shouldn't be available for comparison.

[adamsilverstein]

viewing autosaves is a -currently hidden- option - investigating adding as a screen option. i will turn on viewing autosaves by default in next patch.

[adamsilverstein]

changes

• set initial view to display passed revision id. now when you click to view a revision in the list, that revision displays
• set maximum step width; now when there few revisions slider bar is smaller size;
• remove cancel button, exctraneous
• new backbone view for tickmarks, ticks inside slider bar
• tickmarks indicate revision load state while loading (currently spinner background on 1 line, but open to suggestions)
• tickmark width & color based on scope of changes, initial pass, five weights (noted issue with wider ticks appearing left of centered)
• move compare two checkbox to upper right
• removed 'comparing x of y' language above slider
• reversed direction of revision view - oldest on left, newest on right
• html cleanup in revision.php
• move restore button next to compare from meta, change language to 'Resore this Revision'
• retain compare position when switching modes (bug fix)
• changed slider dragger to blue triangle from mock up; used base64 encoded image, think thats best but request suggestions
• turn viewing autosaves on by default
• started work on tooltips but didn't get that far, need help! trying to have tooltips that work like these: ​http://www.filamentgroup.com/examples/slider_v2/index3.php - slider is low to accomidate tooltips; maybe add revision change lines added/deleted info;not convinced we need them, clunky, same info as below

• misc bug fixes
• add check for latest revision matches post data (new methodology in #16215)
• currently counting lines changed, is this useful or should it be words changed
• currently hard coded thresholds determine scope_of_changes calculation, should be scaled to range of values in all revisions

• up next, change compare method to compare to previous

[karmatosed]

Comparing revisions mockup

[adamsilverstein]

note: latest patch compare two mode wonky, correcting; avoid testing for now please...

[adamsilverstein]

fixed compare two mode

[adamsilverstein]

23497.21.diff should be ready for compare one/compare two testing

also:

• single slider mode now shows 'whats changed in this revision' - always compares to previous revision (instead of compare to current
• current revision currently missing from compare two or compare one mode - there is no 'current' revision until we land #16215

[adamsilverstein]

fixed whitespace replaced with tabs in pluggable.php

[MikeHansenMe]

@adamsilverstein I am still seeing strange things after applying patch 21. I have 3 revisions "first" "second" and "third" now when I view compare revision page I cannot see "third" at all I would expect to see this when on the most recent revision. Showing "Second" was removed and "third" was added. Currently I cannot see that. I am going to do some more testing to see if I can find other quirks with the patch. Thanks for providing updated patches.

[adamsilverstein]

wpRevisionsSettings added via wp_localize_script() props ocean90

[adamsilverstein]

Replying to MikeHansenMe:

@adamsilverstein I am still seeing strange things after applying patch 21. I have 3 revisions "first" "second" and "third" now when I view compare revision page I cannot see "third" at all I would expect to see this when on the most recent revision. Showing "Second" was removed and "third" was added. Currently I cannot see that. I am going to do some more testing to see if I can find other quirks with the patch. Thanks for providing updated patches.

thanks for pointing that out. still fine tuning, will have more updates in a bit.

currently core does not store a revision that matches the current state of the post in the database. (addressed in #16215). i was fudging this in my earlier code by inserting a copy of the current post, will look at that again cause we'll need it till the first post update when the data is corrected.

[adamsilverstein]

Replying to adamsilverstein:

Replying to MikeHansenMe:

@adamsilverstein I am still seeing strange things after applying patch 21. I have 3 revisions "first" "second" and "third" now when I view compare revision page I cannot see "third" at all I would expect to see this when on the most recent revision. Showing "Second" was removed and "third" was added. Currently I cannot see that. I am going to do some more testing to see if I can find other quirks with the patch. Thanks for providing updated patches.

thanks for pointing that out. still fine tuning, will have more updates in a bit.

currently core does not store a revision that matches the current state of the post in the database. (addressed in #16215). i was fudging this in my earlier code by inserting a copy of the current post, will look at that again cause we'll need it till the first post update when the data is corrected.

on second thought - i don't think we want to show the current revision in the one handle slider - what would happen if you hit the restore button (nothing)? i guess i could hide the restore button in the that handle position, would that make sense?

in my latest code you do see the current post data, but only in the compare two handle mode - where you want to compare a previous revision to another revision, possibly the current version. you can't restore to the current revision, because restore always restores the right handle.

[bpetty]

Replying to adamsilverstein:

on second thought - i don't think we want to show the current revision in the one handle slider - what would happen if you hit the restore button (nothing)? i guess i could hide the restore button in the that handle position, would that make sense?

Yeah, I thought the button would just be disabled (maybe not hidden though so users know it's there when applicable).

As I mentioned earlier with regard to the survey, it looks like a large portion of users are using revision comparisons to look at the most recent changes in most cases (whether that's autosaves or the current published revision). They probably still want to see what those changes are even if you can't restore the current revision.

[adamsilverstein]

ok, good point. i will add that next. new update fixes most data issues, i hope.

[adamsilverstein]

bug fixes

[adamsilverstein]

• hide slider when only one revision
• show autosaves by default
• fix most outstanding comparison bugs - i hope!

note: tooltips disabled for now

[adamsilverstein]

more consistent switching between two handle and one handle mode refreshed

[SergeyBiryukov]

I guess it's worth noting that wp_post_revision_title() got a backward-incompatible change in [23506].

Previously, it used to return the revision date and time:
​http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/post-template.php#L1294

Now it returns the revision author name first:
​http://core.trac.wordpress.org/browser/trunk/wp-includes/post-template.php?rev=23653#L1319

This creates three issues:

1. The strings in edit-form-advanced.php still expect date and time first:
   ​http://core.trac.wordpress.org/browser/trunk/wp-admin/edit-form-advanced.php?rev=23683#L39 Changing "from" to "by" and updating the translator comment would probably be enough.
2. The "by (author name)" part in the meta box is redundant: 23497.revisions-meta-box.png​.
3. wp_post_revision_title() inline description should be updated.

[SergeyBiryukov]

$revision_title variable in revision.php is unused since [23506]:
​http://core.trac.wordpress.org/browser/trunk/wp-admin/revision.php?rev=23639#L61

[SergeyBiryukov]

Replying to SergeyBiryukov:

Changing "from" to "by" and updating the translator comment would probably be enough.

That still might be incompatible with custom strings though (see "Customizing the messages" example):
​http://codex.wordpress.org/Function_Reference/register_post_type#Example

[adamsilverstein]

thanks for the feedback, question: when you say

The "by (author name)" part in the meta box is redundant:

do you mean the line at the bottom that shows the restore event meta info? this shows who took the action of restoring the revision, not the author of the revision itself. maybe change language will make that clearer? its supposed to show you - who did the restore, because it might not be you!

since we do want to change the display of author info in the revision meta box, i changed the wp_post_revision_title function; however if these changes are going to break something else i could instead provide a new, differently named & updated function. or are the changes minor enough to be acceptable, but just need documenting?

Replying to SergeyBiryukov:

I guess it's worth noting that wp_post_revision_title() got a backward-incompatible change in [23506].

Previously, it used to return the revision date and time:
​http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/post-template.php#L1294

Now it returns the revision author name first:
​http://core.trac.wordpress.org/browser/trunk/wp-includes/post-template.php?rev=23653#L1319

This creates three issues:

1. The strings in edit-form-advanced.php still expect date and time first:
   ​http://core.trac.wordpress.org/browser/trunk/wp-admin/edit-form-advanced.php?rev=23683#L39 Changing "from" to "by" and updating the translator comment would probably be enough.
2. The "by (author name)" part in the meta box is redundant: 23497.revisions-meta-box.png​.
3. wp_post_revision_title() inline description should be updated.

[adamsilverstein]

changes:

• add current revision to right hand end of slider; when in current revision 'Restore This Revision' button hidden
• started refresh of appearance based on ​most recent compare mockups
• fix remaining issues when switching modes, switching left/right/autosaves

[adamsilverstein]

visual updates to match mockups

[adamsilverstein]

23497.27.diff works further towards the ​latest mockups

• black left handle
• remove extra horizontal lines
• align From: To: lines, from 'revision by'

still different:

• button for Restore This Revision stays near To: line, thats whats restored, trying to make that more obvious
• 'compare two revision' option still in upper right, nacin pointed out this option is not related to post meta line or restore button, so doesn't really belong there. need suggestions for placement of this option
• need to decide about two hidden options: show autosaves (on by default but maybe needs to be an option?) and show combined diff view - currently the diff shows removed lines on the left, added lines on the right; current code supports a stacked mode where all changes are viewed in a single column - might work better for a narrow display, also might be easier to understand for some users

the last feature i would consider adding before the feature freeze would be the tooltips when dragging the handles. now that i have everything ironed out (i think!) i will try to revisit that

[adamsilverstein]

initial tooltip implementation

[adamsilverstein]

23497.28.diff​ adds an initial tooltip implementation; some obvious glitches, needs positioning help, etc. but don't want to be adding any new features after now so wanted to get it in.

[adamsilverstein]

23497.29.dif

• correct switching to two handle mode when issue handle in 0 position
• better tooltip positioning
• weighted tick css adjustment

[adamsilverstein]

correct issue when switching to two handles before 1st load completed

[adamsilverstein]

latest version screenshots:

one handle:

​http://cl.ly/NYV3

two handle:

​http://cl.ly/NZMd

many revisions:

​http://cl.ly/NZ4O

[MikeHansenMe]

@adamsilverstein after some additional testing this morning. Patch 30 seems much more stable and predictable. I did run into a new problem when trying to restore a revision of the compare two mode. It seems if the right handle is not on the current revision the message below prevents you from restoring.

"Are you sure you want to do this?

Please try again."

[adamsilverstein]

Replying to MikeHansenMe:

@adamsilverstein after some additional testing this morning. Patch 30 seems much more stable and predictable. I did run into a new problem when trying to restore a revision of the compare two mode. It seems if the right handle is not on the current revision the message below prevents you from restoring.

"Are you sure you want to do this?

Please try again."

great, thanks for testing again - i really appreciate the extra eyeballs. i will try to reproduce and correct the restore issue. can you tell me if you remember the action immediately before clicking the restore button, did you move left or right handle last or was it just after switching from one to two handle mode? i'm also going to try to roll up the other revisions (data related) tickets into one master patch so they can be tested together.

[MikeHansenMe]

I have been able to consistently reproduce this one. I have 3 revisions, "first", "second", and "third". I click compare two revisions and move the right slider from "third" to "second" so it shows "first" on the left and "second" on the right. Then I click Restore this Revision and receive the notice. Let me know if you have any trouble reproducing it and perhaps I can take a screencast.

[adamsilverstein]

corrects invalid restore noone

[adamsilverstein]

Replying to MikeHansenMe:

I have been able to consistently reproduce this one. I have 3 revisions, "first", "second", and "third". I click compare two revisions and move the right slider from "third" to "second" so it shows "first" on the left and "second" on the right. Then I click Restore this Revision and receive the notice. Let me know if you have any trouble reproducing it and perhaps I can take a screencast.

i found the culprit - invalid noonce. my code/model url changes hadn't been reflected in the calculated restore noonce.

23497.31.diff​ should fix the issue you were having, please re-test. tooltip interactions are still a little glitchy, but mostly i want to find reproducible data type errors. let me know if this works!

[adamsilverstein]

combined patch for testing

[adamsilverstein]

in 23497.32.diff​

• includes changes from #20982
• includes changes from #16215
• minor bug fixes

this is a bundled up patch allowing testing for 16215 & 20982. needs lots more testing!

[SergeyBiryukov]

Replying to adamsilverstein:

thanks for the feedback, question: when you say

The "by (author name)" part in the meta box is redundant:

do you mean the line at the bottom that shows the restore event meta info? this shows who took the action of restoring the revision, not the author of the revision itself. maybe change language will make that clearer? its supposed to show you - who did the restore, because it might not be you!

No, I mean the list of revisions, where the author name is mentioned twice for each row.

since we do want to change the display of author info in the revision meta box, i changed the wp_post_revision_title function; however if these changes are going to break something else i could instead provide a new, differently named & updated function. or are the changes minor enough to be acceptable, but just need documenting?

I guess I'll move this to a new ticket: #23783. This one has too much going on already :)

[MikeHansenMe]

Nonce problem is fixed now. Thanks

First find

• after selecting compare two the diffs do not load it shows "No Difference"

Second find

• on compare two screen move right slider to any other revision but the most current
• go back to single view
• the order of revisions is wrong and shows the revision from the right slider(from compare two) as current for single view

Third find

• when on compare two mode
• move right slider to another revision so the restore button appears
• notice it is next to "To" revsion which indicates that I would be restoring what I see on the right side
• however it restores the from version, which I thin is correct but it is not what the button indicates

Otherwise I think this is getting much better

Let me know if you need me to do some more testing.

[adamsilverstein]

update_post_meta moved to wp_restore_post_revision so passes unit test

[adamsilverstein]

updated unit tests for

[adamsilverstein]

in 20982.16215.ut.diff​​ updated unit tests for #20982 and #16215, both pass after applying 23497.33.diff​​

[adamsilverstein]

bug hunting

[adamsilverstein]

in 23497.34.diff​​ -

• fix duplicated user name in revisions meta - removed author name from wp_list_post_revisions() - author name now returned as part of wp_post_revision_title() so was showing duplicated
• remove 'form-table' option and code from post-template.php - this was the old table with radio buttons for comparing revisions that is no longer used
• larger gravatar, clean up vertical alignment on revision list

[adamsilverstein]

in 23497.35.diff​:

• updated tooltip horizontal positioning for chrome, Firefox now off; may need to change targeting/offset method?
• added (back) text for [Current Revision] and [Autosave] in revision meta display (added to ajax-actions.php)
• removed all logging code, clean up stale TODO items

[adamsilverstein]

in 23497.36.diff​​

it was pointed out in #23783 that changes to wp_post_revision_title might cause problems where just date/time are expected. since this new version of the function also includes the gravatar and author display name it might make the most sense to break it out as a separate function, leaving the old function untouched.

• new function wp_post_revision_title_expanded, reverted wp_post_revision_title back to original - addressing #23783 - new function still needs updated phpdoc and translator instructions
• removed unused call to function ​http://core.trac.wordpress.org/browser/trunk/wp-admin/revision.php?rev=23639#L61

[adamsilverstein]

can you please try testing again after the latest patches? i think i've fixed the restore button mismatch you identified and switching issues.

Replying to MikeHansenMe:

Nonce problem is fixed now. Thanks

First find

• after selecting compare two the diffs do not load it shows "No Difference"

Second find

• on compare two screen move right slider to any other revision but the most current
• go back to single view
• the order of revisions is wrong and shows the revision from the right slider(from compare two) as current for single view

Third find

• when on compare two mode
• move right slider to another revision so the restore button appears
• notice it is next to "To" revsion which indicates that I would be restoring what I see on the right side
• however it restores the from version, which I thin is correct but it is not what the button indicates

Otherwise I think this is getting much better

Let me know if you need me to do some more testing.

[adamsilverstein]

A few notes:

• tooltips could use some interaction help, for example staying open when dragging but pointer leaves handle area
• tooltip positioning varies from firefox to chrome, needs fixing, testing in supported browsers
• the new revisions system does not include the easter egg from the previous version, any ideas how to re-insert it?
• the Codex is really outdated, especially when we get to 3.6. help: ​http://codex.wordpress.org/Revision_Management

[MikeHansenMe]

Replying to adamsilverstein:

can you please try testing again after the latest patches? i think i've fixed the restore button mismatch you identified and switching issues.

Happy to do some more testing. I will comment back with my findings.

[MikeHansenMe]

When I first click compare two revisions
left side loads (empty), right side loads "first" after sliding the left handle over 1 and back it then shows "first" on the left and "third" on the right. Which seems to be correct. it just does not load correctly initially.

The restore button seems to be restoring the version I expect.

In compare two mode if I leave the right slider on "second" and switch back to single mode the revision on the right always seems to be correct but as I move the slider the left never changes and remains empty no matter where the slider is.

[adamsilverstein]

thanks again for the testing...

on the 1st part sounds like the compare two mode is not loading correctly until you start sliding. i will try to reproduce that and see whats going on. i think i've seen that, will check. left side empty sounds like a revision with only title, eg the initial autosave; sounds like thats missing once you start dragging... maybe the wrong left handle model is being loaded. if you then drag the right hand (move it and stop to reload the left handle models), what happens when you drag the left again, does it show blank or 'first' in the 0 position?

some of what you said in the second part makes me think you might be having a common confusion about the diff display. the display always shows what was removed on the left and what was added on the right. it is not showing one revision on each side! A merged mode where the removed added and unchanged lines are all in one column is implemented but currently hidden.

also, to further clarify:

in single handle mode the display shows what has "changed in this revision" - that is, what was added or removed since the previous revision.

in compare two mode you are comparing 'whats changed between these two revisions'. dragging the left handle compares agains the right handle position, dragging the right handle compares agains the left handle position. compares are always newer agains older. clicking restore always restores the right handle position. (at least, thats the way its supposed to work!)

with that in mind, do you still see the issues in part two?

if you were confused about the diff display, thats only natural and demonstrates that the UI needs more explanatory text and visual cues that explain how it works.

[MikeHansenMe]

Replying to adamsilverstein:

thanks again for the testing...

on the 1st part sounds like the compare two mode is not loading correctly until you start sliding. i will try to reproduce that and see whats going on. i think i've seen that, will check. left side empty sounds like a revision with only title, eg the initial autosave; sounds like thats missing once you start dragging... maybe the wrong left handle model is being loaded. if you then drag the right hand (move it and stop to reload the left handle models), what happens when you drag the left again, does it show blank or 'first' in the 0 position?

It shows (empty) initially then "first" after moving the slider back to the first location. So yes it seems to not be loading correctly initially.

some of what you said in the second part makes me think you might be having a common confusion about the diff display. the display always shows what was removed on the left and what was added on the right. it is not showing one revision on each side! A merged mode where the removed added and unchanged lines are all in one column is implemented but currently hidden.

Sorry bad wording on my part. I understand that it is one revision and shows what was removed/added. I would expect it to restore what is show on the right especially since the button is next to "To" however previously it restored the "From" side. This is now working correctly.

also, to further clarify:

in single handle mode the display shows what has "changed in this revision" - that is, what was added or removed since the previous revision.

Initially it shows "first" on the left and "second" on the right which would be correct I replaced the text "first" with "second" after going to compare two mode and back it will show "second" on the right but nothing on the left when it should show "first" because that is what was removed when adding "second" same behavior on "third" as well.

in compare two mode you are comparing 'whats changed between these two revisions'. dragging the left handle compares agains the right handle position, dragging the right handle compares agains the left handle position. compares are always newer agains older. clicking restore always restores the right handle position. (at least, thats the way its supposed to work!)

This does work now, as I mentioned before it previously did restore what was shown on the left even though the button was by "To".

with that in mind, do you still see the issues in part two?

if you were confused about the diff display, thats only natural and demonstrates that the UI needs more explanatory text and visual cues that explain how it works.

The biggest problem I see currently (patch 36) is that after switching from compare two back to single mode those revisions are also out of place. Also the incorrect initial load of compare two.

Let me know if that clarifies your questions. Sometimes I am not a clear as I try to be.

[adamsilverstein]

thanks, you were perfectly clear! i will work on this issue and notify when i have it fixed. getting there!

Replying to MikeHansenMe:

Replying to adamsilverstein:

thanks again for the testing...

on the 1st part sounds like the compare two mode is not loading correctly until you start sliding. i will try to reproduce that and see whats going on. i think i've seen that, will check. left side empty sounds like a revision with only title, eg the initial autosave; sounds like thats missing once you start dragging... maybe the wrong left handle model is being loaded. if you then drag the right hand (move it and stop to reload the left handle models), what happens when you drag the left again, does it show blank or 'first' in the 0 position?

It shows (empty) initially then "first" after moving the slider back to the first location. So yes it seems to not be loading correctly initially.

some of what you said in the second part makes me think you might be having a common confusion about the diff display. the display always shows what was removed on the left and what was added on the right. it is not showing one revision on each side! A merged mode where the removed added and unchanged lines are all in one column is implemented but currently hidden.

Sorry bad wording on my part. I understand that it is one revision and shows what was removed/added. I would expect it to restore what is show on the right especially since the button is next to "To" however previously it restored the "From" side. This is now working correctly.

also, to further clarify:

in single handle mode the display shows what has "changed in this revision" - that is, what was added or removed since the previous revision.

Initially it shows "first" on the left and "second" on the right which would be correct I replaced the text "first" with "second" after going to compare two mode and back it will show "second" on the right but nothing on the left when it should show "first" because that is what was removed when adding "second" same behavior on "third" as well.

in compare two mode you are comparing 'whats changed between these two revisions'. dragging the left handle compares agains the right handle position, dragging the right handle compares agains the left handle position. compares are always newer agains older. clicking restore always restores the right handle position. (at least, thats the way its supposed to work!)

This does work now, as I mentioned before it previously did restore what was shown on the left even though the button was by "To".

with that in mind, do you still see the issues in part two?

if you were confused about the diff display, thats only natural and demonstrates that the UI needs more explanatory text and visual cues that explain how it works.

The biggest problem I see currently (patch 36) is that after switching from compare two back to single mode those revisions are also out of place. Also the incorrect initial load of compare two.

Let me know if that clarifies your questions. Sometimes I am not a clear as I try to be.

[MikeHansenMe]

Yes, getting closer. Let me know if you need more testing.

Replying to adamsilverstein:

thanks, you were perfectly clear! i will work on this issue and notify when i have it fixed. getting there!

[adamsilverstein]

bug hunting!

[adamsilverstein]

in 23497.37.diff​

• diff against trunk, includes intersecting changes to date format changeset 23743
• fixes several bugs when switching modes, as described by @MikeHansenMe

[adamsilverstein]

Replying to MikeHansenMe:

Yes, getting closer. Let me know if you need more testing.

Replying to adamsilverstein:

thanks, you were perfectly clear! i will work on this issue and notify when i have it fixed. getting there!

ready for another test drive, sir!

[MikeHansenMe]

Initially when I load compare two mode the left side is (empty) and the right side has "third" after sliding the left slider to the center it shows "second" on the left and "third" on the right. When I slide the left handle back to the left it shows "first". So something is not loading correctly initially or maybe (empty) is correct and we are losing it after sliding.

When switching back to single view after leaving the right handle on "second" in compare two mode. It retains the correct info. This seems to be working correctly now.

I think the first problem is a decision, do we include the empty one as the first or do we start on the "first" save. In single view the empty post always shows first. In compare two it shows initially but "first" is missing until you slide and "first" replaces empty.

[adamsilverstein]

mmmm - thought i had that fixed! the initial load is wrong because in the leftmost position it should show first, not blank. the 1st 'title only' revision should be ignored, so the 1st revision should show 'first' - as it does when you start dragging the left slider (in one handle mode you do see blank as removed in the left most compare position, thats what changed when you added your first revision).

sounds like the initial load when you switched to compare two mode was off. i will try to reproduce and fix, does it matter what position the handle is in when you switch to two handled mode?

glad to hear switching back to one handle mode works correctly now.

i will fix the 1st issue and report back here, thanks for the tireless testing.

Replying to MikeHansenMe:

Initially when I load compare two mode the left side is (empty) and the right side has "third" after sliding the left slider to the center it shows "second" on the left and "third" on the right. When I slide the left handle back to the left it shows "first". So something is not loading correctly initially or maybe (empty) is correct and we are losing it after sliding.

When switching back to single view after leaving the right handle on "second" in compare two mode. It retains the correct info. This seems to be working correctly now.

I think the first problem is a decision, do we include the empty one as the first or do we start on the "first" save. In single view the empty post always shows first. In compare two it shows initially but "first" is missing until you slide and "first" replaces empty.

[MikeHansenMe]

Replying to adamsilverstein:

i will fix the 1st issue and report back here, thanks for the tireless testing.

No problem, thanks for the tireless patching.

[adamsilverstein]

fixes default left handle position 1 not 0

[adamsilverstein]

23497.38.diff​​ should fix the issue you discovered. the left handle had a default value of 0 that should have been 1. now that i changed that, the compare two mode should load with 'first' on the left, not blank.

please re-test! any more issues?

Replying to MikeHansenMe:

Initially when I load compare two mode the left side is (empty) and the right side has "third" after sliding the left slider to the center it shows "second" on the left and "third" on the right. When I slide the left handle back to the left it shows "first". So something is not loading correctly initially or maybe (empty) is correct and we are losing it after sliding.

When switching back to single view after leaving the right handle on "second" in compare two mode. It retains the correct info. This seems to be working correctly now.

I think the first problem is a decision, do we include the empty one as the first or do we start on the "first" save. In single view the empty post always shows first. In compare two it shows initially but "first" is missing until you slide and "first" replaces empty.

[MikeHansenMe]

The previous bugs mentioned all seem to be fixed. I will do more though testing to see if I can find any new/missed bugs.

[adamsilverstein]

Replying to MikeHansenMe:

The previous bugs mentioned all seem to be fixed. I will do more though testing to see if I can find any new/missed bugs.

woo-hoo!

if you have any old posts laying around, see what happens when you test them - don't update them first or you will 'fix' their data. old posts are missing a current version revision, but the code _should_ accomodate that.

[westi]

In 23769:

Revisions: UI Update.

• Refines the UI to make it clearer and easier to use
• Introduces weighted tickmarks
• Fixes comparison bugs.

See #23497 props adamsilverstein

[ryan]

Another pluggable function? We've been trying to avoid adding new ones, ever.

[adamsilverstein]

Replying to ryan:

Another pluggable function? We've been trying to avoid adding new ones, ever.

assume you are talking about the new 'wp_text_diff_with_count' function that is just like wp_text_diff, but keeps track of changed lines while calculating the diffs. i created a new function because i was worried changing the original function would break something unexpectedly.

possible fixes:

• move the function elsewhere, so its not in in pluggable.php
• change the original function instead so no new function is added

which is the preferable correction? let me know and i will provide a patch.

[markjaquith]

I'd move it elsewhere. Non-pluggable.

[adamsilverstein]

Replying to markjaquith:

I'd move it elsewhere. Non-pluggable.

will move to wp-includes/revision.php

[adamsilverstein]

in 23497.39.diff​:

• cleaned up wp_first_revision_matches_current_version - tabs not spaces, not sure how those made it in...
• moved wp_text_diff_with_count from pluggable.php to revision.php, no more pluggable functions! (thanks @ryan)
• restores _post_restored_from functionality; this was left out of 23769 because it was unrelated to UI, keeps track of last restore action; already displayed in the bottom of the revision meta box list when present:

[koopersmith]

revisions.js is quite messy and could use some polish for both style and optimization. I'll try to run through and clean it up sometime soon.

Please drop a note if you're actively working on a patch that touches revisions.js.

[markjaquith]

In 23811:

Minor revisions PHP reorg, code cleanup, restores _post_restored_from functionality.

props adamsilverstein. see #23497

[markjaquith]

In 23813:

*Very* rough runthough of revisions.js for JS style, variable names, etc.

Consider this the lawnmower that precedes Daryl's scissors.

see #23497

[adamsilverstein]

thanks for the beautification, also very informative for me - sorry to distract you with all that tedious work.

Replying to markjaquith:

In 23813:

*Very* rough runthough of revisions.js for JS style, variable names, etc.

Consider this the lawnmower that precedes Daryl's scissors.

see #23497

[adamsilverstein]

Replying to koopersmith:

revisions.js is quite messy and could use some polish for both style and optimization. I'll try to run through and clean it up sometime soon.

Please drop a note if you're actively working on a patch that touches revisions.js.

thanks in advance.

[adamsilverstein]

notes:

• placemnt of ticks, when heavier than 2 pixels wide, is off center to the left
• placement of tooltip varies by browser (firefox/chrome different)
• while dragging, tooltip should stay open even when pointer leaves handle area and stay with handle, but go away on mouse up
• 'compare two revision' option in upper right seems off
• color contrast/needs accessibility review, better readability on diff view
• need a sneaky way to re-introduce the easter egg
• maximum number of revisions on slider?

[adamsilverstein]

corrects tooltip/meta mismatch

[markjaquith]

In 23831:

correct a revision/tooltip mismatch.

props adamsilverstein. see #23497

[adamsilverstein]

bug fixes

[markjaquith]

In 23832:

Make sure that the ID of the right revision is not less than the ID of the left revision

see #23497

[markjaquith]

In 23834:

Kill the HR on revisions page. see #23497

[adamsilverstein]

i am testing again and noticed with only one revision the single slider mode is off. looking at the db, i see that we no longer seem to be storing a blank content/only title initial revision. this revision was actually useful, because it was used in the comparison to show the content of the 1st actual content revision. without it i would need to fudge the code to add a blank initial revision to compare to for the display.

also, i am concerned about what happens when we have some revisions with the data and some without it. might just need to add the hook back that was removed in #16215

[adamsilverstein]

corrects missing initial revision for comparison, present in all former data

[adamsilverstein]

the latest commit from 16215 introduced an issue where the initial revisions with title only/blank content were not being created. while this sounds nice, it messed up the revisions UI which was expecting that initial revision. since its present in all former data i think its fine to leave in place, 23497.42.diff​ fixes.

[azaozz]

Replying to adamsilverstein:

the latest commit from 16215 introduced an issue where the initial revisions with title only/blank content were not being created.

Thinking that when a post was published from QuickPress or PressThis, there is no such revision either so some existing posts will not have them. These revisions were only created from the New Post screen and (as far as I remember) were discussed as not needed/bugs at some point.

Edit: confirmed, these revisions are a result from the first autosave that we run manually when the user clicks from the post title to the editor (that's why the content is empty). So posts created not from the New Post screen won't have them.

If we have to keep making these empty revisions, the return of add_action( 'pre_post_update', 'wp_save_post_revision' ) in 23497.42.diff would need to make sure this runs only the very first time a past is saved. This also means we will have a minimum of 3 rows in the DB for each post, whether it's ever edited or not.

[markjaquith]

Moved it to wp_insert_post. Similar, but runs every time, not just on updates. Don't want to lose that first version of the post.

[markjaquith]

Let's move on to individual tickets for new issues. This ticket is kinda insane now.

[nacin]

Mind if I re-open this for some things I think can be cleaned up? Here are some class/ID names from the revisions template:

• diff_from_current_revision
• difftitlefrom
• diff-left-hand-meta-row
• tmpl-revisionvinteract

These should be consistently hyphens. But more so, it seems like there are more elements, classes, and IDs than are necessary. Most/all IDs should be classes, some classes may be redundant and can be replaced with simpler selectors, etc.

[SergeyBiryukov]

Replying to nacin:

Here are some class/ID names from the revisions template

Also mentioned in ticket:23897:2.

[ocean90]

Mind if I re-open this for some things I think can be cleaned up?

Sorry, but please open a new one. I doesn't want to break my fingers while scrolling this long ticket. ;)