feat: Save current state of a file before downloading it

[silverkszlo]

Resolves https://github.com/issues/assigned?issue=nextcloud-gmbh%7Ccustomer-feature-requests%7C1059

The related PR in Text is here.

Before:

When editing a markdown document online in files and download it from the editor/viewer without saving it manually, the last saved version is downloaded. The last saved version may not include what was just added/edited after the previous save operation. So the downloaded file may differ from the file one sees online.

Now:

Clicking on Download now triggers an automatic save operation of the document. If the saving for some reason fails, the last saved version is still being downloaded.

[skjnldsv]

Hey @silverkszlo is this a bug? feat? Please add labels + milestone accordingly :)

[silverkszlo]

Hey @silverkszlo is this a bug? feat? Please add labels + milestone accordingly :)

Hey @skjnldsv , yes, sorry, this PR is still a draft, all the necessary information are incoming 🙏

[silverkszlo]

@skjnldsv I would be really really grateful if you could review this PR, as this change has been due for quite some time now. The cypress errors seem unrelated?

Also @mejo- , if you could check if I'm using editorComponents the right way? :)

[mejo-]

Tested and works well 😊

[skjnldsv]

Thanks for taking the time, Unfortunately I'm not super keen on approving this. Viewer should stay app-agnostic and be clean of external APIs as much as possible.

Have you considered adding a service on your side that intercept the click ?
You could probably do it with event delegation like document.addEventListener('click', (event) => { and catch any link that have the current document dav url in them ?

[silverkszlo]

Thanks for taking the time, Unfortunately I'm not super keen on approving this. Viewer should stay app-agnostic and be clean of external APIs as much as possible.

Have you considered adding a service on your side that intercept the click ? You could probably do it with event delegation like document.addEventListener('click', (event) => { and catch any link that have the current document dav url in them ?

Thank you for the suggestion! I’ve discussed it with @mejo- , and we think that intercepting the click might be a fragile solution, since it would depend on timing and require the Text app to be aware of Viewer’s internal implementation details.

@mejo- had another idea: adding a preSaveCallback() function to the existing registerHandler(). It could default to an empty function so that existing handlers continue to work without any changes. Text could then save the file, and Viewer would proceed with the download. This way, the callback becomes an explicit part of the existing handler API rather than a side effect. Any app could also use this hook for pre-download logic (e.g., validation, logging, etc.).

It could look like the following in Viewer:

function registerHandler(handler) {
    // Set default empty function if no callback provided
    handler.preSaveCallback = handler.preSaveCallback || (() => {})
    
    // Store the handler
    handlers.push(handler)
}

and:

async onDownload() {
    const handler = this.currentHandler
    
    // Always safe to call - either custom callback or empty function
    await handler.preSaveCallback(this.currentFile)
    
    // Continue with download
    this.performDownload(this.currentFile)
}

What do you think?

[skjnldsv]

I mean, there is also the fact that Viewer is getting re-written for this release cycle, and we'l use the official Files actions. So maybe it's worth ensuring the downloadAction is implementing your solution straight away, and not viewer ?

[silverkszlo]

I mean, there is also the fact that Viewer is getting re-written for this release cycle, and we'l use the official Files actions. So maybe it's worth ensuring the downloadAction is implementing your solution straight away, and not viewer ?

Ah good to know! Okay, then yes it probably makes more sense to adjust the action. What do you think @juliusknorr ?

[silverkszlo]

I mean, there is also the fact that Viewer is getting re-written for this release cycle, and we'l use the official Files actions. So maybe it's worth ensuring the downloadAction is implementing your solution straight away, and not viewer ?

As this will have to be backported anyway, being a customer request and kind of urgent, I'd suggest to go with the handler approach now and refactor/change/delete it after the rewrite is complete?

[juliusknorr]

Ah good to know! Okay, then yes it probably makes more sense to adjust the action. What do you think @juliusknorr ?

I'd say the current approach is then good for stable32, but I agree that for master we still need to adjust then to make this work with the file action after the rewrite bits.

[juliusknorr]

Looks good and works well in combination with the companion branch in text

[juliusknorr]

Cypress seems unrelated, rebased to rerun CI again

[silverkszlo]

/backport to stable32 please

[silverkszlo]

/backport to stable31 please

[silverkszlo]

/backport to stable30 please