Simplify Closure-sourced code for menus #3880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `goog-menuitem-icon` and `goog-menuitem-noicon` classes are not present in Blockly. Blockly doesn’t support the CSS compiler, so #noflip has no effect. Shorten uncompressible warning string.
Also remove the “Copied from Closure” notes. These were intended so that the CSS could be easily updated as the Closure Library evolved. We are no longer linked to the Closure Library.
Blockly never uses the lazy-creation feature. Remove for simplicity.
Previously, open playground. Right-click on workspace. Mouse-over “Add comment” (it highlights). Mouse over “Download screenshot” (disabled option). Mouse over “Add comment” (highlighting is lost). Also remove `canHighlightItem` helper function. In theory this helps abstract the concept of non-highlightable options. But in practice it was only called in one of the several places that it should have been. This was a false abstraction.
The JSDoc for `setHighlightedIndex` specifically states, “If another item was previously highlighted, it is un-highlighted.” This is not what was implemented, but it should be. This commit adds the un-highlighting, and removes all the calls previously required to correct this bug.
NeilFraser
changed the title
Real OS menus don’t wrap when one cursors off the top or bottom. Also, replace the overly complicated helper function with a simple 1/-1 step value.
Remove unneeded sets to RTL on Menu (only MenuItem cares).
Context menus only disposed properly when an option was clicked. If they were dismissed by clicking outside the menu there was no disposal. This might result in a memory leak. Also un-extract (inject?) several now trivial functions.
Component is now only used by the category tree.
These were used by Menu/MenuItem.
Less code, simpler.
Old classes remain in DOM and are deprecated so that any custom CSS will continue to function.
|
@NeilFraser, looks like you're still working on this, let me know when it's ready. |
|
It's ready.
Sent (awkwardly) from an Android device.
…On Wed, 6 May 2020, 12:29 Sam El-Husseini, ***@***.***> wrote:
@NeilFraser <https://github.com/NeilFraser>, looks like you're still
working on this, let me know when it's ready.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3880 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABMCTP7X3KFHGI5GU7CAW4DRQG3AXANCNFSM4MYIKKOQ>
.
|
core/components/menu/menuitem.js
Outdated
| element.id = this.getId(); | ||
| this.setElementInternal(element); | ||
| element.id = Blockly.utils.IdGenerator.getNextUniqueId(); | ||
| element.blocklyMenuItem = this; // Link DOM back to this data structure. |
There was a problem hiding this comment.
Frankly, I'm not a huge fan of this pattern, as it shortcuts typing. There is precedence for this pattern in Blockly (eg: tooltips), so not saying it has to be changed. But here are my thoughts on the pattern:
- It makes it harder to debug for a someone new to the team / area, as there's hidden context (traditionally a DOM element doesn't have additional properties that are not DOM properties)
- The closure typechecker is not complaining (currently) because strictTypeChecks are not on, but when we do eventually get there, this will introduce issues. There are ways around that, ie: Create a new type that subclasses HTMLElement, and adds a blocklyMenuItem of type MenuItem as a property, but its clunky.
That being said, if the perf benefit is worth it for you, then by all means keep as is.
There was a problem hiding this comment.
The original Closure code maintained a database for efficient look-ups. But it was a lot of code and complexity undeserving for this task. The DOM-to-JS links were simple and efficient, but have the problems you cite. I've replaced this with an O(n) search of the menu's options. Shouldn't be a problem given that our menus/dropdowns aren't intended to have hundreds or thousands of items.
samelhusseini
added
the
breaking change
|
Adding |
The previous tags were inherited from Closure and don’t reflect current usage in the Blockly codebase. The core/components/tree files are non-compliant in this regard, but I’m not going to update them since they need to be replaced and there’s no need to create an interim API change.
samelhusseini
approved these changes
May 7, 2020
Performance is slower (O(n) rather than (O(1)), but ’n’ is the number of entries on the menu, so shouldn’t be more than a dozen or so.
Fixes a compile error (node != element).
Usually we avoid parentElement in Blockly. That’s because it has very spotty behaviour with SVG. But in this case we are in pure HTML.
|
Finally got the compiler happy. Nodes vs Elements are annoying. |
|
Awesome, thanks! Lgtm. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bunch of changes relating to menus. Highlights: