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

fix!: Use setShadowState instead of setShadow in shadow-block-converter #2150

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

johnnesky
Copy link
Member

@johnnesky johnnesky commented Jan 12, 2024

The basics

The details

Resolves

Fixes #2064

Proposed Changes

The shadow-block-converter no longer uses the setShadow method of the edited shadow block. Instead, it serializes and re-instantiates any edited shadow block as a regular block with a different id, and attaches this block to the parent connection on top of the existing shadow block. It then uses the setShadowState method of the parent Connection to revert any changes made to the connection's persistent shadow state, preserving the original shadow state as a default value.

Reason for Changes

The shadowState method on blocks is considered internal and is not intended to be used by external code such as plugins, because when used to convert a regular block to a shadow block that was not previously associated with a shadow state it can have confusing results. The plugin behavior was otherwise good, in fact it was nicer in some ways than the new behavior because it preserved the id and the instance of the edited block, and did not need to serialize or deserialize anything.

Test Coverage

The existing shadow-block-converter tests were updated to accommodate the fact that the edited blocks became new instances with new ids. I also added tests to ensure that the connection's shadow state is consistent after edits, undos, and redos, so that we could rely on the shadow state acting as a default value.

Documentation

I updated the readme.

Additional Information

I attempted to at least preserve the id of the edited blocks, but ran into problems. Ideally, we would let the shadow block and the regular block have the same id, and in principle that ought to be possible since they don't need to coexist at the same time. However, we need to call setShadowState on the connection after attaching the regular block to revert any changes made by attaching the block, and the setShadowState implementation temporarily instantiates the provided shadow state, which can't have the same id as a block in the workspace. Changing the shadow state id won't be compatible with any existing undo history on the shadow block, such as the block change event that triggered this whole shadow conversion, so the only alternative is to change the id of the regular block so that the shadow state won't conflict with it.

Breaking changes

This is a breaking change because it removes a previously publicly exported event type, BlockShadowChange (and replaces it with a different one with a different interface, BlockShadowStateChange).

@johnnesky johnnesky requested a review from a team as a code owner January 12, 2024 22:40
@johnnesky johnnesky requested review from NeilFraser and removed request for a team January 12, 2024 22:40
@BeksOmega BeksOmega assigned BeksOmega and unassigned NeilFraser Jan 12, 2024
@BeksOmega BeksOmega requested review from BeksOmega and removed request for NeilFraser January 12, 2024 22:41
@johnnesky johnnesky merged commit 7cd4c5c into master Jan 12, 2024
13 checks passed
@johnnesky johnnesky deleted the revert-2149-revert-2090-nesky_setShadowState branch January 13, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shadow-block-converter should use setShadowState instead of setShadow
3 participants