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

Make icon bubble use the rendering queue #7068

Closed
1 task
Tracked by #7446
maribethb opened this issue May 9, 2023 · 1 comment
Closed
1 task
Tracked by #7446

Make icon bubble use the rendering queue #7068

maribethb opened this issue May 9, 2023 · 1 comment
Assignees
Labels
issue: feature request Describes a new feature and why it should be added

Comments

@maribethb
Copy link
Contributor

Check for duplicates

  • I have searched for similar issues before opening a new one.

Problem

Icon rendering currently has a special case carved out for it where it will cause an immediate render. This is because when a mutator bubble is shown with setVisible the bubble needs to know where the block is in order to show the bubble in the correct location.

The special case carved out for mutators is ugly, and not performant, because it doesn't wait for the next animation frame or use the render queue, it forces an immediate re-render.

Request

Change setVisible to wait for the afterQueuedRenders promise before making the bubble actually visible. setVisible should also return a promise in case external developers (or the tests) need to wait on the promise before using the results of setVisible such as access to the mutator workspace. Note that this is a breaking change, but one that we think is acceptable because it would eliminate the special case for icons and make rendering more predictable and consistent. However, because this is a breaking change, a PR should only be submitted at times we've already agreed to do a breaking change next (like this quarter)

Note also there are a large number of tests in core that need to be updated to wait on the promise before checking that the bubble is positioned correctly.

Alternatives considered

No response

Additional context

#7024 for the start of this implementation, but backed out because the tests need to be fixed and we didn't want to hold up the rest of the change on this since Beka is busy.

@maribethb maribethb added issue: feature request Describes a new feature and why it should be added issue: triage Issues awaiting triage by a Blockly team member and removed issue: triage Issues awaiting triage by a Blockly team member labels May 9, 2023
@BeksOmega BeksOmega self-assigned this Jul 14, 2023
@BeksOmega
Copy link
Collaborator

Since we're not sure if we're going to do a major release this quarter or not, my plan is to is to add a function to the render management system that forces an immediate render of all queued renders. (As I originally suggested in #7024 ) This will allow us to use the new rendering system, but in such a way that it maintains the existing immediate-render behavior of the icons.

The next time we do a major release we can switch it to the fully correct implementation (the promises based one that Maribeth suggests).

But the immediate-render fix at least unblocks tearing out the remnants of the old system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: feature request Describes a new feature and why it should be added
Projects
None yet
Development

No branches or pull requests

2 participants