WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#24346 closed defect (bug) (invalid)

Revisions reloading all comparisons in two handle mode instead of just available comparisons

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

Description

as ahoereth pointed out, see screencast, all comparisons were reloading after moving one handle; instead, only possible positions for the opposite handle need to be reloaded.

Attachments (4)

24346.diff (941 bytes) - added by adamsilverstein 6 years ago.
mark diff as complete that don't need loading
24346.2.diff (6.7 KB) - added by adamsilverstein 6 years ago.
loading enhancements, bug fixes, cleanup
24346.3.diff (8.6 KB) - added by adamsilverstein 6 years ago.
more rtl trinary explicit parantheses
24346.4.diff (6.7 KB) - added by adamsilverstein 6 years ago.

Download all attachments as: .zip

Change History (17)

@adamsilverstein
6 years ago

mark diff as complete that don't need loading

#1 @adamsilverstein
6 years ago

24346.diff corrects the loading issue, limiting diff loads to possible comparisons

#2 @adamsilverstein
6 years ago

  • Keywords has-patch added

#3 @ocean90
6 years ago

  • Milestone changed from Awaiting Review to 3.6
  • Version set to trunk

#4 follow-up: @ocean90
6 years ago

Looks good, but the ticks should be still visibile, nor? Currently they just have the scope-of-changes-none class.

#5 in reply to: ↑ 4 ; follow-up: @adamsilverstein
6 years ago

Replying to ocean90:

Looks good, but the ticks should be still visibile, nor? Currently they just have the scope-of-changes-none class.

none style should still be visible, right? none still shows a line on my screen, maybe needs to be darker?

ideally the ones showing 'none' would instead show the other model collection scope value which - the other collection would include a comparison.

#6 in reply to: ↑ 5 ; follow-ups: @ocean90
6 years ago

Replying to adamsilverstein:

ideally the ones showing 'none' would instead show the other model collection scope value which - the other collection would include a comparison.

That's my point. Instead of the 1px line it should get the same width as previously.

#7 in reply to: ↑ 6 @adamsilverstein
6 years ago

Replying to ocean90:

Replying to adamsilverstein:

ideally the ones showing 'none' would instead show the other model collection scope value which - the other collection would include a comparison.

That's my point. Instead of the 1px line it should get the same width as previously.

ah, i see. i think i know how i can fix this, will post a patch.

@adamsilverstein
6 years ago

loading enhancements, bug fixes, cleanup

@adamsilverstein
6 years ago

more rtl trinary explicit parantheses

#8 @adamsilverstein
6 years ago

in 24346.3.diff​:

  • load scopeOfChanges from other handle for tick marks that handle can reach in two handle mode when reloading models, now all ticks have a scope of changes
  • corrects missing parentheses bug introduced by rtl code causing l/r models to always reload- reload shouldn't happen if handle starts and stops in the same spot - nothing has changed.
  • added parentheses in several other places where isRtl trinary operator could be misinterpreted
  • removed delay/setTimeout functionality - just adding overhead/complexity - browsers already queue/limit simultaneous requests, timeout was just a guess anyway

#9 in reply to: ↑ 6 @adamsilverstein
6 years ago

Replying to ocean90:

Replying to adamsilverstein:

ideally the ones showing 'none' would instead show the other model collection scope value which - the other collection would include a comparison.

That's my point. Instead of the 1px line it should get the same width as previously.

latest patch fixes that by using scopeOfChanges from other model set

#10 follow-ups: @ocean90
6 years ago

Can you remove the parentheses from lines like Diff.leftDiffStart = ( isRtl ? ui.values[1] : ui.values[0] );?

Maybe change also ! isRtl ? $( '#next' ) : $( '#previous' ) to isRtl ? $( '#previous' ) : $( '#next' ).

#11 in reply to: ↑ 10 @adamsilverstein
6 years ago

Replying to ocean90:

Can you remove the parentheses from lines like Diff.leftDiffStart = ( isRtl ? ui.values[1] : ui.values[0] );?

Maybe change also ! isRtl ? $( '#next' ) : $( '#previous' ) to isRtl ? $( '#previous' ) : $( '#next' ).

like the 2nd suggestion better - i will do that; added parentheses because original code was ambiguous and actually wasn't doing what you would expect;

#12 in reply to: ↑ 10 @adamsilverstein
6 years ago

Replying to ocean90:

Can you remove the parentheses from lines like Diff.leftDiffStart = ( isRtl ? ui.values[1] : ui.values[0] );?

Maybe change also ! isRtl ? $( '#next' ) : $( '#previous' ) to isRtl ? $( '#previous' ) : $( '#next' ).

added 24346.4.diff​ - trinary always positive so logic clear

#13 @ocean90
6 years ago

  • Milestone 3.6 deleted
  • Resolution set to invalid
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.