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: switch most remaining render calls to queueRender #7024

Merged
merged 6 commits into from
May 9, 2023

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Apr 27, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Was looking at #6980 and realized it would be easier to document if we just have one rendering path rather than two :P So I'm looking at trying to get rid of the old one.

Proposed Changes

Looked through all of the remaining uses of render and switched any over to queueRender that could use queueRender.

Remaining uses of render after this PR include icons (because they need to know their locations immediately) and deserialization (because it's causing forced layouts that I need to investigate).

Reason for Changes

Unification of rendering paths.

Test Coverage

Manual poking at things.

Documentation

N/A - but soon! hopefully 🤞

Additional Information

The plan for the icons is to add a function to the render management system that forces an immediate render of all queued renders.

@BeksOmega BeksOmega requested a review from a team as a code owner April 27, 2023 19:48
@BeksOmega BeksOmega requested a review from maribethb April 27, 2023 19:48
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Apr 27, 2023
@BeksOmega BeksOmega force-pushed the fix/remove-remaining-render branch from 8b20f89 to 3e755b5 Compare April 27, 2023 22:49
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Apr 27, 2023
Copy link
Contributor

@maribethb maribethb left a comment

Choose a reason for hiding this comment

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

mostly questions so i understand how rendering works!

@@ -1000,6 +1000,8 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
this.comment = null; // For backwards compatibility.
}
if (this.rendered) {
// Icons must force an immediate render so that bubbles can be opened
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead maybe add the comment icon in the afterQueuedRenders or whatever we called it? I don't actually know what I'm talking about here so feel free to ignore me if this is nonsense :p

otherwise: nit: (here and below) immediately typo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wrt your comment about finishQueuedRenders, that doesn't solve for this use case because the icon needs to be appended to the block before the render is triggered so the renderer can position it correctly.

But now that I'm thinking about this again, I'm going to investigate this one more time.

I was trying to solve for the use case of:

myBlock.setCommentText('yay a comment');
myBlock.getCommentIcon().setVisible(true);

I thought that for this to correctly position the bubble, the setCommentText method must:

  1. Append the comment to the block.
  2. Rerender the block to position the icon.
  3. Tell the icon what its new position is.

So that when setVisible is called, the icon has the correct information to position the bubble.

But thinking about it again, if we use the render queueing it should look like:

  1. Append the comment to the block.
  2. Open the bubble at the incorrect position.
  3. Rerender the block to position the icon.
  4. Tell the icon what it new position is.
  5. Update the position of the bubble for the new position of the icon.
  6. Actually paint the frame to the screen.

This should still be correct. But it was causing some unit tests to fail, so I need to remember why that was.

core/rendered_connection.ts Show resolved Hide resolved
blocks/procedures.js Show resolved Hide resolved
@BeksOmega
Copy link
Collaborator Author

@maribethb I tried wrapping the setVisible calls so that the logic is triggered after the current render completes, but that means that external developers won't be able to access the contents of the bubble immediately (e.g. the mutator workspace).

I don't particularly like that breaking change, but on the other hand I don't really like external developers messing with the contents of the bubble anyway? So wanted to confirm before I go fix the 40 remaining tests :P

@BeksOmega BeksOmega force-pushed the fix/remove-remaining-render branch from 2ea1fea to 4e5e52c Compare May 9, 2023 23:46
@BeksOmega BeksOmega merged commit af991f5 into google:develop May 9, 2023
@BeksOmega BeksOmega deleted the fix/remove-remaining-render branch May 14, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants