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

cool#9992 doc sign: fix update of the signature widget in the status bar #10069

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

vmiklos
Copy link
Contributor

@vmiklos vmiklos commented Sep 18, 2024

Load a document with a signature, click on the status bar icon to open
the signatures dialog, remove the signature, close the dialog. The
status bar still claims the document has a signature, even it doesn't
have one anymore.

What happens is that the initial signature state is set based on the
incoming 'signaturestatus:' message, but updates to the signature state
is done via the 'statechanged: .uno:Signature=' message. Handle this in
Control.StatusBar.js.

Then Signing.js sets an empty status bar text for the "no signatures"
case, extend that to do the same as on the desktop, so we attempt to
call app.map.statusBar.showSigningItem().

Finally the update of the signstatus widget only worked once: we had
code in place at _makeIdUnique() to make sure all widgets have a unique
ID, but we didn't clear the ID of the old widget before building the new
widget, so the replacement of the signstatus widget had a signstatus1
ID. The result was that next time we didn't find the widget to update.

Signed-off-by: Miklos Vajna vmiklos@collabora.com
Change-Id: I93a21d3969fcf4754e77ef295c8f3cd872300d01

@vmiklos
Copy link
Contributor Author

vmiklos commented Sep 18, 2024

https://cpci.cbg.collabora.co.uk:8080/job/github_online_master_debug_vs_co-24.04_cypress_desktop/3125/console failed in e.g. calc/bottom_bar_spec.js, but locally trying that fails with:

    1) "before each" hook for "Bottom tool bar."
          cy:log ✱  Starting test: integration_tests/desktop/calc/bottom_bar_spec.js / Calc bottom bar tests. / Bottom tool bar.
          cy:log ✱  >> setupAndLoadDocument - start
          cy:log ✱  >> setupDocument - start
          cy:log ✱  Param - filePath: calc/BottomBar.ods
      cy:command ✔  task	copyFile, Object{4}
          cy:log ✱  << setupDocument - end
          cy:log ✱  >> loadDocument - start
          cy:log ✱  Param - filePath: calc/BottomBar-Bottom-tool-bar--ixx1q.ods
      cy:command ✔  cSetActiveFrame	#coolframe
          cy:log ✱  >> loadDocumentNoIntegration - start
      cy:command ✔  visit	/browser/c6c73bdf5d/debug.html?lang=en-US&file_path=/home/vmiklos/git/collaboraonline/online-24.04/cypress_test/workdir/data/desktop/calc/BottomBar-Bottom-tool-bar--ixx1q.ods
      cy:command ✘  uncaught exception	ReferenceError: SlideChangeGl is not defined
      cy:command ✔  error:	SlideChangeGl is not defined
                    ReferenceError: SlideChangeGl is not defined
                        at http://localhost:9900/browser/c6c73bdf/src/slideshow/Transition2d.js:74:3
      cy:command ✘  error:	SlideChangeGl is not defined
                    ReferenceError: SlideChangeGl is not defined
                        at http://localhost:9900/browser/c6c73bdf/src/slideshow/Transition2d.js:74:3
      cy:command ✘  uncaught exception	ReferenceError: TextureAnimationBase is not defined
      cy:command ✔  error:	TextureAnimationBase is not defined
                    ReferenceError: TextureAnimationBase is not defined
                        at http://localhost:9900/browser/c6c73bdf/src/slideshow/StaticTextRenderer.js:81:3
      cy:command ✔  error:	TextureAnimationBase is not defined
                    ReferenceError: TextureAnimationBase is not defined
                        at http://localhost:9900/browser/c6c73bdf/src/slideshow/StaticTextRenderer.js:81:3
      cy:command ✘  uncaught exception	TypeError: Class extends value undefined is not a constructor or null
      cy:command ✔  error:	Class extends value undefined is not a constructor or null
                    TypeError: Class extends value undefined is not a constructor or null
                        at __extends (http://localhost:9900/browser/c6c73bdf/src/canvas/sections/CommentSection.js:11:19)
                        at http://localhost:9900/browser/c6c73bdf/src/slideshow/PauseTimer.js:32:5
                        at http://localhost:9900/browser/c6c73bdf/src/slideshow/PauseTimer.js:94:2

I'm not sure what is actionable here, let's re-try cypress-desktop first.

Edit: local make clean && make got rid of the above error. Also the mobile tests point out a real problem, I've added a missing null check for the case I could reproduce.

@vmiklos vmiklos force-pushed the private/vmiklos/master branch from 187901e to d1ecfba Compare September 18, 2024 13:55
@vmiklos
Copy link
Contributor Author

vmiklos commented Sep 18, 2024

https://cpci.cbg.collabora.co.uk:8080/job/github_online_master_debug_vs_co-24.04_cypress_desktop/3133/console fails in calc/scrolling_spec.js and calc/bottom_bar_spec.js, but locally trying:

make -C cypress_test check-desktop spec=calc/scrolling_spec.js
make -C cypress_test check-desktop spec=calc/bottom_bar_spec.js

works fine, hmm. Let's try once more.

Edit: ah, the trouble with the scrolling is that once we have a signature widget for the "no signature" case, it changes what widgets are visible as you scroll in a small window. But I only want to have an empty widget after we already had a signature, so don't touch the statusbar in the no signatures -> no signatures transition.

@vmiklos vmiklos force-pushed the private/vmiklos/master branch from d1ecfba to 9644e0b Compare September 19, 2024 07:22
@vmiklos
Copy link
Contributor Author

vmiklos commented Sep 19, 2024

https://cpci.cbg.collabora.co.uk:8080/job/github_online_master_debug_vs_co-24.04_cypress_multi_user/3139/console failed in writer/invalidations.spec, but local make -C cypress_test check-multi spec=writer/invalidations_spec.js passes, retry.

@vmiklos
Copy link
Contributor Author

vmiklos commented Sep 19, 2024

@vmiklos vmiklos requested a review from caolanm September 19, 2024 09:45
@vmiklos
Copy link
Contributor Author

vmiklos commented Sep 19, 2024

@caolanm could you please review this? Thanks.

The mobile case is just a minimal fix, looks like signing on mobile is broken (app.map.mobileTopBar.showSigningItem is undefined), but I suggest let's worry about that minor problem once the normal desktop case is in a reasonable shape. Other than that, removing signatures should work reasonably well with this.

var statusIcon = '';
// Have a non-empty status text by default, so a signatures -> no signatures
// transition updates the status bar.
var statusText = 'The document is not signed.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need the _() markup so it gets translated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, let me fix.

Load a document with a signature, click on the status bar icon to open
the signatures dialog, remove the signature, close the dialog. The
status bar still claims the document has a signature, even it doesn't
have one anymore.

What happens is that the initial signature state is set based on the
incoming 'signaturestatus:' message, but updates to the signature state
is done via the 'statechanged: .uno:Signature=' message. Handle this in
Control.StatusBar.js.

Then Signing.js sets an empty status bar text for the "no signatures"
case, extend that to do the same as on the desktop, so we attempt to
call app.map.statusBar.showSigningItem() for the "had signatures" -> "no
signatures" transition.

Finally the update of the signstatus widget only worked once: we had
code in place at _makeIdUnique() to make sure all widgets have a unique
ID, but we didn't clear the ID of the old widget before building the new
widget, so the replacement of the signstatus widget had a signstatus1
ID. The result was that next time we didn't find the widget to update.

Signed-off-by: Miklos Vajna <vmiklos@collabora.com>
Change-Id: I93a21d3969fcf4754e77ef295c8f3cd872300d01
@vmiklos vmiklos force-pushed the private/vmiklos/master branch from 9644e0b to d63bfd2 Compare September 19, 2024 12:18
@vmiklos vmiklos merged commit 9571682 into master Sep 19, 2024
14 checks passed
@vmiklos vmiklos deleted the private/vmiklos/master branch September 19, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants