Adding line numbers in editor #940

Mash707 · 2025-02-10T14:32:51Z

#916
I have done a basic implementation for displaying line numbers in the editor. Currently I have tested it with resizing my browser window

Mash707 · 2025-02-10T14:33:47Z

@jelly @garrett have a look at this. Thank You.

jelly · 2025-02-11T09:23:13Z

From a quick look at the PR:

The line numbers show a scrollbar, it should not as we have a scrollbar on the right already:
Very long lines are shown incorrectly for example line 8 here:

Resized, shown as 3 lines:

Create file does not yet have these line numbers, but that's because it uses its own TextArea, that can be fixed later by refactoring some things

Mash707 · 2025-02-11T13:22:16Z

I have removed the scroll bar from line numbers. Working on long lines issue.

allisonkarlitskaya · 2025-02-11T16:21:13Z

It might be worth looking at the history in #509 a bit to see what we've tried and failed with. @garrett tried adding line numbers there and we took them out because we couldn't get them working reliably.

Mash707 · 2025-02-11T16:39:52Z

It might be worth looking at the history in #509 a bit to see what we've tried and failed with. @garrett tried adding line numbers there and we took them out because we couldn't get them working reliably.

I went through the discussion, implementing long lines thing is a bit complex. Currently I am trying to do something similar to this article.

allisonkarlitskaya · 2025-02-11T19:16:03Z

I went through the discussion, implementing long lines thing is a bit complex. Currently I am trying to do something similar to this article.

The canvas-based approaches look pretty promising but potentially slow on larger text files. Please consider some form of memoization if you're going to do this.

Mash707 · 2025-02-17T10:59:23Z

prism-react-editor uses prismjs , works pretty well but is dependency heavy.

garrett · 2025-02-18T09:01:48Z

Aside note: With my proof of concept, there were versions and updates. Make sure you're testing the latest version, not a previous one please. Some of the CSS changed to fix wrapping and line calculation, where it mostly worked in earlier versions, it's closer to (or exactly as, at least with what I tested) what is expected.

Mash707 · 2025-02-18T18:09:38Z

For now, I do have a solution which implements horizontal scrolling instead of line-wrapping.

Recording.2025-02-18.234242.mp4

garrett · 2025-02-20T10:38:11Z

For now, I do have a solution which implements horizontal scrolling instead of line-wrapping.

We might even want to make it toggleable. In most cases, soft wrapping is the right approach.

I would like to look at this again when I have time to do so. Right now, I do not have the time to do so. I don't know when that would be, but it's not right this moment. Apologies!

Thanks for looking into this and trying to find a solution! Having a PR to work in is fantastic!

My demo seemed to be working outside of Cockpit, and there's the whole issue where something works in vitro but not in vivo. (That is, borrowing from science: something may work in an isolated area, like a test tube, but doesn't work quite the same when inside a body. Same for Cockpit, as there are a lot more things going on within Cockpit already and also with PatternFly than some simplified test area.)

jelly · 2025-02-20T13:50:45Z

I have taken a look at the PR and the line wrapping doesn't work as @garrett's demo code wasn't ported to this PR. Did you have issues integrating it?

I have taken a look and it doesn't seem super hard to create something with a canvas approach, we do need to work on optimization it. At least made some notes for myself.

The required code up front to create a canvas to use for context.measureText()

const textarea = document.querySelector('#editor-text-area')
const styles = window.getComputedStyle(textarea, null)

const parseValue = (v) => v.endsWith('px')  ? parseInt(v.slice(0, -2), 10)  : 0;

const textareaStyles = window.getComputedStyle(textarea);
const paddingLeft = parseValue(textareaStyles.paddingLeft);
const paddingRight = parseValue(textareaStyles.paddingRight);
const textareaWidth = textarea.getBoundingClientRect().width - paddingLeft - paddingRight;

const font_size = textareaStyles.getPropertyValue('font-size');
const font_family = textareaStyles.getPropertyValue('font-family');
const font = `${font_size} ${font_family}`;
const canvas = document.createElement('canvas');
const context = canvas.getContext('2d');
context.font = font;

Also for reference take a look at this github example.

jelly · 2025-02-20T14:29:59Z

Some things for testing as a canvas can have a performance impact:

Large documents we limit the size to 1MB so we should handle that
Different charactersets like Japanese, Arabic, etc.

Mash707 · 2025-02-20T16:19:59Z

