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

Added option to disable Edit Locally menu option #41582

Closed
wants to merge 2 commits into from

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Nov 17, 2023

Summary

Many of our users has no installed software for edit file locally. This leads to struggling with mentioned menu option. For me this option also does nothing for any file format. So as an option - we can add a way to disable it completely.

by default:
image

with option

# config/config.php

<?php
$CONFIG = array (
  // ...
  'disable_edit_locally' => true,
);

image

Checklist

@Koc Koc force-pushed the feature/disable-edit-locally branch from 156b160 to a683aeb Compare November 17, 2023 17:06
@AndyScherzinger AndyScherzinger added the 3. to review Waiting for reviews label Nov 17, 2023
@Koc Koc force-pushed the feature/disable-edit-locally branch 6 times, most recently from 8fb985e to e2a35cf Compare November 20, 2023 23:04
@solracsf solracsf added this to the Nextcloud 28 milestone Nov 21, 2023
@Koc
Copy link
Contributor Author

Koc commented Nov 21, 2023

Test failures looks unrelated

@blizzz blizzz mentioned this pull request Nov 22, 2023
5 tasks
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@Koc Koc force-pushed the feature/disable-edit-locally branch 2 times, most recently from 00248c4 to 944aadc Compare November 28, 2023 12:06
@Koc
Copy link
Contributor Author

Koc commented Dec 4, 2023

@blizzz @solracsf @AndyScherzinger hey mates, any chance to review this small PR? 🙏

@solracsf
Copy link
Member

solracsf commented Dec 5, 2023

config.php option should also be documented.

@AndyScherzinger AndyScherzinger added the pending documentation This pull request needs an associated documentation update label Dec 5, 2023
@AndyScherzinger AndyScherzinger requested review from jancborchardt and removed request for PVince81 December 5, 2023 17:01
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

We should move away from PHP template params and use provideInitialState on backend + loadState from @nextcloud/initial-state on frontend

@Koc Koc force-pushed the feature/disable-edit-locally branch from 4e3623e to 8d42af0 Compare December 7, 2023 16:41
@Koc
Copy link
Contributor Author

Koc commented Dec 7, 2023

@solracsf good catch, added notes in config.sample.php

@Pytal oh, I wasnt aware about that but I like it more than jQuery val() 😄 . Fixed + manually re-tested

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Rather than having a switch to disable this altogether, wouldn’t the proper approach here be that the desktop client communicates if a relevant desktop app can be handled? And if the desktop client is not available or installed for the account, then we can disable the entry too (not sure if we already do this).

@tobiasKaminsky @mgallien is this possible?

I understand this is probably more complicated to code, but then there is 0 additional configuration.

@jancborchardt
Copy link
Member

For me this option also does nothing for any file format.

@Koc not even for file formats where you have a handler installed, and you have the Nextcloud desktop client installed? This is a real issue we should fix then for everyone. Simply allowing some people to disable the feature altogether does not fix the original issue unfortunately. :)

@mgallien
Copy link
Contributor

mgallien commented Dec 20, 2023

Rather than having a switch to disable this altogether, wouldn’t the proper approach here be that the desktop client communicates if a relevant desktop app can be handled? And if the desktop client is not available or installed for the account, then we can disable the entry too (not sure if we already do this).

@tobiasKaminsky @mgallien is this possible?

I understand this is probably more complicated to code, but then there is 0 additional configuration.

technically would require (as far as I understand) to have an extension in your browser (like KeePassXC integration or plasma desktop integration) that would be able to communicate with the desktop client (or other software) and check for their presence
feasible and we would know what to do but still a significant effort

Signed-off-by: Konstantin Myakshin <molodchick@gmail.com>
Signed-off-by: Konstantin Myakshin <molodchick@gmail.com>
@Koc Koc force-pushed the feature/disable-edit-locally branch from 823e58e to fef09bc Compare December 20, 2023 23:17
@jancborchardt
Copy link
Member

@mgallien ok, no, requiring a browser extension would not be feasible. I thought we could know whether the browser currently in use and the desktop client are on the same system for example, as they are connected by the server.

Then @sorbaugh @AndyScherzinger cc regarding that off-switch for this feature, seems fine if it is only a config.php setting?

This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
@skjnldsv skjnldsv closed this Aug 3, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress pending documentation This pull request needs an associated documentation update stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants