Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Communication: Add additional input formatting options #9657

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ import { ItalicAction } from 'app/shared/monaco-editor/model/actions/italic.acti
import { UnderlineAction } from 'app/shared/monaco-editor/model/actions/underline.action';
import { CodeAction } from 'app/shared/monaco-editor/model/actions/code.action';
import { UrlAction } from 'app/shared/monaco-editor/model/actions/url.action';
import { UnorderedListAction } from 'app/shared/monaco-editor/model/actions/unordered-list.action';
import { OrderedListAction } from 'app/shared/monaco-editor/model/actions/ordered-list.action';
import { BulletedListAction } from 'app/shared/monaco-editor/model/actions/bulleted-list.action';
import { StrikethroughAction } from 'app/shared/monaco-editor/model/actions/strikethrough.action';
import { InsertShortAnswerSpotAction } from 'app/shared/monaco-editor/model/actions/quiz/insert-short-answer-spot.action';
import { TextEditorAction } from 'app/shared/monaco-editor/model/actions/text-editor-action.model';
import { InsertShortAnswerOptionAction } from 'app/shared/monaco-editor/model/actions/quiz/insert-short-answer-option.action';
Expand Down Expand Up @@ -122,9 +123,10 @@ export class ShortAnswerQuestionEditComponent implements OnInit, OnChanges, Afte
new BoldAction(),
new ItalicAction(),
new UnderlineAction(),
new StrikethroughAction(),
new CodeAction(),
new UrlAction(),
new UnorderedListAction(),
new BulletedListAction(),
new OrderedListAction(),
Comment on lines +126 to 130
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Remaining references to UnorderedListAction need to be updated

  • src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts: Tests are still using UnorderedListAction
  • src/main/webapp/app/shared/monaco-editor/model/actions/unordered-list.action.ts: The old action file still exists and needs to be removed

The codebase still contains references to the old UnorderedListAction in test files and the original action file. These need to be updated to use BulletedListAction for consistency.

🔗 Analysis chain

Verify the replacement of UnorderedListAction with BulletedListAction.

The changes look good, but let's verify that all existing usages of UnorderedListAction have been updated to use BulletedListAction to prevent any broken functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to UnorderedListAction
rg "UnorderedListAction" -A 5

# Search for existing usages of BulletedListAction to ensure proper integration
rg "BulletedListAction" -A 5

Length of output: 20864

this.insertShortAnswerSpotAction,
this.insertShortAnswerOptionAction,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ import { CodeAction } from 'app/shared/monaco-editor/model/actions/code.action';
import { CodeBlockAction } from 'app/shared/monaco-editor/model/actions/code-block.action';
import { UrlAction } from 'app/shared/monaco-editor/model/actions/url.action';
import { AttachmentAction } from 'app/shared/monaco-editor/model/actions/attachment.action';
import { UnorderedListAction } from 'app/shared/monaco-editor/model/actions/unordered-list.action';
import { BulletedListAction } from 'app/shared/monaco-editor/model/actions/bulleted-list.action';
import { StrikethroughAction } from 'app/shared/monaco-editor/model/actions/strikethrough.action';
import { OrderedListAction } from 'app/shared/monaco-editor/model/actions/ordered-list.action';
import { faAngleDown, faGripLines, faQuestionCircle } from '@fortawesome/free-solid-svg-icons';
import { v4 as uuid } from 'uuid';
Expand Down Expand Up @@ -159,13 +160,14 @@ export class MarkdownEditorMonacoComponent implements AfterContentInit, AfterVie
new BoldAction(),
new ItalicAction(),
new UnderlineAction(),
new StrikethroughAction(),
new QuoteAction(),
new CodeAction(),
new CodeBlockAction('java'),
new UrlAction(),
new AttachmentAction(),
new OrderedListAction(),
new UnorderedListAction(),
new BulletedListAction(),
];

