Implement viewer/editor for text files #509

allisonkarlitskaya · 2024-06-05T15:24:34Z

This can give us a bit of a model for playing around with.

jelly · 2024-06-05T15:33:32Z

I did some research regarding editing and my conclusions are:

Limit the to be edited filesize, we can't load everything into the browser. The goal is editing config files so I looked up the max size of a config file in /etc/

37540   /etc/libvirt/qemu.conf
38661   /etc/nginx/mime.types
106943  /etc/valkey/valkey.conf
107546  /etc/redis/redis.conf
114331  /etc/lvm/lvm.conf

Show and maybe toggle line numbers?
Optionally syntax highlight but that is a follow up
Use fsreplace1 for saving the file.

Design is here: #471 (comment)

allisonkarlitskaya · 2024-06-05T17:46:12Z

lol totally missed #107 sorry!

garrett · 2024-06-06T07:47:54Z

Show and maybe toggle line numbers?

I think it's fine to just show them, provided they work properly. They're useful when having wrapping on (which is necessary when in a browser, as to avoid having to scroll horizontally). Without line numbers, you're not sure if a line is a wrapped one or two separate lines, which could break a configuration. Without wrapping, you can't see the context of a long line.

So I'd suggest wrapping and line numbers (that support wrapping), without toggles.

garrett · 2024-06-06T08:16:26Z

There's a full path in the title; it should just be the filename.
Why is the edit area so tiny?
The text editor is a proportional font; it should be monospace
Nothing but "Cancel" should be a link styled button. Either we need to change the string to "Cancel" or make "Close" to a secondary button. (Related: "Close" should never be a primary, even if it is the only action; it should always be secondary.)

Here are the mockups (same as in the mockup discussion):

They're subject to change, of course.

I guess we should decide on whether we want to go with this implementation, the one in #107, or another.

garrett · 2024-06-18T14:44:25Z

I've updated the textarea with line numbers demo @ https://codepen.io/garrett/pen/qBGPzWZ

allisonkarlitskaya · 2024-06-18T15:20:39Z

I've updated the textarea with line numbers demo @ https://codepen.io/garrett/pen/qBGPzWZ

I played with this a bit and read the code. After understanding what it does I set out to try breaking it, and came up with this:

Screencast.from.2024-06-18.17-05-45.webm

This is caused by your line-computation code assuming that a word can only be split on a space character, whereas the browser is happy to wrap after a hyphen. We could add hyphens, but then we'd have to worry about unicode hyphens. And other things that a browser may or may not do. It feels a bit too cat-and-mouse for my tastes.

Another approach is to actually ask the browser how it would wrap a given block of text with the given styling options. https://github.com/component/textarea-caret-position is a evil bit of work that does exactly that. I don't think the approach they take would be directly useful to us but I could see some sort of a cached form of it where we measure each (logical) line for its physical height... that way on typing we'd only have to recompute a single line...

It all seems a bit complicated, though. I'm starting to think we should punt on line numbers for the time being...

garrett · 2024-06-19T13:31:28Z

It all seems a bit complicated, though. I'm starting to think we should punt on line numbers for the time being...

Right, agreed. The demo was intended to see if we could use a text area with line numbers and how well it'd work.

It could also create an invisible element with the same formatting (font, width, white-space: pre-wrap) and a line-height of 1, find the height once with filler text (just any character would be enough), then per line: inject the text, find the height, divide the height by the original height, and we'd know how tall each line is. The rest of the demo would work the same. This way, we'd inherit the exact line-wrapping rules. It could skip lines that have < the fixed width of characters (80ch) as just 1 line tall and it could just update when the height of the textarea changes (instead of on keypress).

Not sure if this would be better than the text caret method or not, but we could combine ideas or even possibly clone the textarea in a fragment and manipulate the selection in the copy perhaps (if we can measure the size of the selection somehow).

But, yeah, stuff for later. For now? Textarea without anything fancy.

jelly

I haven't tested it but it seems good. I think to land this we need:

Tests
Only open files which are < 1MB (We obviously can't edit big files easy, load them in the browser and crash)

My one open question is, does fsreplace1 retain file permissions? If an admin edits /etc/fstab does that save as admin:admin or whatever it was?

jelly · 2024-06-19T15:20:06Z

src/dialogs/editor.tsx

+    Modal, ModalVariant,
+    Stack,
+    TextArea,
+} from '@patternfly/react-core';


