WordPress.org

Make WordPress Core

#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 11 months ago.
mark diff as complete that don't need loading
24346.2.diff (6.7 KB) - added by adamsilverstein 11 months ago.
loading enhancements, bug fixes, cleanup
24346.3.diff (8.6 KB) - added by adamsilverstein 11 months ago.
more rtl trinary explicit parantheses
24346.4.diff (6.7 KB) - added by adamsilverstein 11 months ago.

Download all attachments as: .zip

Change History (17)

adamsilverstein11 months ago

mark diff as complete that don't need loading

comment:1 adamsilverstein11 months ago

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

comment:2 adamsilverstein11 months ago

  • Keywords has-patch added

comment:3 ocean9011 months ago

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

comment:4 follow-up: ocean9011 months ago

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

comment:5 in reply to: ↑ 4 ; follow-up: adamsilverstein11 months 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.

comment:6 in reply to: ↑ 5 ; follow-ups: ocean9011 months 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.

comment:7 in reply to: ↑ 6 adamsilverstein11 months 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.

adamsilverstein11 months ago

loading enhancements, bug fixes, cleanup

adamsilverstein11 months ago

more rtl trinary explicit parantheses

comment:8 adamsilverstein11 months 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

comment:9 in reply to: ↑ 6 adamsilverstein11 months 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

comment:10 follow-ups: ocean9011 months 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' ).

comment:11 in reply to: ↑ 10 adamsilverstein11 months 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;

adamsilverstein11 months ago

comment:12 in reply to: ↑ 10 adamsilverstein11 months 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

comment:13 ocean9010 months ago

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