Feat/read only setting #4902

benthie · 2023-10-26T12:09:06Z

📝 Summary

Resolves: Add a setting to open files read-only by default #3923

Hi everybody,

So far the following is implemented:

A global setting open_read_only_enabled has been added, which can be toggled in the same way as workspace_available via command line. Thus, the original functionality remains untouched.
With open_read_only_enabled=true text/markdown files are opened in read-only mode by default and an edit button is shown next to the "Show Outline" button. Clicking on the button makes the file editable, the edit button is replaced by a "Save document" button and the formatting menu bar is shown as in normal mode beneath the button.

While playing around with that new mode, we figured it would be useful to have a possibility to prevent the file from being auto-saved on exit if the user changed the file. Instead, we thought of showing a confirmation dialog in case of unsaved changes asking whether the user wants to save them.
I tried to add such a dialog to the Editor.vue component, but as far as as I understand it, the editor is destroyed by the parent Viewer component and in the editor's beforeDestroy life hook, where the auto-save is performed, I am not able to launch a modal that waits for the user input, because the remaining life hooks are executed immediately after beforeDestroy.
The ImageEditor.vue component of the Viewer app does exactly this, but it uses the FilerobotImageEditor which can be provided with an onClose callback.
Is there any other way we could achieve the same functionality?

I am new to web development and I know that many of the changes I propose here can be improved. I happy about your feedback.

🖼️ Screenshots

Read-only

Editable

🚧 TODO

Menu bar appearance in rich workspaces
Menu bar appearance in text files
SCSS / layout for mobile devices

🏁 Checklist

Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
Sign-off message is added to all commits
Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
Documentation (README or documentation) has been updated or is not required

juliusknorr · 2023-10-30T15:16:41Z

Thanks a lot for your contribution. Implementation wise I only had a first quick look, but this seems reasonable.

I'd be ok having a setting for this if it is covered by a e2e test that verifies it doesn't break, however would also like some confirmation from @nextcloud/designers on this especially given the previous discussion in #3923 which didn't have a common conclusion yet on that topic.

jancborchardt

I’m quite unsure if this is a good idea – this is not something that desktop apps for text or office do, and neither Google Docs, Notion or others, at least by default.

If we do this (which as said I’m leaning against), then there is some feedback for this pull request:

In view-only mode, the "checkmark" save button is also shown, floating in mid-air. This would need to be removed
In edit mode, the "Save" button already exists as the "checkmark" save button on the right. The "Save document" button with the floppy disk icon (outdated, should not be used) is not necessary, also as it introduces another vertical bar

benthie · 2023-11-10T07:48:14Z

Hi @juliushaertl and @jancborchardt,

thank you for your feedback.

Before we put further effort into this PR and try to implement the final parts, we would like to make sure that there exists the chance it will actually get accepted eventually.

We understand that some of you are not convinced by the feature, because comparable editors handle things differently. And adding a new feature always entails maintenance work.

When looking at the votes of the currently open tickets, however, this feature is among the top 10 voted tickets with a handful of people that would be happy to see it coming. And the way it is implemented now, Text will not become an editor that handles things differently compared to other text apps / editors, because the feature is kind of hidden and must be enabled first.

rvjr · 2023-11-11T11:47:49Z

I think if everything is fixed and tests are ok this could be a very helpful addition. And because it's optional now it should not disturb anyone expecting to always edit files directly. At least to me it seemed in #3923 that there might be quite a few other people liking the more explicit editing behaviour. But indeed it depends on @jancborchardt first, before it makes sense to polish everything for a merge...

benthie · 2023-11-27T15:39:20Z

Hi @juliushaertl and @jancborchardt, this is a friendly ping. Is there any progress in deciding whether the feature might get accepted?

benthie · 2023-11-28T09:02:08Z

I’m quite unsure if this is a good idea – this is not something that desktop apps for text or office do, and neither Google Docs, Notion or others, at least by default.

Desktop apps for text or office usually do not to automatically save the document in the background, but instead the user is notified upon closing the document that there are some unsaved changes, offering the possibility to prevent unwanted changes. Web based editors like text, Google Docs and Notion, however, do automatically save and in all of them that is desired behavior (which of course makes sense) and cannot be disabled as far as I know.

