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

Version support and iframe refactoring #3152

Merged
merged 7 commits into from
Sep 8, 2023
Merged

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Aug 31, 2023

Fixes #3102
Fixes #3011

Best to be reviewed as without whitespaces

Requires nextcloud/server#40184 for a smooth restore

Sorry I went a bit nuts on refactoring bits in the document controller, so this is a larger changeset, but the relevant changes in summary:

  • Introduce a token endpoint to generate a wopi token
  • Rework the viewer vue component to request the token from the new endpoint and directly load Collabora in an iframe instead of loading an intermediate page (DocumentController::index) which used to generate the token and load another iframe within
  • Remove transition in vue component to avoid flickering
  • Move missing post message logic into vue component (as the existing implementation from document.js is no longer used now in the viewer)
  • Add cypress tests for important integration parts where the post message handling is happening
  • Simplify and merge common logic to get a the file and share for token generation
  • With that we can now also load versions through the token endpoint which enables us to make use of the version preview and side by side comparison with 27.1

Summary

  • Refactor iframe loading for viewer to only load one instead of two nested iframes
  • Generate a token before through an API call
  • Implement support for version preview and comparison with viewer
  • Remove micro transition for animating the fade in as it caused more troubles then it helped

TODO

  • Double check all cases for iframe loading
  • Document follow ups to see where we still need to replace the double iframe
    • Public single file shares
    • Direct editing
    • Federated editing
  • Cleanup post message handling
  • Tests
  • Follow up issue for text to handle restore gracefully

Checklist

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

@juliusknorr juliusknorr added enhancement New feature or request 2. developing Work in progress labels Aug 31, 2023
@cypress
Copy link

cypress bot commented Aug 31, 2023

Passing run #586 ↗︎

0 27 5 0 Flakiness 0

Details:

Version support and iframe refactoring
Project: Richdocuments Commit: e34a607082
Status: Passed Duration: 03:02 💡
Started: Sep 8, 2023 9:23 PM Ended: Sep 8, 2023 9:26 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@juliusknorr
Copy link
Member Author

juliusknorr commented Aug 31, 2023

Quick profile of what we save between old and new appraoch

From Viewer loading to actual collabora frame being loaded

Old

Full additional page load with core javascript initialization, times hard to measure reliably but in the profile ~4s
Screenshot 2023-08-31 at 12 53 09

New

Only one API call ~80ms
Screenshot 2023-08-31 at 12 56 03

@juliusknorr juliusknorr force-pushed the chore/refactor-iframe branch 8 times, most recently from 6eaa31a to 001651d Compare August 31, 2023 21:37
@juliusknorr juliusknorr marked this pull request as ready for review September 1, 2023 09:02
@juliusknorr juliusknorr added 3. to review Ready to be reviewed and removed 2. developing Work in progress labels Sep 1, 2023
@juliusknorr
Copy link
Member Author

/backport to stable27

lib/TokenManager.php Outdated Show resolved Hide resolved
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Just had a closer look at the mixins. They look good except for one catch that i think is too broad.

Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr juliusknorr merged commit 6459966 into main Sep 8, 2023
46 checks passed
@juliusknorr juliusknorr deleted the chore/refactor-iframe branch September 8, 2023 22:08
@juliusknorr
Copy link
Member Author

/backport to stable27

@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

1 similar comment
@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version preview support in 27.1 Refactor frontend to remove second iframe
2 participants