Change how textdiff settings changes are handled #1284
Conversation
| this.isShowingDiffs.subscribe(newValue => { | ||
| if (newValue) this.render(); | ||
| }); | ||
| this.textDiffType.value.subscribe(() => { | ||
| if (this.isShowingDiffs()) this.render(); |
There was a problem hiding this comment.
Does a change in type no longer invalidate the diffs?
There was a problem hiding this comment.
No, because it isn't needed.
The diff type setting and "load more" are client side features and don't require server requests.
The whitespace setting on the other hand is a git feature and requires a server request.
components/textdiff/textdiff.js
Outdated
| block.lines = block.allLines.slice(0, remaining); | ||
| } else if (block.allLines) { | ||
| block.lines = block.allLines; | ||
| block.allLines = undefined; |
There was a problem hiding this comment.
at block level, is there a reason to have allLines separately?
We could loosely follow the line count restrictions. if limit is 10 and we first block has 12 lines of code, we could just display all 12 lines without doing this.
Not sure if it is worth to follow that diff line limit strictly
There was a problem hiding this comment.
It was there before (we implemented it like this 5 years ago 7bcd34c). I just changed it to enable "loading more" without invalidating the diff (reverting and really fixing #876).
But yes, beeing strict has also some bugs like this one where the whitespace changes are only visible after loading the hole block:
But one code block could also be huge like hundreds of lines and defeating the purpose of #376...
There was a problem hiding this comment.
For sure there are some tech debts here. If I remember correctly, the main cause of inefficiencies targeted in #376 was due to animation loop moving many elements and triggering recalculations that was blocked by animation loop due to large diff box opened up, which ultimately addressed in #630. Performance part of the incremental load shouldn't be a consideration any more.
However, if we are concerned by unexpectedly adding large block. We could do conservative approach and load only that is below the limit.
There was a problem hiding this comment.
I have changed the logic to always load complete blocks as long as the load limit is not reached.
campersau
force-pushed
the
textdiffsettings
branch
from
9985efd
to
df774ef
Compare
Fixes performance issues with
signaland should use less memory.This is the PR for one of my proposals from #1091 (comment)