juliusknorr · 2023-11-28T09:21:22Z

cc @nimishavijay As Jan is currently out of office :)

jancborchardt

@benthie sorry for the late reply! :) I would be fine with this feature, as long as the config.php setting (open_read_only_enabled as far as I understand?) is false by default. And considering my design feedback from above comment #4902 (review)

juliusknorr · 2024-01-12T11:28:16Z

All right, in order to get this merged I'd definitely require proper tests that cover this feature. I'd suggest to write a cypress e2e test that could make use of the "testing" app in server to set the app value, then load a file and check the new UI parts.

juliusknorr · 2024-01-12T11:31:14Z

src/components/Editor.vue

 				<!-- Rich Menu -->
 				<template v-else>
+					<div v-if="openReadOnlyEnabled" class="text-editor--editable-bar-save">
+						<EditableBarSave />


UI wise I would prefer to not have an extra menu line but rather integrate this into the existing menu bar This could be passed into a slot of the Menubar component

juliusknorr · 2024-01-12T11:31:58Z

src/components/Editor.vue

@@ -457,9 +478,11 @@ export default {
 		onOpened({ document, session }) {
 			this.currentSession = session
 			this.document = document
-			this.readOnly = document.readOnly
+			this.documentReadOnly = document.readOnly


Not a fan of having a copy of the state, can we just use document.readOnly instead in places where this is used?

juliusknorr · 2024-01-12T11:32:32Z

src/components/Editor.vue

@@ -457,9 +478,11 @@ export default {
 		onOpened({ document, session }) {
 			this.currentSession = session
 			this.document = document
-			this.readOnly = document.readOnly
+			this.documentReadOnly = document.readOnly
+			this.editable = !this.documentReadOnly && !this.openReadOnlyEnabled


Maybe call this editMode to be a bit more clear what this value is about?

juliusknorr · 2024-01-12T11:34:31Z

src/components/Menu/entries.js

@@ -65,6 +67,22 @@ export const ReadonlyEntries = [{
 	},
 }]

+export const EditEntries = [{


I think we can just directly hardcode those into the bar components and reduce the complexity a bit here.

benthie · 2024-02-14T14:55:56Z

Hi everybody, I finally have the time to get started working on the requested changes/tests.

Right now I cannot get the cypress e2e tests running. What I do is running the following in the terminal of the devcontainer:

export CYPRESS_baseUrl='http://localhost/index.php'
make test-cypress

which produces this output:

devcontainer@41419b791cd8:/var/www/html/apps-extra/text$ make test-cypress
cd cypress && ./runLocal.sh run
Server is up at http://localhost/index.php

DevTools listening on ws://127.0.0.1:40191/devtools/browser/4d5972cd-f295-4529-8837-f34ab3156995
[32423:0214/143519.296721:ERROR:object_proxy.cc(590)] Failed to call method: org.freedesktop.portal.Settings.Read: object_path= /org/freedesktop/portal/desktop: unknown error type: 
Missing baseUrl in compilerOptions. tsconfig-paths will be skipped
Building undefined undefined 

Your configFile threw an error from: /var/www/html/apps-extra/text/cypress.config.js

The error was thrown while executing your e2e.setupNodeEvents() function:

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:387:5)
    at validateString (node:internal/validators:162:11)
    at Object.join (node:path:1172:7)
    at Object.<anonymous> (/var/www/html/apps-extra/text/node_modules/@nextcloud/webpack-vue-config/webpack.config.js:48:20)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.S (/var/www/html/apps-extra/text/node_modules/tsx/dist/cjs/index.cjs:1:1292)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18)
    at Object.<anonymous> (/var/www/html/apps-extra/text/node_modules/@nextcloud/webpack-vue-config/index.js:23:18)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.S (/var/www/html/apps-extra/text/node_modules/tsx/dist/cjs/index.cjs:1:1292)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18)
    at setupNodeEvents (/var/www/html/apps-extra/text/cypress.config.js:24:27)
    at /home/devcontainer/.cache/Cypress/13.6.3/Cypress/resources/app/node_modules/@packages/server/lib/plugins/child/run_plugins.js:122:14
    at tryCatcher (/home/devcontainer/.cache/Cypress/13.6.3/Cypress/resources/app/node_modules/bluebird/js/release/util.js:16:23)
    at Function.Promise.attempt.Promise.try (/home/devcontainer/.cache/Cypress/13.6.3/Cypress/resources/app/node_modules/bluebird/js/release/method.js:39:29)
    at RunPlugins.load (/home/devcontainer/.cache/Cypress/13.6.3/Cypress/resources/app/node_modules/@packages/server/lib/plugins/child/run_plugins.js:119:9)
    at RunPlugins.runSetupNodeEvents (/home/devcontainer/.cache/Cypress/13.6.3/Cypress/resources/app/node_modules/@packages/server/lib/plugins/child/run_plugins.js:59:17)
    at EventEmitter.<anonymous> (/home/devcontainer/.cache/Cypress/13.6.3/Cypress/resources/app/node_modules/@packages/server/lib/plugins/child/run_require_async_child.js:185:22)
    at EventEmitter.emit (node:events:513:28)
    at EventEmitter.emit (node:domain:489:12)
    at process.<anonymous> (/home/devcontainer/.cache/Cypress/13.6.3/Cypress/resources/app/node_modules/@packages/server/lib/plugins/util.js:33:22)
    at process.emit (node:events:513:28)
./runLocal.sh: line 12: docker-compose: command not found
make: *** [Makefile:32: test-cypress] Error 1

If I do not set the CYPRESS_baseUrl environment variable, it just tells me that it cannot find docker-compose.

devcontainer@41419b791cd8:/var/www/html/apps-extra/text$ make test-cypress
cd cypress && ./runLocal.sh run
No server reached at http://localhost:8081/index.php - starting containers.
./runLocal.sh: line 28: docker-compose: command not found

I looked for information in this section of the readme, but it only says that the user must be in the docker group.

Is there anything I am missing or doing wrong?

juliusknorr · 2024-02-15T21:15:25Z

I think running within the dev container is currently not really working. If you can run it outside of the container you could just use your existing URL of your development environment and start cypress with CYPRESS_baseUrl=https://nextcloud.local/index.php npx cypress open

Hope this helps

benthie · 2024-02-26T17:22:51Z

I think running within the dev container is currently not really working. If you can run it outside of the container you could just use your existing URL of your development environment and start cypress with CYPRESS_baseUrl=https://nextcloud.local/index.php npx cypress open

Hope this helps

Hi @juliushaertl,

that helped indeed, thanks. I've written the e2e tests, but currently the tests with open_read_only_enabled=1 are failing on Chrome, although the feature works fine when I test it manually in Chrome. Both Firefox and Electron are passing all tests. It seems that it crashes when the tests try to change from read-only to edit mode. Do you have any ideas on how to fix that?

If you prefer the commits squashed or with another commit message style, just let me know and I will adapt it.

juliusknorr · 2024-02-28T10:53:00Z

Pushed a small commit that hopefully makes them pass on CI.

I had one chrome crash as well when trying locally but not reproducible anymore later on, so for that I have no real clue for now. However would not consider that a blocker once it passes on CI as some chromium or cypress bug could also be the case there.

benthie · 2024-03-11T10:24:33Z

Hi @juliushaertl, did the tests pass as expected? And is there anything left for me to do?

rvjr · 2024-03-25T14:32:34Z

@juliushaertl, @benthie can we get this closer to merge? I would be really happy to have this on our NC instance...

rvjr · 2024-04-12T08:31:07Z

@juliushaertl I talked to @benthie and according to him all tests pass locally for him; how can we make them pass on CI as well?

max-nextcloud · 2024-04-17T05:50:56Z

@rvjr @benthie First of all thanks a lot for your contribution! ❤️

I'm looking into ci issues right now and trying to make it run more robustly. I will also take a look at the failures here and check if they relate to your changes or are flaky tests for other reasons. I'll let you know if I run into anything you'd need to fix.

github-actions · 2024-04-30T02:07:20Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

benthie · 2024-06-05T09:01:28Z

Hi @max-nextcloud,
hi @juliushaertl,

do you have any updates on the test failures? I could try to reproduce this locally if the failures are related to this PR and if it helps bringing if closer to merging.

As @rvjr already said, we would be happy to have this feature in our NC instance.

max-nextcloud · 2024-12-03T13:56:50Z

Dear @benthie

Sorry for letting this sit here for so long. We're making an effort to catch up on community PRs and I will follow up on this soon.

max-nextcloud · 2024-12-11T16:00:02Z

So first of all I read the backlog. It looks like everything was addressed already. The cypress test failing was in runner 2 and I just fixed a flaky test there - so chances are it may pass now.

I will rebase the PR on current master and then push again to see how the tests behave.

Thanks a lot for your work and sorry for letting this slip through.

codecov · 2024-12-11T16:31:19Z

Codecov Report

Attention: Patch coverage is 45.12195% with 45 lines in your changes missing coverage. Please review.

Project coverage is 46.90%. Comparing base (cd24032) to head (d7f4a75).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/Editor.vue	0.00%	14 Missing ⚠️
src/components/Editor/Wrapper.vue	0.00%	9 Missing ⚠️
src/components/Menu/ReadonlyBar.vue	0.00%	8 Missing ⚠️
src/components/Menu/MenuBar.vue	0.00%	6 Missing ⚠️
...rc/components/Suggestion/LinkPicker/suggestions.js	16.66%	5 Missing ⚠️
src/files.js	0.00%	2 Missing ⚠️
src/public.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4902      +/-   ##
==========================================
+ Coverage   46.86%   46.90%   +0.03%     
==========================================
  Files         747      732      -15     
  Lines       34012    34057      +45     
  Branches     1240     1225      -15     
==========================================
+ Hits        15940    15973      +33     
- Misses      17451    17478      +27     
+ Partials      621      606      -15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

max-nextcloud · 2024-12-11T16:42:40Z

The autoload failure is unrelated to this PR also fails on other PRs now:
https://github.com/nextcloud/text/actions/runs/12259466530?pr=6766

max-nextcloud · 2024-12-13T10:52:40Z

@benthie I adjusted the cypress tests some so they run - but they fail because the setting seems to have no effect.

Were they passing for you before?
Would you have some time to check if it actually still works for you locally?

benthie · 2024-12-13T11:05:16Z

To be honest: I don't remember. But according to my comment above, some tests seemed to work:

[...] that helped indeed, thanks. I've written the e2e tests, but currently the tests with open_read_only_enabled=1 are failing on Chrome, although the feature works fine when I test it manually in Chrome. Both Firefox and Electron are passing all tests. It seems that it crashes when the tests try to change from read-only to edit mode.

juliusknorr

Tested and works, small nitpick inline

juliusknorr · 2024-12-16T08:35:03Z

src/components/Menu/entries.js

+export const ReadOnlyDoneEntries = [{
+	key: 'done',
+	label: t('text', 'Done'),
+	keyChar: 'esc',


This does not seem to work, would drop it for now.

@max-nextcloud I tried to push a commit for this but failed. Maybe you can?

@juliusknorr What exactly did you want to do? drop the keyChar value?

Yes, my guess is that implementing proper esc key support is more tricky as it also may conflict with viewer and keyboard navigation handling, so I'd skip this for now.

fyi

git push [email protected]:benthie/text.git feat/read-only-setting

did the trick for me.

Signed-off-by: Benjamin Thiemann <[email protected]>

Signed-off-by: Julius Härtl <[email protected]>

Signed-off-by: Max <[email protected]>

* Does not work right now. * Would conflict with esc key use for viewer and other keyboard nav. Signed-off-by: Max <[email protected]>

juliusknorr added 3. to review enhancement New feature or request labels Oct 30, 2023

juliusknorr requested review from jancborchardt and juliusknorr October 30, 2023 15:17

jancborchardt reviewed Nov 2, 2023

View reviewed changes

jancborchardt reviewed Jan 9, 2024

View reviewed changes

juliusknorr reviewed Jan 12, 2024

View reviewed changes

benthie force-pushed the feat/read-only-setting branch 2 times, most recently from 44eeccb to 77259a8 Compare February 26, 2024 17:13

juliusknorr requested a review from max-nextcloud February 27, 2024 21:53

github-actions bot added the feedback-requested label Apr 30, 2024

max-nextcloud self-assigned this Dec 3, 2024

max-nextcloud marked this pull request as ready for review December 11, 2024 15:58

max-nextcloud requested a review from mejo- as a code owner December 11, 2024 15:58

max-nextcloud force-pushed the feat/read-only-setting branch from b310305 to 491884a Compare December 11, 2024 16:27

max-nextcloud force-pushed the feat/read-only-setting branch from fbccc51 to 3cfd707 Compare December 13, 2024 09:35

juliusknorr approved these changes Dec 16, 2024

View reviewed changes

juliusknorr reviewed Dec 16, 2024

View reviewed changes

max-nextcloud force-pushed the feat/read-only-setting branch from 95cee60 to 2c7eae3 Compare December 16, 2024 09:26

benthie and others added 12 commits December 16, 2024 16:09

readme: Add documentation for read-only mode

96fd407

Signed-off-by: Benjamin Thiemann <[email protected]>

lib: Add config setting for read-only mode

1cfe0da

Signed-off-by: Benjamin Thiemann <[email protected]>

src: Load config setting for read-only mode

7818ee1

Signed-off-by: Benjamin Thiemann <[email protected]>

components: Add menu entries for open read-only mode

f199022

Signed-off-by: Benjamin Thiemann <[email protected]>

components: Add actions for open read-only mode

c7f0c29

Signed-off-by: Benjamin Thiemann <[email protected]>

cypress: e2e: Add test for open read-only mode

0d587eb

Signed-off-by: Benjamin Thiemann <[email protected]>

ci: Fix running new tests on ci

66e6e6c

Signed-off-by: Julius Härtl <[email protected]>

style: Fix code style in tests

a82e9b8

Signed-off-by: Julius Härtl <[email protected]>

fix(reuse): add license header to test file

709643a

Signed-off-by: Max <[email protected]>

fix(cy): introduce cy.setAppConfig

a4df95a

Signed-off-by: Max <[email protected]>

fix(cy): create user once and upload files once

1083972

Signed-off-by: Max <[email protected]>

fix(keys): drop esc for done in read only mode

d7f4a75

* Does not work right now. * Would conflict with esc key use for viewer and other keyboard nav. Signed-off-by: Max <[email protected]>

max-nextcloud force-pushed the feat/read-only-setting branch from 2c7eae3 to d7f4a75 Compare December 16, 2024 15:10

max-nextcloud enabled auto-merge December 16, 2024 15:11

max-nextcloud merged commit 386f793 into nextcloud:main Dec 16, 2024
66 of 68 checks passed

Feat/read only setting #4902

Feat/read only setting #4902

Conversation

benthie commented Oct 26, 2023

📝 Summary

🖼️ Screenshots

Read-only

Editable

🚧 TODO

🏁 Checklist

juliusknorr commented Oct 30, 2023

jancborchardt left a comment

Choose a reason for hiding this comment

benthie commented Nov 10, 2023

rvjr commented Nov 11, 2023

benthie commented Nov 27, 2023

benthie commented Nov 28, 2023

juliusknorr commented Nov 28, 2023

jancborchardt left a comment

Choose a reason for hiding this comment

juliusknorr commented Jan 12, 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

benthie commented Feb 14, 2024

juliusknorr commented Feb 15, 2024

benthie commented Feb 26, 2024

juliusknorr commented Feb 28, 2024

benthie commented Mar 11, 2024

rvjr commented Mar 25, 2024

rvjr commented Apr 12, 2024

max-nextcloud commented Apr 17, 2024

github-actions bot commented Apr 30, 2024

benthie commented Jun 5, 2024

max-nextcloud commented Dec 3, 2024

max-nextcloud commented Dec 11, 2024

codecov bot commented Dec 11, 2024 • edited Loading

Codecov Report

max-nextcloud commented Dec 11, 2024

max-nextcloud commented Dec 13, 2024

benthie commented Dec 13, 2024

juliusknorr 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

codecov bot commented Dec 11, 2024 •

edited

Loading