@Input()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
@if (postingContentPart.contentBeforeReference) {
<span class="markdown-preview" [innerHTML]="postingContentPart.contentBeforeReference | htmlForPostingMarkdown: true : allowedHtmlTags : allowedHtmlAttributes"></span>
<span
class="markdown-preview"
style="white-space: pre-wrap"
[innerHTML]="processedContentBeforeReference | htmlForPostingMarkdown: true : allowedHtmlTags : allowedHtmlAttributes"
></span>
}
@if (postingContentPart.linkToReference) {
<a class="reference" [routerLink]="postingContentPart.linkToReference" [queryParams]="postingContentPart.queryParams">
Expand Down Expand Up @@ -68,5 +72,9 @@ <h5 class="card-body"><fa-icon class="px-1" [icon]="faBan" /><span jhiTranslate=
}

@if (postingContentPart.contentAfterReference) {
<span class="markdown-preview" [innerHTML]="postingContentPart.contentAfterReference | htmlForPostingMarkdown: false : allowedHtmlTags : allowedHtmlAttributes"></span>
<span
class="markdown-preview"
style="white-space: pre-wrap"
[innerHTML]="processedContentAfterReference | htmlForPostingMarkdown: false : allowedHtmlTags : allowedHtmlAttributes"
></span>
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Component, EventEmitter, Input, Output } from '@angular/core';
import { Component, EventEmitter, Input, OnInit, Output } from '@angular/core';
import { PostingContentPart, ReferenceType } from '../../metis.util';
import { FileService } from 'app/shared/http/file.service';

Expand Down Expand Up @@ -26,7 +26,7 @@ import { AccountService } from 'app/core/auth/account.service';
templateUrl: './posting-content-part.component.html',
styleUrls: ['./../../metis.component.scss'],
})
export class PostingContentPartComponent {
export class PostingContentPartComponent implements OnInit {
@Input() postingContentPart: PostingContentPart;
@Output() userReferenceClicked = new EventEmitter<string>();
@Output() channelReferenceClicked = new EventEmitter<number>();
Expand All @@ -35,7 +35,7 @@ export class PostingContentPartComponent {
hasClickedUserReference = false;

// Only allow certain html tags and attributes
allowedHtmlTags: string[] = ['a', 'b', 'br', 'blockquote', 'code', 'del', 'em', 'i', 'ins', 'li', 'mark', 'ol', 'p', 'pre', 'small', 'span', 'strong', 'sub', 'sup', 'ul'];
allowedHtmlTags: string[] = ['a', 'b', 'br', 'blockquote', 'code', 'del', 'em', 'i', 'ins', 'mark', 'p', 'pre', 'small', 's', 'span', 'strong', 'sub', 'sup'];
allowedHtmlAttributes: string[] = ['href'];

// icons
Expand All @@ -45,13 +45,19 @@ export class PostingContentPartComponent {
protected readonly faHashtag = faHashtag;

protected readonly ReferenceType = ReferenceType;
processedContentBeforeReference: string;
processedContentAfterReference: string;

constructor(
private fileService: FileService,
private dialog: MatDialog,
private accountService: AccountService,
) {}

ngOnInit() {
this.processContent();
}

/**
* Opens an attachment with the given URL in a new window
*
Expand All @@ -65,6 +71,20 @@ export class PostingContentPartComponent {
this.imageNotFound = true;
}

processContent() {
if (this.postingContentPart.contentBeforeReference) {
this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference);
}

if (this.postingContentPart.contentAfterReference) {
this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference);
}
}

Comment on lines +74 to +82
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling in processContent method.

The method should handle cases where postingContentPart is undefined or null to prevent runtime errors.

     processContent() {
+        if (!this.postingContentPart) {
+            return;
+        }
+
         if (this.postingContentPart.contentBeforeReference) {
             this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference);
         }

         if (this.postingContentPart.contentAfterReference) {
             this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference);
         }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
processContent() {
if (this.postingContentPart.contentBeforeReference) {
this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference);
}
if (this.postingContentPart.contentAfterReference) {
this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference);
}
}
processContent() {
if (!this.postingContentPart) {
return;
}
if (this.postingContentPart.contentBeforeReference) {
this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference);
}
if (this.postingContentPart.contentAfterReference) {
this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference);
}
}

escapeNumberedList(content: string): string {
return content.replace(/^(\s*\d+)\. /gm, '$1\\. ');
}

/**
* Opens a dialog to display the image in full size
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ import { AttachmentAction } from 'app/shared/monaco-editor/model/actions/attachm
import { ConversationDTO } from 'app/entities/metis/conversation/conversation.model';
import { EmojiAction } from 'app/shared/monaco-editor/model/actions/emoji.action';
import { Overlay, OverlayPositionBuilder } from '@angular/cdk/overlay';
import { BulletedListAction } from 'app/shared/monaco-editor/model/actions/bulleted-list.action';
import { OrderedListAction } from 'app/shared/monaco-editor/model/actions/ordered-list.action';
import { StrikethroughAction } from 'app/shared/monaco-editor/model/actions/strikethrough.action';

@Component({
selector: 'jhi-posting-markdown-editor',
Expand Down Expand Up @@ -97,7 +100,10 @@ export class PostingMarkdownEditorComponent implements OnInit, ControlValueAcces
new BoldAction(),
new ItalicAction(),
new UnderlineAction(),
new StrikethroughAction(),
new EmojiAction(this.viewContainerRef, this.overlay, this.positionBuilder),
new BulletedListAction(),
new OrderedListAction(),
new QuoteAction(),
new CodeAction(),
new CodeBlockAction(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { faListUl } from '@fortawesome/free-solid-svg-icons';
import { ListAction } from './list.action';

const BULLET_PREFIX = '• ';

/**
* Action used to add or modify a bullet-point list in the text editor.
*/
export class BulletedListAction extends ListAction {
static readonly ID = 'bulletedList.action';

protected readonly PREFIX = BULLET_PREFIX;

constructor() {
super(BulletedListAction.ID, 'artemisApp.multipleChoiceQuestion.editor.unorderedList', faListUl, undefined);
}

protected getPrefix(lineNumber: number): string {
void lineNumber;
return this.PREFIX;
}
Comment on lines +18 to +21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve handling of unused lineNumber parameter

The lineNumber parameter is marked as unused with void. If the parameter isn't needed, consider:

  1. Removing it if not required by base class contract
  2. Implementing line number-based prefixing if it should affect the bullet style

If the parameter is truly unused, apply this change:

-    protected getPrefix(lineNumber: number): string {
-        void lineNumber;
-        return this.PREFIX;
-    }
+    protected getPrefix(): string {
+        return this.PREFIX;
+    }

Committable suggestion skipped: line range outside the PR's diff.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
import { TextEditorAction } from 'app/shared/monaco-editor/model/actions/text-editor-action.model';
import { TextEditor } from 'app/shared/monaco-editor/model/actions/adapter/text-editor.interface';
import { TextEditorRange } from 'app/shared/monaco-editor/model/actions/adapter/text-editor-range.model';
import { TextEditorPosition } from 'app/shared/monaco-editor/model/actions/adapter/text-editor-position.model';
import { IconDefinition } from '@fortawesome/fontawesome-svg-core';
import { TextEditorKeybinding } from 'app/shared/monaco-editor/model/actions/adapter/text-editor-keybinding.model';

/**
* Abstract class representing a list action in a text editor.
* This class handles adding and removing list prefixes and supports event listeners
* for features like continuing lists with Shift/Cmd+Enter.
*
* @abstract
* @extends TextEditorAction
*/
export abstract class ListAction extends TextEditorAction {
protected static editorsWithListener = new WeakMap<TextEditor, boolean>();

protected abstract readonly PREFIX: string;
protected abstract getPrefix(lineNumber: number): string;

/**
* Constructor for the ListAction class.
* @param {string} id - The unique ID of the action.
* @param {string} label - The label of the action.
* @param {IconDefinition} icon - The icon to display for the action.
* @param {TextEditorKeybinding[]} shortcut - The keyboard shortcut for the action.
*/
protected constructor(id: string, label: string, icon: IconDefinition | undefined, shortcut: TextEditorKeybinding[] | undefined) {
super(id, label, icon, shortcut);
}

/**
* Removes any list prefix (either bullet or numbered) from the given line.
* @param {string} line - The line to process.
* @returns {string} - The line without any list prefix.
*/
protected stripAnyListPrefix(line: string): string {
const numberedListRegex = /^\s*\d+\.\s+/;
const bulletListRegex = /^\s*[-*+•]\s+/;

if (numberedListRegex.test(line)) {
return line.replace(numberedListRegex, '');
} else if (bulletListRegex.test(line)) {
return line.replace(bulletListRegex, '');
}
Comment on lines +39 to +46
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

[Critical Issue] Correct the regular expressions to match list prefixes accurately

The regular expressions used to detect bullet list prefixes may incorrectly interpret the hyphen - as a range indicator within the character class, leading to unexpected matches. To avoid this issue, the hyphen should either be escaped or placed at the beginning or end of the character class.

Apply the following diff to fix the regular expressions:

In `stripAnyListPrefix` method (lines 39-46):
- const bulletListRegex = /^\s*[-*+•]\s+/;
+ const bulletListRegex = /^\s*[*+•-]\s+/;

In `hasPrefix` method (lines 132-135):
- const bulletListRegex = /^\s*[•\-*+]\s+/;
+ const bulletListRegex = /^\s*[*+•-]\s+/;

In `handleBackspace` method (lines 179-190):
- const linePrefixMatch = lineContent.match(/^\s*(\d+\.\s+|[-*+•]\s+)/);
+ const linePrefixMatch = lineContent.match(/^\s*(\d+\.\s+|[*+•-]\s+)/);

Also applies to: 132-135, 179-190

return line;
}

/**
* Adds or removes a list prefix from the selected text.
* Also handles the Shift/Cmd+Enter key combination to continue the list.
* @param {TextEditor} editor - The editor instance where the action is applied.
*/
run(editor: TextEditor) {
if (!ListAction.editorsWithListener.has(editor)) {
ListAction.editorsWithListener.set(editor, true);

editor.getDomNode()?.addEventListener('keydown', (event: KeyboardEvent) => {
if (event.key === 'Enter' && event.shiftKey) {
event.preventDefault();
this.handleShiftEnter(editor);
} else if (event.key === 'Enter' && event.metaKey) {
event.preventDefault();
event.stopPropagation();
this.handleShiftEnter(editor);
} else if (event.key === 'Backspace') {
this.handleBackspace(editor, event);
}
});
}

const selection = editor.getSelection();
if (!selection) {
return;
}

const selectedText = editor.getTextAtRange(selection);
const lines = selectedText.split('\n');

// Check if the cursor is at the end of the line to add or remove the prefix
let position = editor.getPosition();
if (position) {
const currentLineText = editor.getLineText(position.getLineNumber());

if (!selectedText && position.getColumn() <= currentLineText.length) {
const endPosition = new TextEditorPosition(position.getLineNumber(), currentLineText.length + 1);
editor.setPosition(endPosition);
editor.focus();
position = endPosition;
}

if (position.getColumn() === currentLineText.length + 1) {
const lineWithoutPrefix = this.stripAnyListPrefix(currentLineText);
const newPrefix = this.getPrefix(1);

const updatedLine = currentLineText.startsWith(newPrefix) ? lineWithoutPrefix : newPrefix + lineWithoutPrefix;

editor.replaceTextAtRange(
new TextEditorRange(new TextEditorPosition(position.getLineNumber(), 1), new TextEditorPosition(position.getLineNumber(), currentLineText.length + 1)),
updatedLine,
);
editor.focus();
return;
}
}

const startLineNumber = selection.getStartPosition().getLineNumber();
const currentPrefix = this.getPrefix(startLineNumber);

// Determine if all lines have the current prefix
let allLinesHaveCurrentPrefix;
if (this.getPrefix(1) != '• ') {
const numberedListRegex = /^\s*\d+\.\s+/;
allLinesHaveCurrentPrefix = lines.every((line) => numberedListRegex.test(line));
} else {
allLinesHaveCurrentPrefix = lines.every((line) => line.startsWith(currentPrefix));
}

let updatedLines: string[];
if (allLinesHaveCurrentPrefix) {
updatedLines = lines.map((line) => this.stripAnyListPrefix(line));
} else {
const linesWithoutPrefix = lines.map((line) => this.stripAnyListPrefix(line));

updatedLines = linesWithoutPrefix.map((line, index) => {
const prefix = this.getPrefix(index) != '• ' ? this.getPrefix(index + 1) : this.getPrefix(startLineNumber + index);
return prefix + line;
});
}

const updatedText = updatedLines.join('\n');
editor.replaceTextAtRange(selection, updatedText);
editor.focus();
}

/**
* Checks if a line has any list prefix (either bullet or numbered).
* @param {string} line - The line to check.
* @returns {boolean} - True if the line has a prefix, false otherwise.
*/
protected hasPrefix(line: string): boolean {
const numberedListRegex = /^\s*\d+\.\s+/;
const bulletListRegex = /^\s*[•\-*+]\s+/;
return numberedListRegex.test(line) || bulletListRegex.test(line);
}

/**
* Handles the Shift+Enter key combination to continue the list.
* @param {TextEditor} editor - The editor instance.
*/
protected handleShiftEnter(editor: TextEditor) {
const position = editor.getPosition();
if (position) {
const currentLineText = editor.getLineText(position.getLineNumber());
let nextLinePrefix = '';

// Check if the current line starts with a prefix and continue the list
if (this.hasPrefix(currentLineText)) {
const isNumbered = /^\s*\d+\.\s+/.test(currentLineText);

if (isNumbered) {
const match = currentLineText.match(/^\s*(\d+)\.\s+/);
const currentNumber = match ? parseInt(match[1], 10) : 0;
nextLinePrefix = `${currentNumber + 1}. `;
} else {
nextLinePrefix = '• ';
}
}

editor.replaceTextAtRange(new TextEditorRange(position, position), '\n' + nextLinePrefix);

const newLineNumber = position.getLineNumber() + 1;
const newColumnPosition = nextLinePrefix ? nextLinePrefix.length + 1 : 1;
editor.setPosition(new TextEditorPosition(newLineNumber, newColumnPosition));
editor.focus();
}
}

/**
* Handles the Backspace key press to remove a prefix if the cursor is just after it.
* @param {TextEditor} editor - The editor instance.
* @param {KeyboardEvent} event - The keyboard event.
*/
protected handleBackspace(editor: TextEditor, event: KeyboardEvent) {
const position = editor.getPosition();
if (position) {
const lineNumber = position.getLineNumber();
const lineContent = editor.getLineText(lineNumber);
const linePrefixMatch = lineContent.match(/^\s*(\d+\.\s+|[-*+•]\s+)/);

if (linePrefixMatch) {
const prefixLength = linePrefixMatch[0].length;
// Check if the cursor is just after the prefix
if (position.getColumn() === prefixLength + 1) {
event.preventDefault();
editor.replaceTextAtRange(new TextEditorRange(new TextEditorPosition(lineNumber, 1), new TextEditorPosition(lineNumber, prefixLength + 1)), ' ');
editor.focus();
}
}
}
}
}
Loading
Loading