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.

Sign up for GitHub

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

Jump to bottom

Implement direct editing API #2405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Implement direct editing API #2405

wants to merge 1 commit into from

Conversation

Raudius
Copy link
Contributor

@Raudius Raudius commented Aug 25, 2022

Summary

Implements the direct editing API for mimetypes supporrted by Collabora.

TODO

  • Test on desktop client
  • Test on mobile clients

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required

@Raudius Raudius added enhancement New feature or request 2. developing Work in progress labels Aug 25, 2022
@Raudius Raudius self-assigned this Aug 25, 2022
@Raudius Raudius mentioned this pull request Aug 25, 2022
Open
juliusknorr
juliusknorr reviewed Sep 21, 2022
View reviewed changes
lib/DirectEditing/PresentationCreator.php Outdated
}

public function getExtension(): string {
return 'odp';
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to check for the preferred default document format Open Document Format or Office Open XML and pick the extension / mime type depending on that as done here:

richdocuments/lib/TemplateManager.php

Line 459 in 9bb9661

$ooxml = $this->config->getAppValue(Application::APPNAME, 'doc_format', '') === 'ooxml';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Added this to the text/presetnation/spreadsheet creators.

Unsure what to do with the graphics one, as far as I know there is no odg equivalent in Microsoft Office.

@juliusknorr juliusknorr self-requested a review October 11, 2022 08:48
@juliusknorr
Copy link
Member

  • Test with desktop and android
  • Ping Tobi about that we need to disable the old mechanism in android
  • Check back with Marino about iOS

juliusknorr
juliusknorr reviewed Oct 13, 2022
View reviewed changes
lib/DirectEditing/DirectEditor.php Outdated
public function open(IToken $token): Response {
$token->useTokenScope();
try {
$session = $this->apiService->create($token->getFile()->getId());
Copy link
Member

Choose a reason for hiding this comment

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

Seems there is still some logic missing here to make this work ;)

@juliusknorr
Copy link
Member

@Raudius I released and pushed a small typo fix for the namespace, but seems this is not in a functional state yet. Since you mentioned testing it locally, maybe you didn't push your recent changes? :)

@Raudius
Copy link
Contributor Author

Raudius commented Oct 13, 2022

@juliushaertl Looks like I messed up when pushing this 🙈 Had to redo some of the changes but should be back to what I had before.

Seems to work on the desktop client for me, but still unable to get the direct edit view to load on Andorid.

@juliusknorr
Copy link
Member

juliusknorr commented Oct 13, 2022 •
edited
Loading

I think we need a few more alignments for android/iOS.

Currently the server side direct editing uses different post messages for emitting a status, so we probably should unify, standardise and document what we use and what is expected.

For now only a writeup on what is currently there:

https://github.com/nextcloud/text/blob/master/src/views/DirectEditing.vue#L50-L83

Richdocuments direct editing (old) (emitted by the web app)

https://github.com/nextcloud/richdocuments/search?q=callMobileMessage

  • documentLoaded
  • downloadAs
  • fileRename
  • paste
  • close ✅
  • insertGraphic
  • share ✅
  • hyperlink(args)

Server direct editing (emitted by the web app)

https://github.com/nextcloud/text/blob/master/src/views/DirectEditing.vue#L107-L124

  • loading
  • loaded
  • share
  • close

Interfaces called by the clients

https://sourcegraph.com/search?q=context:global+OCA.RichDocuments.documentsMain.&patternType=standard

  • OCA.RichDocuments.documentsMain.postAsset(filename, url)
  • OCA.RichDocuments.documentsMain.postGrabFocus()
  • OCA.RichDocuments.documentsMain.onClose()

Related text app issue nextcloud/text#1170

@juliusknorr
Copy link
Member

  • start by emitting loaded next to documentLoaded in richdocuments
  • Create a npm package to standardise the postMessages that are exchanged between editor and mobile apps
    • Migrate "Interfaces called by the clients" to post messages being triggered by the mobile apps through window.postMessage()
      • For the client called methods we either require them (onClose) or they can be skipped by editors (OCA.RichDocuments.documentsMain.postGrabFocus / OCA.RichDocuments.documentsMain.postAsset)
    • Move postMessage parsing from https://github.com/nextcloud/richdocuments/blob/81b09056fdcc7d3a845bf1ba0c2e7f4d179d1c10/src/helpers/mobile.js over to the library
    • Library API would need methods to
      • emit a message to mobile apps (callMobileMessage)
      • register a listener for incoming messages from the mobile apps (listenForMobileMessage)
    • List of supported messages is documented and validated in the library

@juliusknorr juliusknorr mentioned this pull request Oct 26, 2022
Open
@Raudius Raudius force-pushed the enh/direct_editing branch 3 times, most recently from e355ea1 to 13897b2 Compare November 24, 2022 15:54
@Raudius @juliusknorr
Implement direct editing API
9981fe3 
Signed-off-by: Raul <r.ferreira.fuentes@gmail.com>

Implement direct editing API

Signed-off-by: Raul <r.ferreira.fuentes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Raudius Raudius

Labels
2. developing Work in progress enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement direct editing API
2 participants
@Raudius @juliusknorr