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

Fix/toc without transaction #5893

Merged
merged 8 commits into from
Jul 25, 2024
Merged

Fix/toc without transaction #5893

merged 8 commits into from
Jul 25, 2024

Conversation

max-nextcloud
Copy link
Collaborator

@max-nextcloud max-nextcloud commented Jun 12, 2024 •
edited
Loading

📝 Summary

🚧 TODO

  • implement click function
  • sync plugin state to vuex
  • cleanup file structure

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@max-nextcloud max-nextcloud force-pushed the fix/toc-without-transaction branch 2 times, most recently from 443d9f3 to 53ae9d7 Compare June 12, 2024 20:06
@max-nextcloud max-nextcloud marked this pull request as ready for review June 12, 2024 20:07
@max-nextcloud max-nextcloud requested a review from mejo- June 12, 2024 20:07
max-nextcloud
max-nextcloud commented Jun 12, 2024
View reviewed changes
src/components/Editor/TableOfContents.vue
Comment on lines -43 to +48
this.$editor
.chain()
.focus()
.setTextSelection(heading.position)
.scrollIntoView()
.run()
document.getElementById(heading.id).scrollIntoView()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is slightly different from what we used to do. In particular we do not set the cursor anymore.

We could also use the old way replacing heading.position with heading.offset + 1. I changed it as relying on the id seemed more robust to me than the offset at that time - but both should be fine now as we recompute and update the offset whenever the doc changes.

@juliusknorr
Copy link
Member

Very nice, unfortunately this does not seem to fix the initial update step stent for read only sessions.

@juliusknorr
Copy link
Member

Also happens without headings for files that have not been opened by a write session before:

Screenshot 2024-06-13 at 07 25 53

@juliusknorr
Copy link
Member

Additional backend attempt at #5895

@max-nextcloud
Copy link
Collaborator Author

Very nice, unfortunately this does not seem to fix the initial update step stent for read only sessions.

Yes... that's exactly what I wondered this morning - whether the toc transaction was really the cause.

Thanks for coming up with the backend fix. I'll take a look at it.

@max-nextcloud
Copy link
Collaborator Author

Since this does not fix the 403s fully, there's no rush to get it into todays RC anymore.

I'll fix the tests and see if it fixes nextcloud/collectives#1114 .

@max-nextcloud max-nextcloud self-assigned this Jun 13, 2024
@juliusknorr juliusknorr added bug Something isn't working 2. developing labels Jun 13, 2024
@juliusknorr juliusknorr added this to the Nextcloud 30 milestone Jun 13, 2024
@max-nextcloud max-nextcloud force-pushed the fix/toc-without-transaction branch 3 times, most recently from e78caba to 6015463 Compare June 17, 2024 11:21
@max-nextcloud

This comment was marked as resolved.

Sign in to view
@max-nextcloud
Copy link
Collaborator Author

One major issue remains though. When typing at the beginning of the heading nothing happens. I suspect this is due to the slightly different markup:

I figured it out. By default decorations are put behind the cursor in the given position. Adding a { side: -1 } fixed that and now the cursor is between the anchor and the first letter of the heading and typing works just fine.

@max-nextcloud max-nextcloud force-pushed the fix/toc-without-transaction branch 3 times, most recently from f3a233d to da1b008 Compare June 18, 2024 13:42
@mejo- mejo- force-pushed the fix/toc-without-transaction branch 2 times, most recently from 8d3502e to 0594780 Compare July 23, 2024 13:11
@max-nextcloud @mejo-
refactor(heading): move css to prosemirror.scss
cfd2106 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud @mejo-
fix: use decorations for heading anchors
7d9a975 
This avoids transactions that actually change the document state.

Fixes #5861.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud @mejo-
test(cy): test headings without node-view-content
1a9f305 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud @mejo-
fix(editor): only setEditable when needed
4c529f4 
This will trigger an update event on the editor
which in turn will trigger all update handlers.

Avoid this when not necessary.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud @mejo-
fix(heading): put anchor in front of cursor and first letter
56f603e 
This allows typing even at the beginning of the heading
as the cursor now is between the anchor and the first letter.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud @mejo-
test(cy): do not wait for push after initialization
2fc6268 
The toc does not trigger a transaction anymore
and therefore there is no push to wait for either.

Also use cypress aliases and avoid deep nesting
of cypress calls when possible.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud @mejo-
fix(headings): adjust decoration if heading level changes
f73b501 
Signed-off-by: Max <max@nextcloud.com>
@mejo- mejo- force-pushed the fix/toc-without-transaction branch from 81bae7b to ea2e93d Compare July 25, 2024 11:05
@mejo-
Copy link
Member

mejo- commented Jul 25, 2024

/backport to stable29

@mejo-
Copy link
Member

mejo- commented Jul 25, 2024

/backport to stable28

@mejo-
fix(headingAnchor): Rebuild decorationSet in case of replacements
e0d10b2 
When the whole document gets replaced by the same content,
`DecorationSet.map()` returns an empty decorationSet. So only use it for
updates where no decorations get removed.

Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- force-pushed the fix/toc-without-transaction branch from ea2e93d to e0d10b2 Compare July 25, 2024 11:06
@juliusknorr juliusknorr merged commit 236860a into main Jul 25, 2024
61 checks passed
@juliusknorr juliusknorr deleted the fix/toc-without-transaction branch July 25, 2024 12:32
This was referenced Jul 25, 2024
Closed
Merged
@mejo- mejo- mentioned this pull request Jul 25, 2024
Closed
@blizzz blizzz mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mejo- mejo- mejo- approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

@max-nextcloud max-nextcloud

Labels
2. developing bug Something isn't working
Projects
📝 Office team
Archived in project
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

Initializing the table of contents creates a transaction even in read only mode
3 participants
@max-nextcloud @juliusknorr @mejo-