Text-editor extensions #7967

diocas · 2022-11-14T18:14:51Z

Sets canBeDefault: true to ensure that files are not downloaded first.

Adds a new configuration parameter that allows us to open a configurable number of other extensions.

diocas · 2022-12-02T16:00:02Z

It looks like canBeDefault: true is preventing us from opening .md on codimd by default... @elizavetaRa can you check please? Maybe instead of using that, what we need to do is ensuring that the download action is always the last (so any app takes precedence)

kulmann · 2022-12-05T08:20:50Z

It looks like canBeDefault: true is preventing us from opening .md on codimd by default... @elizavetaRa can you check please? Maybe instead of using that, what we need to do is ensuring that the download action is always the last (so any app takes precedence)

Yes, at the moment an app that handles a specific file extension explicitly (like the text-editor that explicitly handles .txt and .md) would have precedence over the external app. The perfect solution is probably to let the user choose which app they want to use for a specific file type.

diocas · 2022-12-05T12:37:50Z

That would be the best, but I would be surprised if we could have that in the short term, or am I wrong? :)

I've asked @elizavetaRa to have a look and suggested we order the actions like following: 1st canBeDefault, 2nd all remaining apps, 3rd system actions (like download). Then we can always execute the first one on the list.

To me it makes more sense that we use an app if one is registered. What do you think, @kulmann ?

kulmann · 2022-12-05T13:47:34Z

That would be the best, but I would be surprised if we could have that in the short term, or am I wrong? :)

Yes, nothing for short term.
One could store the user choice in the browsers localStorage, like we already do for other user choices (e.g. showing/hiding hidden files, showing/hiding file extensions in file names). My team can't work on that at the moment.

I've asked @elizavetaRa to have a look and suggested we order the actions like following: 1st canBeDefault, 2nd all remaining apps, 3rd system actions (like download). Then we can always execute the first one on the list.

To me it makes more sense that we use an app if one is registered. What do you think, @kulmann ?

The name canBeDefault would be pretty misleading then, as it would just be a boolean way of prioritizing certain apps over other apps. But I agree with the approach. If we have an app for a mime type, it doesn't make sense to ignore it when trying to open a file.

diocas · 2022-12-05T15:45:32Z

The name canBeDefault would be pretty misleading then, as it would just be a boolean way of prioritizing certain apps over other apps.

Isn't this what we're doing already in a way? Should we rename it?

kulmann · 2022-12-05T20:25:31Z

The name canBeDefault would be pretty misleading then, as it would just be a boolean way of prioritizing certain apps over other apps.

Isn't this what we're doing already in a way? Should we rename it?

Yes and yes 😄 Do you have a proposal for a better var name?

diocas · 2022-12-06T08:49:58Z

priority? Or too generic?

kulmann · 2022-12-06T09:53:01Z

works for me, priority or hasPriority? (I prefer hasPriority but don't care too much).

So as a result: we want to sort all the apps and external-apps in one pool by following criteria:

hasPriority=true first, hasPriority=false / absent last
"dedicated" apps first, external-apps last
by name

Correct?

elizavetaRa · 2022-12-06T13:34:54Z

works for me, priority or hasPriority? (I prefer hasPriority but don't care too much).

So as a result: we want to sort all the apps and external-apps in one pool by following criteria:

hasPriority=true first, hasPriority=false / absent last

"dedicated" apps first, external-apps last

by name

Correct?

sounds good! The priority sort should apply after the "external apps last" sort

kulmann · 2023-02-09T16:24:40Z

sounds good! The priority sort should apply after the "external apps last" sort

Ok. What's the next step, can you take care of finishing this PR?

sonarcloud · 2023-03-24T09:34:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

ownclouders · 2023-03-24T09:52:33Z

Results for e2e-tests oCIS https://drone.owncloud.com/owncloud/web/36303/12/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36303/tracing/public-link-for-space-alice-2023-6-6-08-49-46.zip

💥 The e2e-ocis tests pipeline failed. The build has been cancelled.

diocas · 2023-03-24T11:09:13Z

@kulmann please let us know if you prefer to split the PR into text-editor stuff and the more general default extensions handling.

elizavetaRa · 2023-03-24T12:53:36Z

@kulmann and please let us know if you see the use case to implement hasPriority for internal editors as well. Right now there are no internal editors that are in conflict with each other to open the same extension, so the internal editor will be taken first unless there is a hasPriority for external app. But maybe something is planned for the future?

kulmann · 2023-04-27T20:36:09Z

packages/web-pkg/src/composables/actions/types.ts

@@ -12,6 +12,7 @@ export interface Action<T = ActionOptions> {
  componentType: 'button' | 'router-link'
  class: string
  canBeDefault?: boolean


Hey @diocas and @elizavetaRa

my intend (see discussion, I think yours as well at some point) was to rephrase canBeDefault to hasPriority. Meaning, having both in parallel now makes things unnecessarily complex. 🙈

On top of that: I think the approach with the app provider - define via config which app has priority - is the way to go. An extension developer should never be the one who decides which application has priority over another one. The admin has to decide that, so that's configuration only, not code.

Could you look into realizing the following?

get rid of canBeDefault entirely

introduce hasPriority but only through configuration, nothing hardcoded. For the app provider apps it's already there with the default_application attribute on mime types.

determine the app that gets opened on click in file list (= "default app") by throwing all standalone apps and all app provider apps into one list, sort them by priority, then by internal vs app provider, and pick the first.

What do you think? Does that make sense?

@diocas @elizavetaRa anything we can do to support you here? Do you have feedback or further need for clarification?

@elizavetaRa has been a bit busy. I'll check with her tomorrow when she can fit this in her tasks.

@diocas if you're generally fine with what Benedikt suggested I could also take over?

@pascalwengerter @kulmann I'll take this issue, will update the pr these days, thanks for waiting!

@kulmann 2 questions:

what is the purpose of "canBeDefault" in terms priority in the composables/actions files like web-app-files/src/composables/actions/files/useFileActionsCreateNewFile.ts?

"introduce hasPriority but only through configuration, nothing hardcoded." You mean move setting hasPriority of all the extensions, also editors to config.json instead of index.ts of each extension?

hey @elizavetaRa - cool that you are working on this, thank you! 🤩

in the case of file action composables, canBeDefault doesn't belong there. Could be removed entirely, doesn't make sense.

yes, that's exactly what I meant. :-) Please note that providing config for an app is not possible in the apps array of the config.json, but only for the external_apps array. That one is a bit misleading, name wise, but that's fine for now. Only means that if an admin wants to give an app priority over another one for the same file type they need to define the app in external_apps. We need to clean that up (big time), but I wouldn't do that in the scope of this PR. So please just keep the assumption that providing config for an app is only possible in the external_apps array of the config.json. That's fine.

@kulmann @pascalwengerter Following example of config.json with use case: extension A (not external app) opens file types.aaa and .txt. Type .txt should have hasPriority over internal text editor. Would it look something like that?

"external_apps": [ ..., { "id": "A", "path": "/aaa", "config": { "fileExtensions": [ { "extension": "txt", "hasPriority": true } ] } } ],

Or should we call the option something like "priorityExtensions" and set priority to all file types that are in this list?

So proposal 2 for config.json:

"external_apps": [ ..., { "id": "A", "path": "/aaa", "config": { "priorityExtensions": ["txt"] ] } } ],

tbsbdr · 2023-06-01T11:20:53Z

@elizavetaRa you will take care of this, right?

elizavetaRa · 2023-06-01T13:06:28Z

@tbsbdr on it, yes

pascalwengerter · 2023-06-05T14:04:47Z

@elizavetaRa could you add a changelog item? :)

kulmann

One tiny nitpick (see comment), otherwise works like a charm 👍

kulmann · 2023-06-07T08:43:04Z

packages/web-app-text-editor/src/index.ts

  let primaryExtensions = (window as any).__$store.getters.extensionConfigByAppId(appId)
    .primaryExtensions || ['txt', 'md']
+
+  let extraExtensions = config.extraExtensions || []


the extraExtensions are never re-used after they got pushed into the extensions array. So I'd be in favour of the following one liner to keep it shorter / without the extraExtensions variable assignment:

extensions.push(...(config.extraExtensions || []).map((ext) => ({ extension: ext })))

Also, please move that line directly below const config = ... and above let primaryExtensions = ... for following logical grouping:

assign all possible extensions into the extensions array.

define primaryExtensions

return the extensions in the correct format

sonarcloud · 2023-06-07T12:31:56Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

0.0% Coverage
0.0% Duplication

kulmann

nice, thank you for taking care! 💪

tbsbdr · 2023-06-13T08:40:51Z

relates to 323030a

diocas added the Early-Adopter:CERN label Nov 14, 2022

diocas force-pushed the up_text_editor_extensions branch from efc2f8a to 50bc489 Compare November 14, 2022 18:15

elizavetaRa force-pushed the up_text_editor_extensions branch 2 times, most recently from d3d76bd to 42f1656 Compare March 24, 2023 08:33

kulmann reviewed Apr 27, 2023

View reviewed changes

elizavetaRa force-pushed the up_text_editor_extensions branch from 42f1656 to ca5a310 Compare May 23, 2023 14:34

elizavetaRa force-pushed the up_text_editor_extensions branch from ca5a310 to 586dfe7 Compare June 1, 2023 09:54

elizavetaRa force-pushed the up_text_editor_extensions branch from 2b7b602 to bd3ddce Compare June 5, 2023 08:37

hasPriority instead of canBeDefault

06bead3

elizavetaRa force-pushed the up_text_editor_extensions branch from bd3ddce to 06bead3 Compare June 5, 2023 08:53

add changelog

9edf63f

kulmann requested changes Jun 7, 2023

View reviewed changes

implement suggested code refactoring

019e3a8

kulmann approved these changes Jun 7, 2023

View reviewed changes

kulmann merged commit 4529f06 into owncloud:master Jun 7, 2023

tbsbdr mentioned this pull request Jun 26, 2023

[CERN] Buckets for Web Merge #9278

Closed

37 tasks

tbsbdr added this to the CERN Web Merge milestone Jul 10, 2023

tbsbdr added the Platform:Web label Jul 14, 2023

micbar mentioned this pull request Jul 24, 2023

Release 4.0.0 owncloud/ocis#6838

Closed

68 tasks

diocas deleted the up_text_editor_extensions branch July 24, 2023 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Text-editor extensions #7967

Text-editor extensions #7967

diocas commented Nov 14, 2022

diocas commented Dec 2, 2022

kulmann commented Dec 5, 2022

diocas commented Dec 5, 2022

kulmann commented Dec 5, 2022

diocas commented Dec 5, 2022

kulmann commented Dec 5, 2022

diocas commented Dec 6, 2022

kulmann commented Dec 6, 2022

elizavetaRa commented Dec 6, 2022

kulmann commented Feb 9, 2023

sonarcloud bot commented Mar 24, 2023

ownclouders commented Mar 24, 2023 •

edited

Loading

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36303/tracing/public-link-for-space-alice-2023-6-6-08-49-46.zip

diocas commented Mar 24, 2023

elizavetaRa commented Mar 24, 2023

kulmann Apr 27, 2023

pascalwengerter May 16, 2023

diocas May 16, 2023

pascalwengerter May 17, 2023

elizavetaRa May 22, 2023

elizavetaRa May 23, 2023

kulmann May 23, 2023

elizavetaRa Jun 1, 2023 •

edited

Loading

elizavetaRa Jun 1, 2023 •

edited

Loading

tbsbdr commented Jun 1, 2023

elizavetaRa commented Jun 1, 2023

pascalwengerter commented Jun 5, 2023

kulmann left a comment

kulmann Jun 7, 2023

sonarcloud bot commented Jun 7, 2023

kulmann left a comment

tbsbdr commented Jun 13, 2023

Text-editor extensions #7967

Text-editor extensions #7967

Conversation

diocas commented Nov 14, 2022

diocas commented Dec 2, 2022

kulmann commented Dec 5, 2022

diocas commented Dec 5, 2022

kulmann commented Dec 5, 2022

diocas commented Dec 5, 2022

kulmann commented Dec 5, 2022

diocas commented Dec 6, 2022

kulmann commented Dec 6, 2022

elizavetaRa commented Dec 6, 2022

kulmann commented Feb 9, 2023

sonarcloud bot commented Mar 24, 2023

ownclouders commented Mar 24, 2023 • edited Loading

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36303/tracing/public-link-for-space-alice-2023-6-6-08-49-46.zip

diocas commented Mar 24, 2023

elizavetaRa commented Mar 24, 2023

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

elizavetaRa Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

elizavetaRa Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

tbsbdr commented Jun 1, 2023

elizavetaRa commented Jun 1, 2023

pascalwengerter commented Jun 5, 2023

kulmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 7, 2023

kulmann left a comment

Choose a reason for hiding this comment

tbsbdr commented Jun 13, 2023

ownclouders commented Mar 24, 2023 •

edited

Loading

elizavetaRa Jun 1, 2023 •

edited

Loading

elizavetaRa Jun 1, 2023 •

edited

Loading