Update activecode locked and highlighted region markings on scroll#1190
Conversation
|
Testing advice: Add about 80 lines (blank is fine) between the marked areas, scroll up and down. Currently, the markings disappear when they scroll far enough offscreen. |
|
Not seeing them disappear in Safari, but I'll take your word for it. |
There was a problem hiding this comment.
Pull request overview
This pull request updates the ActiveCode CodeMirror integration so locked-region markers and highlighted lines are re-applied as the editor scrolls, with the intent to only decorate currently rendered/visible lines for better performance and visual consistency.
Changes:
- Added a CodeMirror
scrolllistener to re-run locked/highlight decoration logic as the viewport changes. - Updated
setHighlightLines()to apply highlighting relative todisplay.viewFrom/viewTo. - Refactored locked gutter marker placement into
placeLock()and adjusted locked-line decoration to target the current view range.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| editor.display.scrollbars.vert.style.bottom = "16px"; | ||
| }); | ||
| }); | ||
| // Update line makers on scroll. Codemirror only renders a few lines offscreen |
There was a problem hiding this comment.
Typo in the new comment: “line makers” should be “line markers” (and “CodeMirror” capitalization would match the rest of the file).
| // Update line makers on scroll. Codemirror only renders a few lines offscreen | |
| // Update line markers on scroll. CodeMirror only renders a few lines offscreen |
| CodeMirror.on(editor, "scroll", (cm) => { | ||
| window.requestAnimationFrame(() => { | ||
| this.setLockedRegions(); | ||
| this.setHighlightLines(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The scroll handler schedules a new requestAnimationFrame for every scroll event; during continuous scrolling this can queue many redundant callbacks in the same frame. Consider debouncing with a stored RAF id (schedule once until it runs) and only re-decorate when the rendered view range actually changed.
| window.requestAnimationFrame(() => { | ||
| this.setLockedRegions(); | ||
| this.setHighlightLines(); | ||
| }); |
There was a problem hiding this comment.
Calling setLockedRegions() on every scroll re-runs markText() for the prefix/suffix each time. CodeMirror creates a new TextMarker per call, so this will accumulate markers and can cause memory/perf issues. Suggest: create/readOnly markers once (store the returned markers and skip if already present, or clear/recreate explicitly) and keep the per-scroll work limited to updating the visible-line decorations/gutter markers.
| let lineNum = line.split("-"); | ||
| if (lineNum.length > 1) { | ||
| for (let i = parseInt(lineNum[0]); i <= parseInt(lineNum[1]); i++) { | ||
| let startLine = parseInt(lineNum[0]) - this.editor.display.viewFrom; | ||
| let endLine = parseInt(lineNum[1]) - this.editor.display.viewFrom; | ||
| for (let i = startLine; i <= endLine; i++) { | ||
| lines[i - 1].classList.add("CodeMirror__highlight-line"); | ||
| } |
There was a problem hiding this comment.
Range highlighting can compute startLine/endLine outside the current rendered view (e.g., when the highlighted range is offscreen). The loop then calls lines[i - 1].classList on undefined for negative/too-large indices, which will throw. Clamp the range to the visible window (e.g., max(viewFrom+1, start) .. min(viewTo, end)) and guard against missing DOM nodes before adding the class.
| @@ -509,7 +530,11 @@ export class ActiveCode extends RunestoneBase { | |||
| this.editor.doc.getValue().length - this.visibleSuffixLength; | |||
| let endPos = this.editor.posFromIndex(endIndex); | |||
| let lastLine = this.editor.doc.lastLine(); | |||
| decorateLines.call(this, endPos.line, lastLine); | |||
| placeLock.call(this, Math.floor((endPos.line + lastLine) / 2)); | |||
|
|
|||
| let startLine = Math.max(endPos.line - startViewRange, 0); | |||
| let endLine = Math.min(lastLine - startViewRange, endViewRange); | |||
| decorateLines.call(this, startLine, endLine); | |||
There was a problem hiding this comment.
Visible-range math for locked-line decoration mixes absolute doc line numbers (viewTo) with DOM-relative indices (0..lines.length-1). For example endLine uses Math.min(lastLine - viewFrom, viewTo) which can greatly exceed the rendered line count when scrolled, causing large unnecessary loops each scroll and risking incorrect decoration bounds. Compute the max index using (viewTo - viewFrom) (and confirm whether viewTo is exclusive), then clamp start/end to [0, renderedCount-1].
ascholerChemeketa
force-pushed
the
activecode-lock-display-improvement
branch
from
9897e2c to
3d57405
Compare
|
Went back to the drawing board and had ChatGPT help we figure out a better way to address the slow down issue that the custom line decoration was avoiding. The CodeMirror built in methods for adding/removing classes are pretty inefficient - they completely remove the DOM element for lines and replace them with new ones. But, as long as they are batched using So this force push now has a refactor of that logic to simplify things. No more manual decorations and trying to update on scroll. |
This pull request improves the handling and rendering of locked and highlighted lines in the
ActiveCodeinteractive editor, especially when scrolling or when only part of the code is visible. The main focus is to ensure that decorations for locked and highlighted lines are applied correctly to only the visible lines, enhancing performance and visual consistency.Editor rendering and decoration improvements:
setHighlightLinesto only highlight lines that are currently visible in the editor, using theviewFromandviewToproperties for accurate line offsets.Locked line marker placement and decoration:
placeLockfunction, and updated its usage to place markers at the correct line numbers.