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

[adamsilverstein]

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.

[adamsilverstein]

mark diff as complete that don't need loading

[adamsilverstein]

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

[ocean90]

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

[adamsilverstein]

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.

[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.

[adamsilverstein]

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]

loading enhancements, bug fixes, cleanup

[adamsilverstein]

more rtl trinary explicit parantheses

[adamsilverstein]

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

[adamsilverstein]

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

[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' ).

[adamsilverstein]

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;

[adamsilverstein]

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

[ocean90]

[24520] / #24425