I have taken a look at the PR and the line wrapping doesn't work as @garrett's demo code wasn't ported to this PR. Did you have issues integrating it?

I am testing code changes locally but haven't pushed them here.

Mash707 · 2025-02-23T19:30:22Z

Also for reference take a look at this github example.

I have tried implementing the canvas approach. It still breaks though.

20250223-1924-55.4817904.mp4

Mash707 · 2025-02-23T19:39:04Z

Here is a demo of pasting text.

Recording.2025-02-24.010501.mp4

Recording.2025-02-24.010649.mp4

English sample text
Japanese sample text

Mash707 · 2025-02-23T19:50:47Z

I have some how messed up package.json.
node_modules are also shown in the changes.

jelly

Progress looks good! There is an issue with the calculation of the textarea size which causes the "weird bugs" you see when resizing the textarea.

jelly · 2025-02-24T14:20:11Z

src/dialogs/editor.tsx

+        const displayLineNumbers = () => {
+          const lineNumbers = calculateLineNumbers();
+          lineNumbersEle.innerHTML = lineNumbers
+            .map((num) => `<div>${num === 0 ? "&nbsp;" : num}</div>`)


Like @garrett's example we should use a CSS property for the number of lines and use that with calc() instead of adding more html elements.

jelly · 2025-02-24T14:21:06Z

src/dialogs/editor.tsx

+        });
+
+        const resizeObserver = new ResizeObserver(() => {
+          lineNumbersEle.style.height = `${textarea.getBoundingClientRect().height}px`;


This should be done in layout, I don't understand why this is needed?

jelly · 2025-02-24T14:25:27Z

src/dialogs/editor.tsx

+              (lineNumbersEle.style as any)[property] = textareaStyles[property];
+            }
+          });
+        };


This should be done once, and saved in state and changed on resize. paddingTop/paddingBottom should not be interesting for us.

jelly · 2025-02-24T14:26:34Z

src/dialogs/editor.tsx

+
+          const paddingLeft = parseValue(textareaStyles.paddingLeft);
+          const paddingRight = parseValue(textareaStyles.paddingRight);
+          const textareaWidth = textarea.getBoundingClientRect().width - paddingLeft - paddingRight;


This is called every input now and is rather expensive, needs to be put into state and only get called onResize and initially.

I did some further digging and I found out why you have bugs, this width is wrong as it doesn't account for the scrollbar and this is why there are line number issues.

Instead of textarea.getBoundingClientRect().width I tried using textarea.clientWidth , clientWidth excludes scrollbar width. The problem still persists.

jelly · 2025-02-24T14:34:49Z

src/dialogs/editor.tsx

+
+          words.forEach((word) => {
+            const wordWidth = context.measureText(word + " ").width;
+            const lineWidth = context.measureText(currentLine).width;


it feels like we can optimize this away and just add a new variable let lineWidth = 0 and keep appending to that.

Should we use characterWidth instead of wordWidth? I think it will provide more accurate line wrapping.

jelly marked this pull request as draft February 20, 2025 12:58

added canvas approach

e8ae79f

Mash707 force-pushed the adding-line-numbers-in-editor branch from c6f7f51 to e8ae79f Compare February 23, 2025 19:07

jelly reviewed Feb 25, 2025

View reviewed changes

Adding line numbers in editor #940

Are you sure you want to change the base?

Adding line numbers in editor #940

Uh oh!

Conversation

Mash707 commented Feb 10, 2025

Uh oh!

Mash707 commented Feb 10, 2025

Uh oh!

jelly commented Feb 11, 2025

Uh oh!

Mash707 commented Feb 11, 2025

Uh oh!

allisonkarlitskaya commented Feb 11, 2025

Uh oh!

Mash707 commented Feb 11, 2025

Uh oh!

allisonkarlitskaya commented Feb 11, 2025

Uh oh!

Mash707 commented Feb 17, 2025

Uh oh!

garrett commented Feb 18, 2025

Uh oh!

Mash707 commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garrett commented Feb 20, 2025

Uh oh!

jelly commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelly commented Feb 20, 2025

Uh oh!

Mash707 commented Feb 20, 2025

Uh oh!

Mash707 commented Feb 23, 2025

Uh oh!

Mash707 commented Feb 23, 2025

Uh oh!

Mash707 commented Feb 23, 2025

Uh oh!

jelly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mash707 commented Feb 18, 2025 •

edited

Loading

jelly commented Feb 20, 2025 •

edited

Loading