This will need to be converted to esm imports.

jelly · 2024-06-19T15:33:15Z

src/dialogs/editor.tsx

+
+    React.useEffect(() => {
+        const before_unload = (event: BeforeUnloadEvent) => {
+            event.preventDefault();


I think this lacks things for older browsers, everywhere in Cockpit we this as well.

// Included for legacy support, e.g. Chrome/Edge < 119 event.returnValue = true;

jelly · 2024-06-19T15:37:58Z

src/dialogs/editor.tsx

+        const before_unload = (event: BeforeUnloadEvent) => {
+            event.preventDefault();
+        };
+        if (modified) {


So this seems to add the beforeunload every time modified is set and EditFileModal is re-rendered?

This feels rather over-engineerd with useEffect. We could useInit and then either check in the eventlistener if the file was modified and then call preventDefault() if needed. (Or just always it really doesn't hurt that much)

Especially as we already have modified

jelly · 2024-06-19T15:41:33Z

src/dialogs/editor.tsx

+                  id='editor-text-area'
+                  className='file-editor'
+                  value={content || ''}
+                  onChange={modify}


it probably makes sense to also disabled the textarea when there is an error isDisabled={error !== null}

jelly · 2024-06-19T15:53:02Z

file permissions are not retained:

[admin@fedora-40-127-0-0-2-2201 ~]$ sudo chown admin:users bwjiejowier
[admin@fedora-40-127-0-0-2-2201 ~]$ ls -lh bwjiejowier
-rw-r--r--. 1 admin users 229 Jun 19 15:51 bwjiejowier

Edit and save

[admin@fedora-40-127-0-0-2-2201 ~]$ ls -lh bwjiejowier
-rw-r--r--. 1 admin admin 234 Jun 19 15:52 bwjiejowier

Coreutils does this correctly.

[admin@fedora-40-127-0-0-2-2201 ~]$ sudo chown admin:users bwjiejowier
[admin@fedora-40-127-0-0-2-2201 ~]$ echo 'wefw' >> bwjiejowier
[admin@fedora-40-127-0-0-2-2201 ~]$ ls -lh bwjiejowier
-rw-r--r--. 1 admin users 239 Jun 19 15:52 bwjiejowier

The main use-case editing config files does not work:

garrett · 2024-06-25T09:07:41Z

When you're not allowed to edit a file but you are able to view it still, why is the text area still interactive? It should be marked as read-only. https://www.patternfly.org/components/forms/text-area/#read-only

...And why does it have a star in the title?

And what's with the error message? What action? It really should say it more straightforward. Does it mean that you're not allowed to edit the file? That shouldn't be an error, but some information; it shoulnd't be an alert.

The design is a bit unusual in comparison with our other components, but keeping track of the complex state interactions is difficult enough on its own, without getting React involved, so we use a separate class. Thanks to Garrett for help with the styling.

allisonkarlitskaya · 2024-06-28T18:57:02Z

Since last round:

implementation moved to its own class — the state interactions are too complicated here to also get tangled up in react.
"view mode" for files if they're readonly
detection of changes to the file on disk (and a reload/overwrite banner)
other minor cleanups

cockpituous · 2024-06-28T19:00:38Z

src/dialogs/editor.tsx

+class EditorState {
+    error: string | null = null; // if there is an error to show
+    modified: boolean = false; // if there are unsaved changes
+    saving: boolean = false; // saving in progress?
+    tag_at_load: string | null = null; // the one we loaded
+    tag_now: string | null = null; // the one on disk
+    content: string = '';
+    writable: boolean = false;


These 8 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:38Z

src/dialogs/editor.tsx

+    update(updates: Partial<EditorState>) {
+        Object.assign(this.state, updates);
+        this.emit('updated', { ...this.state });


These 3 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:38Z

src/dialogs/editor.tsx

+    modify(content: string) {
+        this.update({ content, modified: true });


These 2 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:38Z

src/dialogs/editor.tsx

+        this.update({ content, modified: true });
+    }
+
+    load_file() {


This added line is not executed by any test.

cockpituous · 2024-06-28T19:00:38Z

src/dialogs/editor.tsx

+        this.file.read()
+                .then(((content: string, tag: string) => {
+                    this.update({ content, tag_now: tag, tag_at_load: tag });
+                }) as any /* eslint-disable-line @typescript-eslint/no-explicit-any */)
+                .catch(error => {
+                    this.update({ error: cockpit.message(error) });


These 6 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:39Z

src/dialogs/editor.tsx

+                          <AlertActionLink onClick={() => editor && editor.load_file()}>
+                              {_("Reload file (abandon our changes)")}
+                          </AlertActionLink>
+                          <AlertActionLink onClick={() => editor && editor.save()}>
+                              {_("Overwrite with our changes")}
+                          </AlertActionLink>


These 6 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:40Z

src/dialogs/editor.tsx

+                <TextArea
+                  id='editor-text-area'
+                  className='file-editor'
+                  isDisabled={!state.writable}
+                  value={state.content}
+                  onChange={(_ev, content) => editor && editor.modify(content)}


These 6 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:40Z

src/dialogs/editor.tsx

+            </Stack>
+        </Modal>


These 2 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:40Z

src/dialogs/editor.tsx

+export function edit_file(dialogs: Dialogs, path: string) {
+    dialogs.run(EditFileModal, { path });


These 2 added lines are not executed by any test.

cockpituous · 2024-06-28T19:00:40Z

src/menu.tsx

+                {
+                    id: "open-file",
+                    title: _("Open text file"),
+                    onClick: () => edit_file(dialogs, `${currentPath}${item.name}`)


This added line is not executed by any test.

garrett · 2024-07-01T13:46:33Z

We probably want these two CSS lines to make the text editor more reliable for editing files:

textarea {
  white-space-collapse: break-spaces;
  hyphens: none;
  /* ... */
}

(...this also happens to make my numbers demo more reliable too, but is useful with a plain textarea too.)

martinpitt

I was curious and had a first look 😁

martinpitt · 2024-07-03T05:16:35Z

src/dialogs/editor.tsx

+
+    load_file() {
+        // Can't do this async because we can't get the tag via await
+        this.file.read()


The code below limits this to 1 MB, but for belt-and-suspenders this should get a max_read_size. The default 16 MB is way too high for this purpose.

martinpitt · 2024-07-03T05:30:41Z

src/dialogs/editor.tsx

+        this.file.watch((_content, tag_now) => {
+            this.update({ tag_now });
+        }, { read: false });


This needs a debounce treatment (I recommend the usual 500 ms). These watch events often fire in batches, and we don't want to load the file and re-render the editor several times.

martinpitt · 2024-07-03T05:32:26Z

src/dialogs/editor.tsx

+        if (!this.state.tag_now)
+            return;


Is this a "Should Not Happen"™ situation? then please console.error so that it breaks our tests when it runs into that. Or does that happen for creating a new file? (IIRC we didn't want to support that). But empty files have a - tag, so this smells more like "not yet initialized"?

martinpitt · 2024-07-03T05:35:38Z

src/dialogs/editor.tsx

+            const tag = await this.file.replace(this.state.content, this.state.tag_now);
+            this.update({ saving: false, modified: false, tag_now: tag, tag_at_load: tag });
+        } catch (exc: any /* eslint-disable-line @typescript-eslint/no-explicit-any */) {
+            this.update({ error: cockpit.message(exc), saving: false });


There's two common error cases here, right? access-denied, which cockpit.message() translates to "Not permitted to perform this action.". That's a bit confusing for the case of "You don't have permissions to edit this file. This should already be intercepted by the test -w state, so maybe that's just a corner case.

The other is "the file changed underneath your edit", and I don't think we have a matching error code, at least none of the ones that cockpit.message() knows about is appropriate. So this needs some fine tuning.

martinpitt · 2024-07-03T05:38:07Z

src/dialogs/editor.tsx

+          variant={ModalVariant.large}
+          className='file-editor-modal'
+          footer={
+              state?.writable &&


The dialog should have a "Close" button for non-writable files.

martinpitt · 2024-07-03T05:38:29Z

src/dialogs/editor.tsx

+                              {_("Reload file (abandon our changes)")}
+                          </AlertActionLink>
+                          <AlertActionLink onClick={() => editor && editor.save()}>
+                              {_("Overwrite with our changes")}


martinpitt · 2024-07-03T05:40:20Z

src/dialogs/editor.tsx

+                  title="The file has changed on disk"
+                  actionLinks={
+                      <>
+                          <AlertActionLink onClick={() => editor && editor.load_file()}>


The buttons shouldn't even appear (or be isDisabled) if the editor component is initialized asynchronously here. An enabled button should never do a no-op on click.

(Aside from that, in other situations this is better written as editor?.load_file(), but it doesn't apply here)

martinpitt · 2024-07-03T05:42:16Z

src/menu.tsx

@@ -91,38 +92,49 @@ export function get_menu_items(
            }
        );
    } else if (selected.length === 1) {
+        const item = selected[0];
+
+        if (item.type === 'reg' && item.size && item.size < 1000000) // 1MB


Bikeshedding: This simple editor isn't really suitable for such large files, especially if they are binary. This feels more like in the "kB" magnitude.

martinpitt · 2024-07-03T05:43:29Z

src/menu.tsx

+            menuItems.push(
+                {
+                    id: "open-file",
+                    title: _("Open text file"),


How do you know it's a text file? There's no check if it's a binary file. That would be a good idea, but other than that it feels less confusing to just say "Edit file"?

martinpitt · 2024-07-03T05:44:10Z

src/menu.tsx

+        if (item.type === 'reg' && item.size && item.size < 1000000) // 1MB
+            menuItems.push(
+                {
+                    id: "open-file",


And dito here -- this is "edit". There's other ways how you can "open" a file (image viewer, run executable, render HTML in a new tab, etc.)

allisonkarlitskaya requested review from garrett and jelly June 5, 2024 15:24

allisonkarlitskaya force-pushed the editor branch from 85b891a to 9f5f0be Compare June 6, 2024 08:51

jelly added the release-note label Jun 7, 2024

allisonkarlitskaya force-pushed the editor branch from 2722475 to 1e1ebc5 Compare June 12, 2024 08:14

cockpit-project deleted a comment from cockpituous Jun 19, 2024

jelly reviewed Jun 19, 2024

View reviewed changes

allisonkarlitskaya mentioned this pull request Jun 25, 2024

Add basic text editor using textarea #107

Closed

2 tasks

allisonkarlitskaya force-pushed the editor branch from 1e1ebc5 to b719d1d Compare June 28, 2024 18:52

allisonkarlitskaya changed the title ~~Add a toy editor dialog based on TextArea~~ Implement viewer/editor for text files Jun 28, 2024

allisonkarlitskaya requested a review from jelly June 28, 2024 18:52

allisonkarlitskaya added 2 commits June 28, 2024 20:56

HACK USE UNOFFICIAL COCKPITLIB

0bf271a

allisonkarlitskaya force-pushed the editor branch from b719d1d to 0bf271a Compare June 28, 2024 18:56

cockpituous reviewed Jun 28, 2024

View reviewed changes

garrett added 3 commits July 2, 2024 17:39

editor: Change modified styling

283f284

editor: Prevent hyphens and use break-spaces

bc06dae

editor: Switch between close (non-modified) and cancel (modified)

98ae5f5

martinpitt requested changes Jul 3, 2024

View reviewed changes

		modify(content: string) {
		this.update({ content, modified: true });

		export function edit_file(dialogs: Dialogs, path: string) {
		dialogs.run(EditFileModal, { path });

Implement viewer/editor for text files #509

Are you sure you want to change the base?

Implement viewer/editor for text files #509

Conversation

allisonkarlitskaya commented Jun 5, 2024

jelly commented Jun 5, 2024

allisonkarlitskaya commented Jun 5, 2024

garrett commented Jun 6, 2024

garrett commented Jun 6, 2024

garrett commented Jun 18, 2024

allisonkarlitskaya commented Jun 18, 2024

garrett commented Jun 19, 2024

jelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Jun 19, 2024 • edited Loading

garrett commented Jun 25, 2024 • edited Loading

allisonkarlitskaya commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett commented Jul 1, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Jun 19, 2024 •

edited

Loading

garrett commented Jun 25, 2024 •

edited

Loading