Revisions cleanup

[markjaquith]

I'm looking over Revisions with Nacin and Koop. We think it needs a general cleanup, on the JS side, and for the Ajax endpoint. Tracking that on this ticket. This might run into some other tickets, but I didn't want to hijack them:

#24056
#24346
#24388

[adamsilverstein]

track mouse when dragging handles, snap to step when stopping

[adamsilverstein]

24425.5.diff​​

• handle now follows mouse position when dragging, then snaps back to selected step when dragging stopped/mouseup
• positioning tested in firefox, needs browser testing especially IE (just a guess)
• removed step: 1, thats the default
• added animate: true, this should enable clicking to slide the handles once tick code is removed
• refresh may be too much, but does put handle(s) back where they belong

[adamsilverstein]

24425.draft.3.diff​ looks good, loads immediately even with many revisions. happy to test when ready.

[adamsilverstein]

thanks for all the work reworking the revisions code.

i've noticed a few missing items and am listing them here as a point of reference, hope this is helpful:

• restore this revision button
• next/previous buttons
• clicked revision not selected on initial load
• compare two mode
• tick marks missing, simple tick marks might help visually (repeating background css?)
• tooltips
• revision meta info
• RTL mode (current revision should be in rightmost position when using RTL language)

newer code thats missing:

• mouse dragging code 24425.5.diff​
• pagination support, see #24056 - not sure we need this?

remaining:

Compare two mode in RTL

[markjaquith]

Make sure fetch() doesn't remove models from the diffs collection

[markjaquith]

Some chunked loading.

[adamsilverstein]

set slider handle to intiially selected/passed revision

[adamsilverstein]

improved find the initially selected revision

[adamsilverstein]

adds previous/next buttons and functionality back

[adamsilverstein]

includes missing file

[adamsilverstein]

adds more features back, author meta

[adamsilverstein]

restore button functionality

[adamsilverstein]

remove nested object template kludge

[adamsilverstein]

adds back mouse tracking for handle while dragging

[adamsilverstein]

fixes extraneous ajax calls when using next/previous

[markjaquith]

In 24520:

Cleanup of the revisions screen, both on the PHP API side, and the JS.

• Much simpler PHP API
• Cleaner and more Backbone-y JS API
• Consequently, does batch queries; this now scales up to hundreds of revisions

Currently missing, but much easier considering the cleaned up base:

• Compare two mode
• RTL

props koopersmith, nacin, adamsilverstein, ocean90. see #24425

[adamsilverstein]

completes Rtl functionality, diff against current trunk

[ocean90]

In 24529:

Revisions UI: Fix table colgroup (Say what?) for post titles when they haven't been changed. see #24425.

[adamsilverstein]

adds compare two mode, history tracking

[adamsilverstein]

24425.draft.18.diff​​

• reintroduces compare two revisions mode
• adds backbone history (although this slows down interactions slightly, not sure its worth keeping) comment out line 'Backbone.history.start();' to see the difference, does enable browser back/forward buttons
• compare two mode untested in Rtl mode, likely needs work

notes:

• mouse dragging/tracking disabled in compare two mode
• next/previous button interaction needs cleanup
• next/previous buttons need to be hidden in compare two mode

[adamsilverstein]

hide next/previous buttons in compare two mode

[adamsilverstein]

after linting and cleanup

[adamsilverstein]

adds tooltip when hovering over slider bar

[adamsilverstein]

corrects tooltip placement

[adamsilverstein]

prop to attr/sanity check for tooltip calculated revision index, fixes console error

[adamsilverstein]

address chrome flickering z-index related issue

[adamsilverstein]

as per ocean90: replace $( '.wp-slider.ui-slider' ).mousemove() with this.$el.mousemove(), same for $( '.wp-slider.ui-slider' ).hover()

[ocean90]

Slider: Handle mouse events via Backbone events

[ocean90]

Moves Tooltip, Checkbox and Buttons to its own view

[ocean90]

Little cleanup

[adamsilverstein]

numerous fixes

[adamsilverstein]

in 24425.draft.29.diff​
Corrected:

• Corrects tooltip is misplaced when in compare-2-mode
• Removed bottom border from .revisions-controls div
• Removed markup for tick marks
• Remove markup for the spinner
• Moved if ( this.model.revisions.length < 3 ) to checkbox view
• remove $( '.compare-two-revisions' ).trigger( 'click' ); instead rely on route actions only (started)
• Tooltip message moved into model for easier translation
• Switched to new View for tooltip

Still Remaining:

• RTL mode 2 handles
• Improve getSliderPosition(), use more. (Idea: return an array with left and right, if single only left is filled with a value)
• Restore the .pre class for the diff table
• ignore revision id from URL, instead use #route/to/revision_id

Bugs

• Compare-2-mode: Right handler on the first revisions, left on the last: Expected: See all lines in green color, since it's a diff from the first to the current one seems to work fine; 1st revision isn't blank (except when comparing from in single handle mode)
• Router: If reloading a page it loses the handle positions (currently being messes with by passed revision id)
• duplicate firing on next/previous

[ocean90]

Removes debug cruft

[markjaquith]

Calling this one a blocker until it's RC-able.

[ocean90]

In 24549:

Revisions UI update:

• Restore Compare two mode
• Restore tooltips
• RTL support
• First pass for URL routing and history

props adamsilverstein, see #24425.

[markjaquith]

Debounces the URL updates

[markjaquith]

24425.history-debounce.diff​

The URL updating on mouse drag was massively slowing things down. This patch makes a debounced function (250ms currently), and then only calls that from a custom event that fires after the diff has rendered (diff rendering has its own throttling built in, so this will filter out diffId changes that happen so quickly that by the time they start to render, the diffId has already changed).

Also a little bit of cleanup, removing unnecessary variables, and moving the Router to get passed in a model, instead of the set-it-after thing that was happening.

[markjaquith]

In 24556:

Debounce the Revisions UI URL updates as you scrub, for a massive speedup and a reduction in URL history detritus.

see #24425

[adamsilverstein]

nice fixes, noticed that issue as well. should have other items addressed soon. thanks!

[adamsilverstein]

24425.draft.33.diff​​

• change variable name from sliderHovering to showTooltip
• consolidate handle routing with nifty code from ocean90: this.model.set( { compareTwoMode: '2' === handles } );
• corrects routing behavior, reload works correctly as well original link

[adamsilverstein]

routing fixes, other minor changes

[ocean90]

Tooltip model and remove mouse dragging code

[markjaquith]

In 24565:

Revisions improvements

• Consolidated router code
• Corrected routing behavior
• Tooltip model
• Removed mouse dragging code

props adamsilverstein, ocean90. see #24425.

[ocean90]

CSS cleanup

[ocean90]

In 24574:

Revisions: CSS cleanup, see #24425.

[adamsilverstein]

Replying to ocean90:

In 24574:

Revisions: CSS cleanup, see #24425.

nice cleanup, great to see all those lines removed!

[ocean90]

In 24575:

Revisions improvements

Revert mouse dragging code which was accidentally removed in [24565].

see #24425.

[adamsilverstein]

two handled rtl, bug fixes

[adamsilverstein]

in 24425.draft.37.diff​:

• correct tooltip placement in compare two mode
• enable two handled mode in RTL
• fixes issue with routes not loading correctly
• other minor bug fixes

[ocean90]

24425.draft.38.diff​

• 24425.draft.37.diff​ +
• Simplifies CSS code for tooltips
• Removes jquery-ui-tooltip as dependency
• Adds (no title) if no title exists, props nacin

[ocean90]

tooltip + no title

[ocean90]

In 24578:

Revisions improvements

• Corrected routing behavior
• Simplified CSS for tooltips
• Compare two mode for RTL
• Support for posts without titles

props adamsilverstein, ocean90. see #24425.

[adamsilverstein]

enables smooth handle dragging in two handle mode

[adamsilverstein]

Replying to ocean90:

In 24578:

Revisions improvements

• Corrected routing behavior
• Simplified CSS for tooltips
• Compare two mode for RTL
• Support for posts without titles

props adamsilverstein, ocean90. see #24425.

nice alternate time zone tag team patching! think we are getting close to rc ready here.

[ocean90]

In 24579:

Revisions improvements

• Enable smooth sliding in Compare two mode

props adamsilverstein. see #24425.

[ocean90]

An idea for the revision title and l18n

[markjaquith]

1. In Chrome, I'm getting some avatar flicker as I scrub, even though all the revisions have the same avatar. I'm guessing something related to us rendering too quickly? Any way we can recycle author avatar DOM elements so we're not creating them each time and waiting for Chrome to render?
2. We're not doing any pre-loading in compare-two mode. Which I think is okay, given the multiplicative effect of compare two mode. But when you drag the scrubber really quickly, it creates a flurry of individual requests, while you'll only be seeing the last one, the one the scrubber stopped on. I suggest that we debounce this and consolidate the requests into one. That is, if you flick your finger and generate on-demand requests for 1:3, 1:4, 1:5, 1:6 instead of being 4 separate requests, they should be gathered into one request.
3. When doing an on-demand request, we should fade/remove the current view and show some sort of loading indicator. Currently it displays the current info which means there is a scrubber/view mismatch.

[adamsilverstein]

24425.draft.41.diff​​

• adds simple tickmarks behind slider
• includes bugfix lines from ocean90

next: make tooltips show only when near tickmark spot

[adamsilverstein]

adds simple tickmarks

[adamsilverstein]

only show tooltip when hovering over/near tickmark

[adamsilverstein]

stick tooltips to tickmarks

[adamsilverstein]

alternate version, show tooltip when hovering, but stick to tickmark locations

[adamsilverstein]

center arrow under tooltip

[adamsilverstein]

places arrow below tooltip to right in RTL mode

[markjaquith]

In 24595:

Tick marks are back for revisions

• Simple tick marks
• Tooltips snap to tick marks
• Tooltips arrow reverses side for LTR/RTL
• Fix for routing issue where just the hash is changed in the URL bar relating to compare two mode

see #24425. props adamsilverstein, ocean90.

[adamsilverstein]

center arrow under tooltip (against trunk)

[markjaquith]

What I'd like for on-demand loading (say, in compare-two mode, or for fast scrubbers in single mode):

A way to call ensure (a flag?) that says "I can wait for this". If that flag is set, requests are queued and then a debouncer coalesces them and passes them to load all together. It'll have to be based on $.Deferred, because ensure has to return a promise. Since we don't have a "collecting" debouncer, I was thinking about just maintaining a queue object with keys of diff IDs and values of a deferred object (they can all point to the same one). Then we can just have a dummy debouncer that just serves as a triggering device. When it goes off, we grab the queued ones, clear the queue, add them to the request, then iterate their deferreds, setting them to resolve when the load deferred does.

[koopersmith]

In 24600:

Revisions: Remove redundant method, see #24425.

[koopersmith]

In 24601:

Revisions: Use toggleClass instead of a conditional. See #24425.

[koopersmith]

In 24602:

Revisions: Remove redundant tagNames. Props markjaquith, see #24425.

[koopersmith]

In 24603:

Revisions: A healthy dose of pinking shears. See #24425.

[koopersmith]

In 24604:

Revisions: Remove unused template. See #24425.

[koopersmith]

In 24605:

Revisions: Reduce points where IDs are cast in JS. See #24425.

[koopersmith]

In 24606:

Revisions: Simplify how URLs are updated. See #24425.

[koopersmith]

In 24607:

Revisions: Remove unnecessary router property. See #24425.

[koopersmith]

In 24608:

Revisions: Simplify router property name. See #24425.

[koopersmith]

In 24609:

Revisions: Move data management into the frame state. See #24425.

This helps to solidify the separation between data and UI. We track the diffId internally now, as the property itself was never referenced, and was always derived from the 'from' and 'to' models. This also sets us up for better request debouncing and diff management.

[koopersmith]

In 24611:

Revisions: Debounce fetching Diffs when revisions are updated.

Fixes navigation and casting bugs.

See #24425.

[duck_]

[24520] made the comparator for the Revisions collection to be the revision's ID. This is incorrect behaviour since autosaves are overwritten. This means that an autosave may have a lower ID than some revisions, but contain newer content.

Instead the comparator should be date based, e.g.

comparator: function( a, b ) {
    var a_ = new Date(a.get("date")),
        b_ = new Date(b.get("date"));
    var cmp = (a_ > b_) - (a_ < b_);
    if (cmp == 0 && a.id != b.id)
        cmp = a.id < b.id ? -1 : 1;                                                                                                                                    
    return cmp;
},

Issues with this quick hack are that it doesn't compare down to the second so falling back on the ID may not be enough in some circumstances, it should be tested for cross-browser equality of date parsing (the date property contains an "@" character), and it's not very clear. Maybe it would be better to have a unix timestamp in the model for comparision.

[a.hoereth]

1) "To" string empty when initially opening revision.php
2) "Restore This Revision" not disabled for right most post, restoring this does not change any post content because right most revision is always equal to post.

The "Restore This Revision" button is sometimes disabled when changing back to right most revision, see second half: ​http://www.screenr.com/jSgH

[markjaquith]

In 24613:

Move the loading of surrounding diffs functionality into the model.

• wp.revisions.log() — temporary measure for logging based on wp.revisions.debug
• Return promises from functions that sometimes bail but normally return promises.

See #24425.

[markjaquith]

In 24615:

Pass back dateUnix (a unix timestamp) for better sorting of revisions in Backbone.

Props duck_. See #24425.

[adamsilverstein]

interesting bug possibly related to load sequence, if you are hovering over the slider while loading/reloading the tooltip is visible to the left, with no data. ​http://cl.ly/Q8YJ

as soon as you move the mouse the issue goes away.

[markjaquith]

In 24622:

Don't call updateDiff() in the Revisions FrameState model.

• Stuff needs to hook in, so firing it early results in a half-rendered screen.
• updateDiff() now returns a promise.
• Now, in the frame view's render(), it calls updateDiff(), relying on its promise.

See #24425.

[markjaquith]

In 24627:

Revisions: Update the compare mode before rendering subviews.

See #24425.

[markjaquith]

In 24629:

Revisions: Get rid of an unneccessary ID. IDs are a bad idea.

See #24425.

[adamsilverstein]

latest looks great, loading issues are resolved except tooltip still showing blank if mouse stationary over slider when loading. also, we still need to address item 3 above: ​http://core.trac.wordpress.org/ticket/24425#comment:27

[markjaquith]

In 24636:

Revisions: remove some redundant code.

• Move some slider code into the slider view that was loitering in the checkmark view.
• this.$(), not $().
• Cache a selector.
• toggleClass(), not addClass() with a ternary.
• Remove two classes with a single removeClass().

See #24425.

[markjaquith]

In 24638:

Revisions: simpler hash URLs. Misc refactoring.

• Single mode: #at/:to
• Compare two mode: #from/:from/to/:to
• Make use of _.isUndefined()

See #24425.

[markjaquith]

Done for the night. As adamsilverstein mentioned, we still need to hide the diff and do a loading indicator whenever an on-demand fetch from the server results in a delay.

[duck_]

[24627] caused the previous/next buttons to not be present on initial load. They appear again if you toggle the checkbox. The problem that the compareTwoMode attribute doesn't have a default value, so the call to get() returns undefined. toggleClass() will then add the comparing-two-revisions class since it was not passed a boolean.

Adding a default of false in the FrameState model fixed this for me. It looks like it should always start in single comparison mode, but I wasn't entirely sure.

[markjaquith]

In 24646:

Revisions: fixes for routing and initial setting of compareTwoMode

• Fix routing handler logic (compareTwoMode was reversed).
• Fix routing handler typo (both should use the same handler).
• Set compareTwoMode to false, to start. props duck_.

See #24425.

[ethitter]

The "time ago" displayed when comparing revisions doesn't properly account for the user's chosen timezone offset. The revisions metabox on the edit screen does, however.

In my case, my timezone is set to Eastern, but the "x hours ago" text when comparing revisions includes the difference from GMT.

Comparison: ​http://share.ethitter.com/f/18/
Metabox: ​http://share.ethitter.com/f/19/

[markjaquith]

24425.slider.diff refactors the slider such that the sizing isn't done in JS, but CSS, so there's no resize lag. Widths are zone with percentages, instead. Still needs some tweaking to get the alignment pixel-perfect, or at least only off by one, in some cases.

[markjaquith]

In 24658:

Revisions: Bunch of refactoring and code cleanup

• Extracted a lot of model-y stuff from the view code.
• Slider now has a proper model, with communication with other models.
• Use of get( foo ) instead of findWhere({ id: foo }).
• Properly set the from diff when routing single mode.
• Bind prev() and next() to their model.
• Tick marks are now percentage based, which means the slider resizes with the browser, without JS resize events.
• When scrubbing, the position of the scrubber is considered a hover, so you can fall off the timeline while still scrubbing and the tooltips will persist.
• Tooltips fade in and out.
• Tooltips hang around for a grace period instead of immediately going away. More forgiving.
• Unused code paths removed.
• Got rid of a bunch of view-to-view communication.
• Use _.times() instead of a for loop.
• Removed premature Math.floor() and Math.ceil() calls that were making things not add up to 100%.

See #24425.

[markjaquith]

So, there that is. There are a few minor issues there. Not quite getting pixel-perfect alignment, especially with slider handles. Will have to investigate.

Still need loading indicator, and a fix for the bug ethitter spotted.

[markjaquith]

Also, when scrubbing the "from" handle in compare-two mode, need to set the hoverRevision based on the "from" handle, not the "to" handle.

[markjaquith]

In 24660:

Revisions: Use jQuery animation.

The CSS3 animations caused issues because the element was still there, and there is no standard way to know when a CSS3 animation is done.

See #24425.

[markjaquith]

In 24661:

Revisions: Immediately do a disabled button check on ready().

Solves the issue of Prev/Next buttons not being disabled on initial load, when routing to an extremity.

See #24425.

[nacin]

In 24663:

Compare GMT to GMT. see #24425.

[markjaquith]

In 24664:

Revisions: real URLs and preloading of the requested diff.

• Real URLs are being used now, using pushState. ?revision={id} or ?from={from}&to={to}.
• Drop the redundant action=edit from the URLs (this is the default).
• The initial comparison is preloaded, whether a single revision or a compare-two situation.

See #24425.

[markjaquith]

In 24666:

Revisions: use reject(), not fail(), when rejecting a deferred.

See #24425.

[markjaquith]

In 24668:

Revisions: sort by date, not modified, to match revisions meta box.

See #24425.

[markjaquith]

In 24671:

Revisions: Cleanup, bug fixes, refactoring, polish.

• Hide the tooltip initially.
• Fix a bug with routing.
• Further separate the Slider model and view, refactoring its code.
• More reliance on events than direct calls between areas.
• Smarter background diff loading (single mode). Loads the diffs closest to your position first.
• Removed a bunch of manual templating and render() methods. Now relies more on the WP Backbone Views functionality.
• include the requested id in ensure:load.
• new trigger: ensure, for ensure() attempts, regardless of whether they are already loaded.
• pass along a promise in both ensure and ensure:load.
• in ensure, remove requests for diffs we aready have

See #24425.

[markjaquith]

What remaining issues are people seeing? The one thing I haven't been able to kill yet is the not-quite-pixel-perfect alignment of the sliders and tooltips. Anything else?

[markjaquith]

In 24673:

Revisions: Use all the vendor prefixes for box-sizing.

See #24425.

[markjaquith]

In 24675:

Revisions: Tweak the slider image and alignment.

See #24425.

[markjaquith]

In 24677:

Revisions: pixel perfect tooltips (as far as I can tell), and better checkbox alignment in Firefox.

See #24425.

[markjaquith]

In 24678:

Revisions: Have the tooltip follow the "from" scrubber handle when that is the one being moved, in compare-two mode.

See #24425.

[markjaquith]

In 24679:

Revisions: Code and whitespace cleanup.

See #24425.

[markjaquith]

Okay, I'm calling it. I think we can move on to individual tickets for bugs at this point. Thanks to everyone involved for all their efforts!

[adamsilverstein]

Replying to markjaquith:

24425.slider.diff refactors the slider such that the sizing isn't done in JS, but CSS, so there's no resize lag. Widths are zone with percentages, instead. Still needs some tweaking to get the alignment pixel-perfect, or at least only off by one, in some cases.

much better this way...

[adamsilverstein]

Replying to markjaquith:

Okay, I'm calling it. I think we can move on to individual tickets for bugs at this point. Thanks to everyone involved for all their efforts!

great final sprint work! will test and let you know if I catch anything.

[markjaquith]

In 24686:

Revisions: more graceful tooltips

• Use hoverIntent to prevent inadvertent display and provide more forgiving exploration.
• Nice interruptable fading animation.
• Subtle CSS transition when moving the tooltip side-to-side.

Fixes #24734. See #24425.

[markjaquith]

In 24688:

Revisions: force a scroll bar so that everything doesn't jerk around as you scrub due to the scroll bar popping in and out.

See #24425.

[markjaquith]

In 24699:

Turn off debug mode in revisions.

See #24425.