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

juliushaertl · 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.

juliushaertl · 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)

juliushaertl · 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.

juliushaertl · 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

juliushaertl · 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?

juliushaertl · 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?

juliushaertl · 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?

juliushaertl · 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

Signed-off-by: Benjamin Thiemann <benjamin.thiemann@optonic.com>

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.

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliushaertl · 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.

Signed-off-by: Julius Härtl <jus@bitgrid.net>

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!

rvjr · 2024-05-11T07:52:57Z

@max-nextcloud any updates here? Can we do anything to help?

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

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

jancborchardt reviewed Nov 2, 2023

View reviewed changes

jancborchardt reviewed Jan 9, 2024

View reviewed changes

juliushaertl reviewed Jan 12, 2024

View reviewed changes

benthie added 5 commits February 26, 2024 14:54

readme: Add documentation for read-only mode

e0103bc

Signed-off-by: Benjamin Thiemann <benjamin.thiemann@optonic.com>

lib: Add config setting for read-only mode

378ea45

Signed-off-by: Benjamin Thiemann <benjamin.thiemann@optonic.com>

src: Load config setting for read-only mode

726f278

Signed-off-by: Benjamin Thiemann <benjamin.thiemann@optonic.com>

components: Add menu entries for open read-only mode

3bf84e3

Signed-off-by: Benjamin Thiemann <benjamin.thiemann@optonic.com>

components: Add actions for open read-only mode

d1bb0ae

Signed-off-by: Benjamin Thiemann <benjamin.thiemann@optonic.com>

benthie force-pushed the feat/read-only-setting branch from 958a97b to 44eeccb Compare February 26, 2024 16:45

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

77259a8

Signed-off-by: Benjamin Thiemann <benjamin.thiemann@optonic.com>

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

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

ci: Fix running new tests on ci

c1bcee7

Signed-off-by: Julius Härtl <jus@bitgrid.net>

style: Fix code style in tests

b310305

Signed-off-by: Julius Härtl <jus@bitgrid.net>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/read only setting #4902

Feat/read only setting #4902

benthie commented Oct 26, 2023

juliushaertl commented Oct 30, 2023

jancborchardt left a comment

benthie commented Nov 10, 2023

rvjr commented Nov 11, 2023

benthie commented Nov 27, 2023

benthie commented Nov 28, 2023

juliushaertl commented Nov 28, 2023

jancborchardt left a comment

juliushaertl commented Jan 12, 2024

juliushaertl Jan 12, 2024

juliushaertl Jan 12, 2024

juliushaertl Jan 12, 2024

juliushaertl Jan 12, 2024

benthie commented Feb 14, 2024

juliushaertl commented Feb 15, 2024

benthie commented Feb 26, 2024

juliushaertl 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

rvjr commented May 11, 2024

Feat/read only setting #4902

Are you sure you want to change the base?

Feat/read only setting #4902

Conversation

benthie commented Oct 26, 2023

📝 Summary

🖼️ Screenshots

Read-only

Editable

🚧 TODO

🏁 Checklist

juliushaertl 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

juliushaertl commented Nov 28, 2023

jancborchardt left a comment

Choose a reason for hiding this comment

juliushaertl commented Jan 12, 2024

juliushaertl Jan 12, 2024

Choose a reason for hiding this comment

juliushaertl Jan 12, 2024

Choose a reason for hiding this comment

juliushaertl Jan 12, 2024

Choose a reason for hiding this comment

juliushaertl Jan 12, 2024

Choose a reason for hiding this comment

benthie commented Feb 14, 2024

juliushaertl commented Feb 15, 2024

benthie commented Feb 26, 2024

juliushaertl 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

rvjr commented May 11, 2024