Skip to content

Conversation

@neilcsmith-net
Copy link
Member

Reverts #8462

Unfortunately looks like we might have to back this out of NB26 and also take more time to consider how to integrate this change into NB27. This currently causes a test failure because it routes copy through NbClipboard, but not paste. Setting clipboard contents in NbClipboard is asynchronous - getting contents from it waits for the setting task, but this is not happening because paste is bypassing it.

…tead of directly accessing system clipboard"
@neilcsmith-net neilcsmith-net added the ci:all-tests [ci] enable all tests label May 1, 2025
@apache apache locked and limited conversation to collaborators May 1, 2025
@apache apache unlocked this conversation May 1, 2025
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

safer that way. Who knows what else doesn't expect the asynchronous nature of NbClipboard (#8476 the test didn't expect that at least)

@matthiasblaesing
Copy link
Contributor

I will not intervene, but having a requested feature reverted, because a broken test fails feels wrong to me. Don't know when/if I will look into the problem, so feel free to merge, but I don't agree.

@neilcsmith-net
Copy link
Member Author

@matthiasblaesing yes, it's unfortunate, but in my opinion the test is valid and the fix isn't correct at this point. We need to work out the right approach to take before your other PR with this is merged to master too.

@mbien
Copy link
Member

mbien commented May 1, 2025

the test failed because the behavior changed. Fixing the test is not the problem, I did that already. Its more about wanting to risk that something else fails due to that change or not. I am fairly neutral here but since this release has a few risky merges already I do prefer the revert slightly.

@neilcsmith-net neilcsmith-net added this to the NB26 milestone May 5, 2025
@ebarboni
Copy link
Contributor

ebarboni commented May 5, 2025

ok ok then merging. And if green I will cut the rc3

@ebarboni ebarboni merged commit ef64307 into delivery May 5, 2025
42 of 69 checks passed
@neilcsmith-net neilcsmith-net added the Release process PRs (eg. versions, sync) that are part of the release process and can be ignored in release notes. label May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:all-tests [ci] enable all tests Release process PRs (eg. versions, sync) that are part of the release process and can be ignored in release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants