diff --git a/app/assets/javascripts/code_listing.ts b/app/assets/javascripts/code_listing.ts index 0e0e71c412..aaaca43403 100644 --- a/app/assets/javascripts/code_listing.ts +++ b/app/assets/javascripts/code_listing.ts @@ -9,7 +9,7 @@ import "components/annotations/annotation_options"; import "components/annotations/annotations_count_badge"; import { annotationState } from "state/Annotations"; import { exerciseState } from "state/Exercises"; -import { triggerSelectionEnd } from "components/annotations/select"; +import { triggerSelectionEnd } from "components/annotations/selectionHelpers"; const MARKING_CLASS = "marked"; diff --git a/app/assets/javascripts/components/annotations/annotation_marker.ts b/app/assets/javascripts/components/annotations/annotation_marker.ts index 293f5445c1..3d88d6d541 100644 --- a/app/assets/javascripts/components/annotations/annotation_marker.ts +++ b/app/assets/javascripts/components/annotations/annotation_marker.ts @@ -1,15 +1,17 @@ import { customElement, property } from "lit/decorators.js"; -import { render, html, LitElement, TemplateResult } from "lit"; -import tippy, { Instance as Tippy, createSingleton } from "tippy.js"; -import { AnnotationData, annotationState, compareAnnotationOrders, isUserAnnotation } from "state/Annotations"; -import { StateController } from "state/state_system/StateController"; -import { createDelayer } from "util.js"; - -const setInstancesDelayer = createDelayer(); +import { html, LitElement, TemplateResult } from "lit"; +import { + AnnotationData, + annotationState, + compareAnnotationOrders, + isUserAnnotation +} from "state/Annotations"; +import { MachineAnnotationData } from "state/MachineAnnotations"; /** - * A marker that styles the slotted content and shows a tooltip with annotations. + * A marker that styles the slotted content based on the relevant annotations. + * It applies a background color to user annotations and a wavy underline to machine annotations. * - * @prop {AnnotationData[]} annotations The annotations to show in the tooltip. + * @prop {AnnotationData[]} annotations The annotations to use for styling. * * @element d-annotation-marker */ @@ -18,9 +20,6 @@ export class AnnotationMarker extends LitElement { @property({ type: Array }) annotations: AnnotationData[]; - state = new StateController(this); - - static colors = { "error": "var(--error-color, red)", "warning": "var(--warning-color, yellow)", @@ -33,87 +32,23 @@ export class AnnotationMarker extends LitElement { static getStyle(annotation: AnnotationData): string { if (["error", "warning", "info"].includes(annotation.type)) { - return `text-decoration: wavy underline ${AnnotationMarker.colors[annotation.type]} 1px;`; + return ` + text-decoration: wavy underline ${AnnotationMarker.colors[annotation.type]} 1px; + text-decoration-skip-ink: none; + `; } else { return ` background: ${AnnotationMarker.colors[annotation.type]}; - padding-top: 2px; - padding-bottom: 2px; - margin-top: -2px; - margin-bottom: -2px; `; } } - static tippyInstances: Tippy[] = []; - // Using a singleton to avoid multiple tooltips being open at the same time. - static tippySingleton = createSingleton([], { - placement: "bottom-start", - interactive: true, - interactiveDebounce: 25, - delay: [500, 25], - offset: [-10, -2], - // This transition fixes a bug where overlap with the previous tooltip was taken into account when positioning - moveTransition: "transform 0.001s ease-out", - appendTo: () => document.querySelector(".code-table"), - }); - static updateSingletonInstances(): void { - setInstancesDelayer(() => this.tippySingleton.setInstances(this.tippyInstances), 100); - } - static registerTippyInstance(instance: Tippy): void { - this.tippyInstances.push(instance); - this.updateSingletonInstances(); - } - static unregisterTippyInstance(instance: Tippy): void { - this.tippyInstances = this.tippyInstances.filter(i => i !== instance); - this.updateSingletonInstances(); - } - - // Annotations that are displayed inline should show up as tooltips. - get hiddenAnnotations(): AnnotationData[] { - return this.annotations.filter(a => !annotationState.isVisible(a)).sort(compareAnnotationOrders); - } - - tippyInstance: Tippy; - - renderTooltip(): void { - if (this.tippyInstance) { - AnnotationMarker.unregisterTippyInstance(this.tippyInstance); - this.tippyInstance.destroy(); - this.tippyInstance = undefined; - } - - if (this.hiddenAnnotations.length === 0) { - return; - } - - const tooltip = document.createElement("div"); - tooltip.classList.add("marker-tooltip"); - render(this.hiddenAnnotations.map(a => isUserAnnotation(a) ? - html`` : - html``), tooltip); - - this.tippyInstance = tippy(this, { - content: tooltip, - }); - AnnotationMarker.registerTippyInstance(this.tippyInstance); - } - - disconnectedCallback(): void { - super.disconnectedCallback(); - if (this.tippyInstance) { - AnnotationMarker.unregisterTippyInstance(this.tippyInstance); - this.tippyInstance.destroy(); - this.tippyInstance = undefined; - } - } - get sortedAnnotations(): AnnotationData[] { return this.annotations.sort( compareAnnotationOrders ); } get machineAnnotationMarkerSVG(): TemplateResult | undefined { - const firstMachineAnnotation = this.sortedAnnotations.find(a => !isUserAnnotation(a)); + const firstMachineAnnotation = this.sortedAnnotations.find(a => !isUserAnnotation(a)) as MachineAnnotationData | undefined; const size = 14; return firstMachineAnnotation && html` @@ -125,13 +60,11 @@ export class AnnotationMarker extends LitElement { } render(): TemplateResult { - this.renderTooltip(); - return html`${this.machineAnnotationMarkerSVG}`; + :host { + position: relative; + ${this.annotationStyles} + } + ${this.machineAnnotationMarkerSVG}`; } } diff --git a/app/assets/javascripts/components/annotations/annotation_tooltip.ts b/app/assets/javascripts/components/annotations/annotation_tooltip.ts new file mode 100644 index 0000000000..e97d3f0a5c --- /dev/null +++ b/app/assets/javascripts/components/annotations/annotation_tooltip.ts @@ -0,0 +1,108 @@ +import { customElement, property } from "lit/decorators.js"; +import { render, html, LitElement, TemplateResult, css } from "lit"; +import tippy, { Instance as Tippy, createSingleton } from "tippy.js"; +import { AnnotationData, annotationState, compareAnnotationOrders, isUserAnnotation } from "state/Annotations"; +import { StateController } from "state/state_system/StateController"; +import { createDelayer } from "util.js"; + +const setInstancesDelayer = createDelayer(); +/** + * Adds tooltips with annotations to slotted content + * + * @prop {AnnotationData[]} annotations The annotations to show in the tooltip. + * + * @element d-annotation-tooltip + */ +@customElement("d-annotation-tooltip") +export class AnnotationTooltip extends LitElement { + @property({ type: Array }) + annotations: AnnotationData[]; + + static styles = css`:host { position: relative; }`; + + state = new StateController(this); + + // we need to keep track of all tippy instances to update the singleton + static tippyInstances: Tippy[] = []; + // Using a singleton to avoid multiple tooltips being open at the same time. + static tippySingleton = createSingleton([], { + placement: "bottom-start", + interactive: true, + interactiveDebounce: 25, + delay: [500, 25], + offset: [-10, 0], + // This transition fixes a bug where overlap with the previous tooltip was taken into account when positioning + moveTransition: "transform 0.001s ease-out", + appendTo: () => document.querySelector(".code-table"), + }); + + /** + * Updates the tippy singleton with the current list of tippy instances. + * This method is debounced to avoid updating the singleton too often as it is expensive. + */ + static updateSingletonInstances(): void { + setInstancesDelayer(() => this.tippySingleton.setInstances(this.tippyInstances), 100); + } + + /** + * Adds a tippy instance to the list of instances, which will be used to update the singleton. + */ + static registerTippyInstance(instance: Tippy): void { + this.tippyInstances.push(instance); + this.updateSingletonInstances(); + } + + /** + * Removes a tippy instance from the list of instances, which will be used to update the singleton. + */ + static unregisterTippyInstance(instance: Tippy): void { + this.tippyInstances = this.tippyInstances.filter(i => i !== instance); + this.updateSingletonInstances(); + } + + // Annotations that are not displayed inline should show up as tooltips. + get hiddenAnnotations(): AnnotationData[] { + return this.annotations.filter(a => !annotationState.isVisible(a)).sort(compareAnnotationOrders); + } + + tippyInstance: Tippy; + + disconnectedCallback(): void { + super.disconnectedCallback(); + // before destroying this element, we need to clean up the tippy instance + // and make sure it is removed from the singleton + if (this.tippyInstance) { + AnnotationTooltip.unregisterTippyInstance(this.tippyInstance); + this.tippyInstance.destroy(); + this.tippyInstance = undefined; + } + } + + render(): TemplateResult { + // Clean up the previous tippy instance if it exists. + if (this.tippyInstance) { + AnnotationTooltip.unregisterTippyInstance(this.tippyInstance); + this.tippyInstance.destroy(); + this.tippyInstance = undefined; + } + + if (this.hiddenAnnotations.length > 0) { + const tooltip = document.createElement("div"); + tooltip.classList.add("marker-tooltip"); + render(this.hiddenAnnotations.map(a => isUserAnnotation(a) ? + html` + ` : + html` + `), tooltip); + + this.tippyInstance = tippy(this, { + content: tooltip, + }); + AnnotationTooltip.registerTippyInstance(this.tippyInstance); + } + + // if slot is empty, render an empty svg to make sure the tooltip is positioned correctly + return html` + `; + } +} diff --git a/app/assets/javascripts/components/annotations/code_listing_row.ts b/app/assets/javascripts/components/annotations/code_listing_row.ts index 622f0150d2..9a99ee8c73 100644 --- a/app/assets/javascripts/components/annotations/code_listing_row.ts +++ b/app/assets/javascripts/components/annotations/code_listing_row.ts @@ -1,22 +1,17 @@ import { ShadowlessLitElement } from "components/meta/shadowless_lit_element"; import { customElement, property } from "lit/decorators.js"; import { html, TemplateResult } from "lit"; -import { unsafeHTML } from "lit/directives/unsafe-html.js"; import "components/annotations/annotations_cell"; -import "components/annotations/annotation_marker"; import "components/annotations/hidden_annotations_dot"; import { i18nMixin } from "components/meta/i18n_mixin"; import { initTooltips } from "util.js"; import { PropertyValues } from "@lit/reactive-element"; import { userState } from "state/Users"; -import { AnnotationData, annotationState, compareAnnotationOrders } from "state/Annotations"; -import { MachineAnnotationData, machineAnnotationState } from "state/MachineAnnotations"; -import { wrapRangesInHtml, range, wrapRangesInHtmlCached } from "mark"; -import { SelectedRange, UserAnnotationData, userAnnotationState } from "state/UserAnnotations"; -import { AnnotationMarker } from "components/annotations/annotation_marker"; -import "components/annotations/selection_marker"; +import { annotationState } from "state/Annotations"; +import { userAnnotationState } from "state/UserAnnotations"; import "components/annotations/create_annotation_button"; -import { triggerSelectionStart } from "components/annotations/select"; +import { triggerSelectionStart } from "components/annotations/selectionHelpers"; +import "components/annotations/line_of_code"; /** * This component represents a row in the code listing. @@ -35,60 +30,6 @@ export class CodeListingRow extends i18nMixin(ShadowlessLitElement) { @property({ type: String, attribute: "rendered-code" }) renderedCode: string; - - /** - * Calculates the range of the code that is covered by the given annotation. - * If the annotation spans multiple lines, the range will be the whole line unless this is the first or last line. - * In that case, the range will be the part of the line that is covered by the annotation. - * @param annotation The annotation to calculate the range for. - */ - getRangeFromAnnotation(annotation: AnnotationData | SelectedRange): range { - const isMachineAnnotation = ["error", "warning", "info"].includes((annotation as AnnotationData).type); - const rowsLength = annotation.rows ?? 1; - let lastRow = annotation.row ? annotation.row + rowsLength : 0; - let firstRow = annotation.row ? annotation.row + 1 : 0; - - if (!isMachineAnnotation) { - // rows on user annotations are 1-based, so we need to subtract 1 - firstRow -= 1; - lastRow -= 1; - } - - let start = 0; - if (this.row === firstRow) { - start = annotation.column || 0; - } - - let length = Infinity; - if (this.row === lastRow) { - if (annotation.column !== undefined && annotation.column !== null) { - const defaultLength = isMachineAnnotation ? 0 : Infinity; - length = annotation.columns || defaultLength; - } - } - - return { start: start, length: length, data: annotation }; - } - - get wrappedCode(): string { - const annotationsToMark = [...this.userAnnotationsToMark, ...this.machineAnnotationsToMark].sort(compareAnnotationOrders); - const codeToMark = this.renderedCode; - let annotationsMarked = wrapRangesInHtmlCached( - codeToMark, - annotationsToMark.map(a => this.getRangeFromAnnotation(a)), - "d-annotation-marker", - (node: AnnotationMarker, range) => { - // these nodes will be recompiled to html, so we need to store the data in a json string - const annotations = JSON.parse(node.getAttribute("annotations")) || []; - annotations.push(range.data); - node.setAttribute("annotations", JSON.stringify(annotations)); - }); - if ( userAnnotationState.formShown && this.shouldMarkSelection ) { - annotationsMarked = wrapRangesInHtml(annotationsMarked, [this.getRangeFromAnnotation(userAnnotationState.selectedRange)], "d-selection-marker"); - } - return annotationsMarked; - } - firstUpdated(_changedProperties: PropertyValues): void { super.firstUpdated(_changedProperties); initTooltips(this); @@ -99,20 +40,6 @@ export class CodeListingRow extends i18nMixin(ShadowlessLitElement) { return userState.hasPermission("annotation.create"); } - get machineAnnotationsToMark(): MachineAnnotationData[] { - return machineAnnotationState.byMarkedLine.get(this.row) || []; - } - - get userAnnotationsToMark(): UserAnnotationData[] { - return userAnnotationState.rootIdsByMarkedLine.get(this.row)?.map(i => userAnnotationState.byId.get(i)) || []; - } - - get shouldMarkSelection(): boolean { - return userAnnotationState.selectedRange && - userAnnotationState.selectedRange.row <= this.row && - userAnnotationState.selectedRange.row + (userAnnotationState.selectedRange.rows ?? 1) > this.row; - } - get formShown(): boolean { const range = userAnnotationState.selectedRange; return userAnnotationState.formShown && range && range.row + range.rows - 1 === this.row; @@ -123,20 +50,6 @@ export class CodeListingRow extends i18nMixin(ShadowlessLitElement) { userAnnotationState.selectedRange = undefined; } - get fullLineAnnotations(): UserAnnotationData[] { - return this.userAnnotationsToMark - .filter(a => !a.column&& !a.columns) - .sort(compareAnnotationOrders); - } - - get hasFullLineSelection(): boolean { - return this.shouldMarkSelection && !userAnnotationState.selectedRange.column && !userAnnotationState.selectedRange.columns; - } - - get codeLineClass(): string { - return this.hasFullLineSelection ? `code-line-${annotationState.isQuestionMode ? "question" : "annotation"}` : ""; - } - dragEnter(e: DragEvent): void { if (userAnnotationState.dragStartRow === null) { return; @@ -164,13 +77,7 @@ export class CodeListingRow extends i18nMixin(ShadowlessLitElement) { ${this.row} - ${this.fullLineAnnotations.length > 0 ? html` - - ${unsafeHTML(this.wrappedCode)} - - ` : html` - ${unsafeHTML(this.wrappedCode)} - `} + this.closeForm()} diff --git a/app/assets/javascripts/components/annotations/line_of_code.ts b/app/assets/javascripts/components/annotations/line_of_code.ts new file mode 100644 index 0000000000..c432ab6e0b --- /dev/null +++ b/app/assets/javascripts/components/annotations/line_of_code.ts @@ -0,0 +1,172 @@ +import { ShadowlessLitElement } from "components/meta/shadowless_lit_element"; +import { customElement, property } from "lit/decorators.js"; +import { html, TemplateResult } from "lit"; +import { MachineAnnotationData, machineAnnotationState } from "state/MachineAnnotations"; +import { UserAnnotationData, userAnnotationState } from "state/UserAnnotations"; +import { AnnotationData, compareAnnotationOrders } from "state/Annotations"; +import { submissionState } from "state/Submissions"; +import "components/annotations/annotation_marker"; +import "components/annotations/annotation_tooltip"; +import "components/annotations/selection_layer"; +import { unsafeHTML } from "lit/directives/unsafe-html.js"; + +declare type range = { + start: number; + length: number; + annotations: AnnotationData[]; +}; + +function numberArrayEquals(a: number[], b: number[]): boolean { + return a.length === b.length && a.every((v, i) => v === b[i]); +} + +/** + * renders the code with formatting and tooltips split into four layers: + * - the background layer renders the markup for annotations on the code (background color and underline) + * - the selection layer renders the background color for the selection (split off because keeping selection logic separate from existing annotations makes it much faster) + * - the tooltip layer handles all pointer events (text selection and tooltips) + * - the text layer renders the code with syntax highlighting + * + * @prop {number} row - the row of the code to render + * @prop {string} renderedCode - the syntax highlighted code to render + * + * @element d-code-listing + */ +@customElement("d-line-of-code") +export class LineOfCode extends ShadowlessLitElement { + @property({ type: Number }) + row: number; + @property({ type: String, attribute: "rendered-code" }) + renderedCode: string; + + get code(): string { + return submissionState.codeByLine[this.row - 1]; + } + + get codeLength(): number { + return this.code.length; + } + + get machineAnnotationsToMark(): MachineAnnotationData[] { + return machineAnnotationState.byMarkedLine.get(this.row) || []; + } + + get userAnnotationsToMark(): UserAnnotationData[] { + return userAnnotationState.rootIdsByMarkedLine.get(this.row)?.map(i => userAnnotationState.byId.get(i)) || []; + } + + get fullLineAnnotations(): UserAnnotationData[] { + return this.userAnnotationsToMark + .filter(a => !a.column && !a.columns) + .sort(compareAnnotationOrders); + } + + /** + * Calculates the range of the code that is covered by the given annotation. + * If the annotation spans multiple lines, the range will be the whole line unless this is the first or last line. + * In that case, the range will be the part of the line that is covered by the annotation. + * @param annotation The annotation to calculate the range for. + */ + getRangeFromAnnotation(annotation: AnnotationData, index: number): { start: number, length: number, index: number } { + const isMachineAnnotation = ["error", "warning", "info"].includes(annotation.type); + const rowsLength = annotation.rows ?? 1; + let lastRow = annotation.row ? annotation.row + rowsLength : 0; + let firstRow = annotation.row ? annotation.row + 1 : 0; + + if (!isMachineAnnotation) { + // rows on user annotations are 1-based, so we need to subtract 1 + firstRow -= 1; + lastRow -= 1; + } + + let start = 0; + if (this.row === firstRow) { + start = annotation.column || 0; + } + + let length = this.codeLength - start; + if (this.row === lastRow) { + if (annotation.column !== undefined && annotation.column !== null) { + const defaultLength = isMachineAnnotation ? 0 : this.codeLength - start; + length = annotation.columns || defaultLength; + } + } + + return { start: start, length: length, index: index }; + } + + mergeRanges(ranges: { start: number, length: number, index: number }[]): { start: number, length: number, indexes: number[] }[] { + const annotationsByPosition: number[][] = new Array(this.codeLength).fill(null).map(() => []); + for (const range of ranges) { + for (let i = range.start; i < range.start + range.length; i++) { + annotationsByPosition[i].push(range.index); + } + } + + const zeroLengthRanges = ranges.filter(range => range.length === 0); + const zeroLengthIndexesByPosition: number[][] = new Array(this.codeLength+1).fill(null).map(() => []); + for (const range of zeroLengthRanges) { + zeroLengthIndexesByPosition[range.start].push(range.index); + } + + const rangesToReturn = []; + let i = 0; + while (i < this.codeLength) { + if (zeroLengthIndexesByPosition[i].length) { + rangesToReturn.push({ start: i, length: 0, indexes: zeroLengthIndexesByPosition[i] }); + } + let j = 1; + while (i + j < this.codeLength && numberArrayEquals(annotationsByPosition[i + j], annotationsByPosition[i]) && !zeroLengthIndexesByPosition[i + j].length) { + j++; + } + rangesToReturn.push({ start: i, length: j, indexes: annotationsByPosition[i] }); + i += j; + } + + // Add the zero length ranges at the end of the line + if (zeroLengthIndexesByPosition[this.codeLength].length) { + rangesToReturn.push({ start: this.codeLength, length: 0, indexes: zeroLengthIndexesByPosition[this.codeLength] }); + } + return rangesToReturn; + } + + get ranges(): range[] { + const toMark: AnnotationData[] = [...this.machineAnnotationsToMark, ...this.userAnnotationsToMark]; + // We use indexes to simplify the equality check in mergeRanges + const ranges = toMark.map((annotation, index) => this.getRangeFromAnnotation(annotation, index)); + const mergedRanges = this.mergeRanges(ranges); + return mergedRanges.map(range => { + const annotations = range.indexes.map(i => toMark[i]); + return { start: range.start, length: range.length, annotations: annotations }; + }); + } + + render(): TemplateResult { + const backgroundLayer = []; + const tooltipLayer = []; + + for (const range of this.ranges) { + const substring = this.code.substring(range.start, range.start + range.length); + if (!range.annotations.length) { + backgroundLayer.push(substring); + tooltipLayer.push(substring); + } else { + backgroundLayer.push(html`${substring}`); + tooltipLayer.push(html`${substring}`); + } + } + + return html` + + ${ this.fullLineAnnotations.length > 0 ? html` + + ${backgroundLayer} + ` : html` + ${backgroundLayer} + `} + + ${tooltipLayer} + ${unsafeHTML(this.renderedCode)} + `; + } +} diff --git a/app/assets/javascripts/components/annotations/select.ts b/app/assets/javascripts/components/annotations/selectionHelpers.ts similarity index 95% rename from app/assets/javascripts/components/annotations/select.ts rename to app/assets/javascripts/components/annotations/selectionHelpers.ts index 52cd94b6f4..b7004c29b6 100644 --- a/app/assets/javascripts/components/annotations/select.ts +++ b/app/assets/javascripts/components/annotations/selectionHelpers.ts @@ -133,15 +133,15 @@ export function selectedRangeFromSelection(selection: Selection): SelectedRange const newRange = new Range(); const startLine = document.querySelector(`#line-${range.row}`); const endLine = document.querySelector(`#line-${range.row + range.rows - 1}`); - newRange.setStart(startLine.querySelector(".code-line"), 0); - newRange.setEnd(endLine.querySelector(".code-line"), endLine.querySelector(".code-line").childNodes.length); + newRange.setStart(startLine.querySelector(".tooltip-layer"), 0); + newRange.setEnd(endLine.querySelector(".tooltip-layer"), endLine.querySelector(".tooltip-layer").childNodes.length); selection.addRange(newRange); } else { for (let i = range.row; i < range.row + range.rows; i++) { const newRange = new Range(); const line = document.querySelector(`#line-${i}`); - newRange.setStart(line.querySelector(".code-line"), 0); - newRange.setEnd(line.querySelector(".code-line"), line.querySelector(".code-line").childNodes.length); + newRange.setStart(line.querySelector(".tooltip-layer"), 0); + newRange.setEnd(line.querySelector(".tooltip-layer"), line.querySelector(".tooltip-layer").childNodes.length); selection.addRange(newRange); } } diff --git a/app/assets/javascripts/components/annotations/selection_layer.ts b/app/assets/javascripts/components/annotations/selection_layer.ts new file mode 100644 index 0000000000..74e1dbf9a9 --- /dev/null +++ b/app/assets/javascripts/components/annotations/selection_layer.ts @@ -0,0 +1,56 @@ +import { html, TemplateResult } from "lit"; +import { customElement, property } from "lit/decorators.js"; +import { userAnnotationState } from "state/UserAnnotations"; +import { ShadowlessLitElement } from "components/meta/shadowless_lit_element"; +import { submissionState } from "state/Submissions"; +import { annotationState } from "state/Annotations"; + +/** + * A separate layer that contains the marking for the selection. + * It is separate from code-layers to make the rendering independent and thus much faster. + * + * The selected code is styled to look like a user annotation. + * It is used to mark the selected code while editing an annotation + * @element d-selection-layer + */ +@customElement("d-selection-layer") +class SelectionLayer extends ShadowlessLitElement { + @property({ type: Number }) + row: number; + + get code(): string { + return submissionState.codeByLine[this.row - 1]; + } + + + get shouldMarkSelection(): boolean { + return userAnnotationState.selectedRange && + userAnnotationState.selectedRange.row <= this.row && + userAnnotationState.selectedRange.row + (userAnnotationState.selectedRange.rows ?? 1) > this.row; + } + + get markingClass(): string { + return annotationState.isQuestionMode ? "question-selection-marker" : "annotation-selection-marker"; + } + + render(): TemplateResult { + if (!this.shouldMarkSelection) { + return html``; + } + if (!userAnnotationState.selectedRange.column && !userAnnotationState.selectedRange.columns) { + return html`${this.code}`; + } + + const start = userAnnotationState.selectedRange.column ?? 0; + const end = userAnnotationState.selectedRange.columns ? start + userAnnotationState.selectedRange.columns : this.code.length; + + const selectionLayer: (TemplateResult | string)[] = [ + html``, // this is a hack to force the height of the line to be correct even if no code is on this line + this.code.substring(0, start), + html`${this.code.substring(start, end)}`, + this.code.substring(end) + ]; + + return html`${selectionLayer}`; + } +} diff --git a/app/assets/javascripts/components/annotations/selection_marker.ts b/app/assets/javascripts/components/annotations/selection_marker.ts deleted file mode 100644 index 71d1e24072..0000000000 --- a/app/assets/javascripts/components/annotations/selection_marker.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { LitElement, html, TemplateResult } from "lit"; -import { customElement } from "lit/decorators.js"; -import { AnnotationMarker } from "components/annotations/annotation_marker"; -import { annotationState } from "state/Annotations"; - -/** - * A marker that replaces the selection. - * The slotted content is styled to look like a user annotation. - * It is used to mark the selected code while editing an annotation - * @element d-selection-marker - */ -@customElement("d-selection-marker") -class SelectionMarker extends LitElement { - render(): TemplateResult { - return html``; - } -} diff --git a/app/assets/javascripts/mark.ts b/app/assets/javascripts/mark.ts deleted file mode 100644 index a39dd823a4..0000000000 --- a/app/assets/javascripts/mark.ts +++ /dev/null @@ -1,154 +0,0 @@ -export type range = { start: number, length: number, data?: unknown }; // data is optional -type callback = (node: Node, range: range) => void; - -/** - * Returns an array of text nodes or empty wrapper nodes and their start and end indices in the root node. - * @param root The root node to search for text nodes. - * @param wrapper The type of wrapper to search for. - */ -function getTextNodes(root: Node, wrapper: string): { start: number, end: number, node: Text; }[] { - let val = ""; - const nodes = []; - const iterator = document.createNodeIterator(root, NodeFilter.SHOW_ALL, node => { - if (node.nodeType === Node.TEXT_NODE) { - return NodeFilter.FILTER_ACCEPT; - } else if (node.nodeName.toUpperCase() === wrapper.toUpperCase() && node.childNodes.length === 0) { - // Accept empty wrapper nodes - return NodeFilter.FILTER_ACCEPT; - } else { - return NodeFilter.FILTER_REJECT; - } - }); - - let node; - while (node = iterator.nextNode()) { - nodes.push({ - start: val.length, - end: (val += node.textContent).length, - node - }); - } - return nodes; -} - -/** - * Returns the closest wrapper node of the given type that is an ancestor of the given node. - * @param node The node to search for a wrapper. - * @param wrapper The type of wrapper to search for. - */ -function closestWrapper(node: Node, wrapper: string): Node | null { - let parent = node; - while (parent !== null) { - if (parent.nodeName.toUpperCase() === wrapper.toUpperCase()) { - return parent; - } - parent = parent.parentNode; - } - return null; -} - -/** - * Wraps all elements in the given range of the root node in the given wrapper node. - * For each part of the range that is already wrapped in the given wrapper node, the callback is called with the existing wrapper node. - * For each part of the range that is not wrapped in the given wrapper node, the callback is called for the newly created wrapper node. - * - * If no text nodes are found in the root node, an empty wrapper node is created and the callback is called for it. - * - * @param root The root node to search for text nodes. - * @param range The range to wrap. - * @param wrapper The type of wrapper to create. - * @param callback The callback to call for each wrapper node. - */ -function wrapRange(root: Node, range: range, wrapper: string, callback: callback): void { - const start = range.start; - const end = start + range.length; - const nodes = getTextNodes(root, wrapper); - - let wrappedLength = 0; - for (const node of nodes) { - if (node.end > start && node.start <= end && node.node.textContent !== "\n" || (range.length === 0 && node.end === start)) { - const closest = closestWrapper(node.node, wrapper); - if (closest === null) { - const splitStart = Math.max(0, start - node.start); - let nodeToWrap = node.node; - if (start > node.start) { - nodeToWrap = node.node.splitText(splitStart); - } - - if (node.end > end) { - nodeToWrap.splitText(end - node.start - splitStart); - } - const wrapperNode = document.createElement(wrapper); - wrapperNode.textContent = nodeToWrap.textContent; - nodeToWrap.parentNode.replaceChild(wrapperNode, nodeToWrap); - callback(wrapperNode, range); - - // Avoid needless wrapping of empty text nodes - wrappedLength += wrapperNode.textContent.length; - if (wrappedLength >= range.length) { - return; - } - } else { - callback(closest, range); - // Avoid needless wrapping of empty text nodes - wrappedLength += closest.textContent.length; - if (wrappedLength >= range.length) { - return; - } - } - } - } - - if (nodes.length === 0) { - const wrapperNode = document.createElement(wrapper); - wrapperNode.textContent = root.textContent; - root.appendChild(wrapperNode); - callback(wrapperNode, range); - } -} - -/** - * Wraps all elements in the given ranges of the root node in the given wrapper node. - * @param root The root node to search for text nodes. - * @param ranges The ranges to wrap. - * @param wrapper The type of wrapper to create. - * @param callback The callback to call for each wrapper node. - */ -function wrapRanges(root: Node, ranges: range[], wrapper: string, callback: callback): void { - ranges.forEach(range => wrapRange(root, range, wrapper, callback)); -} - -/** - * Todo move to utils when it is changed to typescript - * - * Returns a function that mimics the given function, but caches its results. - * This is a generic memoization function that works with any number of arguments. - * @param f The function to memoize. - */ -function cached(f: (...args: ArgumentsType) => ReturnType): (...args: ArgumentsType) => ReturnType { - const cache = new Map(); - return (...args: ArgumentsType) => { - const key = JSON.stringify(args); - if (!cache.has(key)) { - cache.set(key, f(...args)); - } - return cache.get(key); - }; -} - -/** - * Wraps all elements in the given ranges of the given target string in the given wrapper node. - * @param target a html string whose text nodes should be wrapped - * @param ranges the ranges of the textcontent to wrap - * @param wrapper the type of wrapper to create - * @param callback the callback to call for each wrapper node - */ - -export function wrapRangesInHtml(target: string, ranges: range[], wrapper: string, callback: callback = () => undefined): string { - const root = document.createElement("div"); - root.innerHTML = target; - wrapRanges(root, ranges, wrapper, callback); - return root.innerHTML; -} - -export const wrapRangesInHtmlCached = cached(wrapRangesInHtml); diff --git a/app/assets/javascripts/state/Annotations.ts b/app/assets/javascripts/state/Annotations.ts index 6a847ea795..f7626c355d 100644 --- a/app/assets/javascripts/state/Annotations.ts +++ b/app/assets/javascripts/state/Annotations.ts @@ -2,7 +2,6 @@ import { MachineAnnotationData } from "state/MachineAnnotations"; import { AnnotationType, UserAnnotationData } from "state/UserAnnotations"; import { State } from "state/state_system/State"; import { stateProperty } from "state/state_system/StateProperty"; -import { StateMap } from "state/state_system/StateMap"; export type AnnotationVisibilityOptions = "all" | "important" | "none"; export type AnnotationData = MachineAnnotationData | UserAnnotationData; diff --git a/app/assets/javascripts/state/Submissions.ts b/app/assets/javascripts/state/Submissions.ts index 8d3e436f26..d104809b55 100644 --- a/app/assets/javascripts/state/Submissions.ts +++ b/app/assets/javascripts/state/Submissions.ts @@ -2,8 +2,18 @@ import { State } from "state/state_system/State"; import { stateProperty } from "state/state_system/StateProperty"; class SubmissionState extends State { - @stateProperty id; - @stateProperty code; + @stateProperty id: number; + @stateProperty _code: string; + @stateProperty codeByLine: string[]; + + set code(code: string) { + this._code = code; + this.codeByLine = code.split("\n"); + } + + get code(): string { + return this._code; + } } export const submissionState = new SubmissionState(); diff --git a/app/assets/javascripts/util.js b/app/assets/javascripts/util.js index 18a5769b5b..57d44ec512 100644 --- a/app/assets/javascripts/util.js +++ b/app/assets/javascripts/util.js @@ -226,9 +226,6 @@ function getParentByClassName(element, classNames) { return null; } -// insert `cached` function here after move to typescript -// the function is currently in `app/assets/javascripts/mark.ts` - export { createDelayer, delay, diff --git a/app/assets/stylesheets/components/code_listing.css.scss b/app/assets/stylesheets/components/code_listing.css.scss index c0de57861f..dd4e8d3b38 100644 --- a/app/assets/stylesheets/components/code_listing.css.scss +++ b/app/assets/stylesheets/components/code_listing.css.scss @@ -104,39 +104,6 @@ --question-color: #{var(--d-warning-container)}; --annotation-color: #{var(--d-warning-container)}; - - pre { - background-color: inherit; - - &.code-line-question { - background-color: var(--d-annotation-question-background-intense); - } - - &.code-line-annotation { - background-color: var(--d-annotation-user-background-intense); - } - } - } - - // Highlight fully selected lines, also force all annotations in the line to the selection color - .lineno .rouge-code pre { - &.code-line-question { - background-color: var(--d-annotation-question-background-intense); - width: 100%; - - d-annotation-marker { - background-color: var(--d-annotation-question-background-intense); - } - } - - &.code-line-annotation { - background-color: var(--d-annotation-user-background-intense); - width: 100%; - - d-annotation-marker { - background-color: var(--d-annotation-user-background-intense); - } - } } .annotation-button { @@ -239,10 +206,6 @@ white-space: pre-wrap; word-break: break-word; min-height: 20px; - margin-bottom: -3px; - padding-bottom: 3px; - margin-top: -2px; - padding-top: 2px; } } @@ -279,12 +242,54 @@ d-code-listing-row { display: contents; + .code-layers { + position: relative; + width: 100%; + height: 100%; + + .background-layer, + .selection-layer, + .text-layer, + .tooltip-layer { + position: absolute; + width: 100%; + height: 100%; + top: 0; + pointer-events: none; + user-select: none; + color: transparent; + } + + .tooltip-layer { + pointer-events: auto; + user-select: auto; + } + + .text-layer { + color: inherit; + background: transparent; + } + + .background-layer { + position: initial; + display: block; + } + } + pre.code-line { overflow: visible; display: inline-block; padding-top: 1px; } + .question-selection-marker { + background-color: var(--d-annotation-question-background-intense) + } + + .annotation-selection-marker { + background-color: var(--d-annotation-user-background-intense) + } + // This is a hack to extend the drop target more to the left .drop-target-extension { position: absolute; diff --git a/test/javascript/components/annotations/select.test.ts b/test/javascript/components/annotations/select.test.ts index 9e09c571ce..9a00199b2b 100644 --- a/test/javascript/components/annotations/select.test.ts +++ b/test/javascript/components/annotations/select.test.ts @@ -1,5 +1,5 @@ import { fixture } from "@open-wc/testing-helpers"; -import { getOffset, selectedRangeFromSelection } from "components/annotations/select"; +import { getOffset, selectedRangeFromSelection } from "components/annotations/selectionHelpers"; import "components/annotations/code_listing_row"; import { submissionState } from "state/Submissions"; @@ -32,6 +32,7 @@ describe("getOffsetTest", () => { describe("selectedRangeFromSelectionTest", () => { let context; beforeEach(async () => { + submissionState.code = "hello world\n\nprint(world)"; context = await fixture(` bar @@ -40,7 +41,6 @@ describe("selectedRangeFromSelectionTest", () => { `); - submissionState.code = "hello world\n\nprint(world)"; window.getSelection().removeAllRanges(); }); @@ -69,10 +69,10 @@ describe("selectedRangeFromSelectionTest", () => { expect(selection.rangeCount).toBe(1); const newRange = selection.getRangeAt(0); - expect(newRange.startContainer).toBe(context.querySelector("#line-1 .code-line")); - expect(newRange.endContainer).toBe(context.querySelector("#line-3 .code-line")); + expect(newRange.startContainer).toBe(context.querySelector("#line-1 .tooltip-layer")); + expect(newRange.endContainer).toBe(context.querySelector("#line-3 .tooltip-layer")); expect(newRange.startOffset).toBe(0); - expect(newRange.endOffset).toBe(5); + expect(newRange.endOffset).toBe(4); }); it("Should create multiple ranges if the selection contains multiple ranges", async () => { @@ -126,10 +126,10 @@ describe("selectedRangeFromSelectionTest", () => { expect(selection.rangeCount).toBe(1); const newRange = selection.getRangeAt(0); - expect(newRange.startContainer).toBe(context.querySelector("#line-3 .code-line")); - expect(newRange.endContainer).toBe(context.querySelector("#line-3 .code-line")); + expect(newRange.startContainer).toBe(context.querySelector("#line-3 .tooltip-layer")); + expect(newRange.endContainer).toBe(context.querySelector("#line-3 .tooltip-layer")); expect(newRange.startOffset).toBe(0); - expect(newRange.endOffset).toBe(5); + expect(newRange.endOffset).toBe(4); }); it("should remove ending empty lines from the selection", async () => { diff --git a/test/javascript/mark.test.ts b/test/javascript/mark.test.ts deleted file mode 100644 index fa6caedd48..0000000000 --- a/test/javascript/mark.test.ts +++ /dev/null @@ -1,54 +0,0 @@ -import { wrapRangesInHtml } from "mark"; - -test("marking a range in an empty string should still create a marker", () => { - const result = wrapRangesInHtml("", [{ start: 0, length: 1 }], "mark", () => undefined); - expect(result).toBe(""); -}); - -test("marking a range in a string should create a marker", () => { - const result = wrapRangesInHtml("Hello World", [{ start: 0, length: 5 }], "mark", () => undefined); - expect(result).toBe("Hello World"); -}); - -test("marking a range in a complex html string should create a marker", () => { - const result = wrapRangesInHtml("Hello World", [{ start: 0, length: 5 }], "mark", () => undefined); - expect(result).toBe("Hello World"); -}); - -test("Callback should be called for each marker", () => { - const callback = jest.fn(); - wrapRangesInHtml("Hello World", [{ start: 0, length: 5 }], "mark", callback); - expect(callback).toHaveBeenCalledTimes(4); -}); - -test("should be able to mark multiple ranges", () => { - const result = wrapRangesInHtml("Hello World", [{ start: 0, length: 5 }, { start: 6, length: 2 }], "mark", () => undefined); - expect(result).toBe("Hello World"); -}); - -test("should be able to mark multiple ranges with overlapping ranges", () => { - const result = wrapRangesInHtml("Hello World", [{ start: 0, length: 5 }, { start: 2, length: 2 }], "mark", () => undefined); - expect(result).toBe("Hello World"); -}); - -test("Callback should be called for each marker for each range", () => { - const callback = jest.fn(); - wrapRangesInHtml("Hello World", [{ start: 0, length: 5 }, { start: 2, length: 2 }], "mark", callback); - expect(callback).toHaveBeenCalledTimes(6); -}); - -test("should be able to edit attributes of the marker in the callback", () => { - const callback = jest.fn((element, range) => { - const test = element.getAttribute("data-test") || ""; - element.setAttribute("data-test", test + range.data); - }); - const result = wrapRangesInHtml("Hello World", [{ start: 0, length: 5, data: "foo" }, { start: 2, length: 7, data: "bar" }], "mark", callback); - expect(callback).toHaveBeenCalledWith(expect.any(HTMLElement), { start: 0, length: 5, data: "foo" }); - expect(callback).toHaveBeenCalledWith(expect.any(HTMLElement), { start: 2, length: 7, data: "bar" }); - expect(result).toBe("Hello World"); -}); - -test("should be able to mark zero length ranges", () => { - const result = wrapRangesInHtml("Hello World", [{ start: 0, length: 0 }, { start: 5, length: 0 }, { start: 8, length: 0 }, { start: 11, length: 0 }], "mark", () => undefined); - expect(result).toBe("Hello World"); -});
${this.row}
${unsafeHTML(this.wrappedCode)}
${backgroundLayer}
${tooltipLayer}
${unsafeHTML(this.renderedCode)}
${this.code}
${selectionLayer}