online icon indicating copy to clipboard operation
online copied to clipboard

Improved autosaved state for comments

Open rparth07 opened this issue 2 years ago • 13 comments

Change-Id: I9ad6c36a393ef5d895ee463f44569c52f1c8fd61

  • Resolves: #7211
  • Target version: master

Checklist

  • [ ] Code is properly formatted
  • [ ] All commits have Change-Id
  • [ ] I have run tests with make check
  • [ ] I have issued make run and manually verified that everything looks okay
  • [ ] Documentation (manuals or wiki) has been updated or is not required

rparth07 avatar Oct 15 '23 21:10 rparth07

it seems that now the comment (edit mode) is visible for other users

Sure, let's clarify the intended behavior. If I understand correctly, you're suggesting that the comment in edit mode should not be visible to other users once the focus is lost. Is that the expected behavior we aim for?

and I also get when opening the same document and trying to comment in two different browsers

17:41:10.008 Uncaught TypeError: this._viewInfo[viewid] is undefined
    getViewName Map.js:872
    insertComment CommentListSection.ts:8
    _insertAnnotationControl Control.NotebookbarBuilder.js:936
    jQuery 2
6 Map.js:872:2

I couldn't replicate the error on my end. Could you please provide detailed steps to reproduce the issue? Additionally, are there specific browsers where this problem occurs?

Also it the user comes back and doesn't press any button and clicks in another part of the document -> it's then impossible to edit the comment

The described behavior does sound unusual. Could you please provide more details on this issue? Also, do you think this behavior is related to the error mentioned earlier?

Any insights you can share will help me in thoroughly understanding and resolving this. Thank you for your assistance @pedropintosilva .

rparth07 avatar Nov 13 '23 10:11 rparth07

@rparth07 hi, are you still working on this PR? :) It requires rebase as we have conflicts with newer commits.

Would be good to put few sentences into commit message:

  • what issue does it fix?
  • how to test that it is fixed?

@lpranam could help here as he knows best the comments and autosave

eszkadev avatar Oct 22 '24 09:10 eszkadev

@rparth07 can you please rebase and solve the conflicts here ?

Darshan-upadhyay1110 avatar Oct 24 '24 09:10 Darshan-upadhyay1110

Thanks @eszkadev @Darshan-upadhyay1110 will do:)

rparth07 avatar Oct 25 '24 04:10 rparth07

Merge commits are not allowed...

lpranam avatar Nov 06 '24 15:11 lpranam

Merge commits are not allowed...

sorry, It was by mistake!

rparth07 avatar Nov 06 '24 15:11 rparth07

I resolved the merge conflicts and updated the relevant test cases for autosaved comments. However, in the multi-user Cypress tests, three test cases related to reply autosave are currently failing.

Upon further investigation, I found a potential issue (#10429) that may be causing these test failures. I believe that addressing this issue will likely resolve the failing test cases.

rparth07 avatar Nov 06 '24 19:11 rparth07

Tests are now passing. @rparth07 will you have some time to update this PR and rebase on top of master?

eszkadev avatar Dec 12 '24 06:12 eszkadev

Tests are now passing. @rparth07 will you have some time to update this PR and rebase on top of master?

Sure, will do it by today.

rparth07 avatar Dec 12 '24 07:12 rparth07

please rebase, we have API change again...

eszkadev avatar Dec 19 '24 10:12 eszkadev

this can have conflicts with https://github.com/CollaboraOnline/online/pull/10772 let's wait for the other to be merged first as tests here are not passing it seems

eszkadev avatar Dec 20 '24 11:12 eszkadev

oh @rparth07 thanks for rebasing last week, I think we had CI broken. Could you try again next qweek? maybe we will merge it finally :)

eszkadev avatar May 09 '25 16:05 eszkadev

@lpranam could you review as you know the comments auto-save code? :)

eszkadev avatar Jun 12 '25 06:06 eszkadev