fix: When using the toolbox-search, auto select the found dropdown item #2222

constantinIliescu · 2024-02-26T09:52:59Z

Fixes: #1940

Uses the Block State as suggested here:
#2004 (comment)

Fixes: bug that didn't indexed the last trigram from a word

The basics

I validated my changes

The details

Resolves

Fixes #1940

Proposed Changes

Use block state for efficient indexing

Reason for Changes

Have the found drop down item automatically selected in the drop down

Test Coverage

Unit test have been added to cover the false positive, false negative and duplicate cases

Documentation

Additional Information

Fixes: google#1940 Uses the Block State as suggested here: google#2004 (comment) Fixes: bug that didn't indexed the last trigram from a word

BeksOmega

Generally LGTM! I'll give it a more thorough review after the first round of changes have been fixed.

BeksOmega · 2024-03-20T22:56:42Z

plugins/toolbox-search/src/block_searcher.ts

+    blockState.id = undefined;
+    blockState.extraState = undefined;
+    blockState.data = undefined;
+    const stateString = JSON.stringify(blockState);


Is there a reason you need to stringify this? I think you should be able to add it to a Set<Blockly.serialization.blocks.State>. If the stringification is for deduplication or something, could you add a comment inline?

Yes, the reason was duplication, without stringify we would have ended with a lot of duplication. I will put a comment in the code

plugins/toolbox-search/src/block_searcher.ts

BeksOmega · 2024-03-20T22:59:18Z

plugins/toolbox-search/src/toolbox_search.ts

+          if (blockState.fields) {
+            return {
+              kind: 'block',
+              type: blockState.type,
+              fields: blockState.fields,
+            };
+          } else
+            return {
+              kind: 'block',
+              type: blockState.type,
+            };


Suggested change

if (blockState.fields) {

return {

kind: 'block',

type: blockState.type,

fields: blockState.fields,

};

} else

return {

kind: 'block',

type: blockState.type,

};

return {kind: 'block', ...blockState};

BeksOmega · 2024-03-20T23:02:00Z

plugins/toolbox-search/src/block_searcher.ts

+      if (blockState != null) {
+        this.indexBlockText(blockType.replaceAll('_', ' '), blockState);
+        block.inputList.forEach((input) => {
+          input.fieldRow.forEach((field) => {
+            this.indexDropdownOption(field, blockState);
+            this.indexBlockText(field.getText(), blockState);
+          });
        });
-      });
+      }


Instead of wrapping this could you use an early return?

if (!blockState) return; this.indexBlockText // etc...

This function already has a lot of nesting and I'd prefer to avoid adding more if we can :P

Agree, I will fix it

BeksOmega · 2024-03-20T23:03:51Z

plugins/toolbox-search/src/block_searcher.ts

+          if (state.fields == undefined) {
+            state.fields = {};
+          }


I think this case should be impossible since you assing to state.fields above. Was the typescript compiler giving you warnings about this?

I think it was the compiler complaining. I don't exactly remember

BeksOmega · 2024-03-20T23:04:36Z

plugins/toolbox-search/src/block_searcher.ts

+            state.fields[field.name] = option[1];
+          }
+          this.indexBlockText(option[0], state);
+          this.indexBlockText(option[1], state);


I don't think we should be indexing the language-neutral text.

Well, I added this on some blocks the language-neutral text was more informative than the block text. I don't remember now a clear example, but I take a look. BTW, sorry for replaying so late, but lately I am little busy with an internal project. I would try to find some time to fix your comments.

No rush! It took us three weeks to get back to you so we can't expect you to be more responsive :P

BeksOmega

Added some more comments for when you have time to take a look!

BeksOmega · 2024-03-26T15:40:02Z

plugins/toolbox-search/CHANGELOG.md

@@ -3,6 +3,12 @@
 All notable changes to this project will be documented in this file.
 See [Conventional Commits](https://conventionalcommits.org) for commit guidelines.

+## [1.2.7](https://github.com/google/blockly-samples/compare/@blockly/toolbox-search@1.2.6...@blockly/toolbox-search@1.2.7) (2024-02-23)


Can you revert this? The CHANGELOG doesn't need to be edited manually (it gets updated automatically based on submitted PRs)

BeksOmega · 2024-03-26T15:40:34Z

plugins/toolbox-search/src/block_searcher.ts

        if (typeof option[0] === 'string') {
-          this.indexBlockText(option[0], blockType);
+          if (state.fields == undefined) {


Throughout, can you replace == with === and != with !==?

BeksOmega · 2024-03-26T15:41:46Z

plugins/toolbox-search/src/block_searcher.ts

+  private indexDropdownOption(
+    field: Blockly.Field,
+    blockState: Blockly.serialization.blocks.State,
+  ) {
    if (field instanceof Blockly.FieldDropdown) {


Can you switch this to use an early return as well? (not your code but I think it would make it more readable).

BeksOmega · 2024-03-26T15:43:55Z

plugins/toolbox-search/src/block_searcher.ts

Can you add a jsdoc comment to the trigramsToBlocks map that specifies that the Set<string> contains stringified JSON?

BeksOmega · 2024-03-26T15:54:58Z

plugins/toolbox-search/src/block_searcher.ts

    ];
+    const resultState = result.map((item) => JSON.parse(item));
+    return resultState;


You should be able to set this up to just return from the top. But I can't put a suggestion over this whole function b/c github :P

Suggested change

];

const resultState = result.map((item) => JSON.parse(item));

return resultState;

].map(JSON.parse);

BeksOmega · 2024-03-26T15:56:16Z

plugins/toolbox-search/src/block_searcher.ts

+    blockState.extraState = undefined;
+    blockState.data = undefined;


Why are these being removed? It seems like they should probably be kept.

BeksOmega · 2024-03-26T16:03:44Z

plugins/toolbox-search/src/block_searcher.ts

   */
-  blockTypesMatching(query: string): string[] {
-    return [
+  blockTypesMatching(query: string): Blockly.serialization.blocks.State[] {


Because this is public, changing the return type of this method is actually a breaking change. Could you create another method blockStatesMatching that has the current behavior here? And then modify this to only return the block /types/ not the full state. You'll need to update other places to use your new blockStatesMatching method.

BeksOmega · 2024-04-09T16:11:36Z

@constantinIliescu are you still interested in working on this? If not I'll close this on Friday!

constantinIliescu · 2024-04-12T10:25:17Z

Hi Beka,
Yes, I intend to finish this , but I still need some more time. I been caught in more urgent things lately.

BeksOmega · 2024-04-12T15:27:48Z

Hi Beka, Yes, I intend to finish this , but I still need some more time. I been caught in more urgent things lately.

All good no rush! Just wanted to check in in case you wanted to hand it over to someone else =)

Fix: When using the toolbox-search, auto select the found dropdown item

3cc4822

Fixes: google#1940 Uses the Block State as suggested here: google#2004 (comment) Fixes: bug that didn't indexed the last trigram from a word

constantinIliescu requested a review from a team as a code owner February 26, 2024 09:53

constantinIliescu requested review from maribethb and removed request for a team February 26, 2024 09:53

github-actions bot assigned maribethb Feb 26, 2024

BeksOmega assigned BeksOmega and unassigned maribethb Mar 20, 2024

BeksOmega requested review from BeksOmega and removed request for maribethb March 20, 2024 22:47

BeksOmega requested changes Mar 20, 2024

View reviewed changes

BeksOmega requested changes Mar 26, 2024

View reviewed changes

maribethb changed the title ~~Fix: When using the toolbox-search, auto select the found dropdown item~~ fix: When using the toolbox-search, auto select the found dropdown item Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: When using the toolbox-search, auto select the found dropdown item #2222

fix: When using the toolbox-search, auto select the found dropdown item #2222

constantinIliescu commented Feb 26, 2024 •

edited by maribethb

BeksOmega left a comment

BeksOmega Mar 20, 2024

constantinIliescu Mar 26, 2024

BeksOmega Mar 20, 2024

BeksOmega Mar 20, 2024

constantinIliescu Mar 26, 2024

BeksOmega Mar 20, 2024

constantinIliescu Mar 26, 2024

BeksOmega Mar 20, 2024

constantinIliescu Mar 26, 2024

BeksOmega Mar 26, 2024

BeksOmega left a comment

BeksOmega Mar 26, 2024

BeksOmega Mar 26, 2024

BeksOmega Mar 26, 2024

BeksOmega Mar 26, 2024

BeksOmega Mar 26, 2024

BeksOmega Mar 26, 2024

BeksOmega Mar 26, 2024

BeksOmega commented Apr 9, 2024

constantinIliescu commented Apr 12, 2024

BeksOmega commented Apr 12, 2024

		blockState.extraState = undefined;
		blockState.data = undefined;

fix: When using the toolbox-search, auto select the found dropdown item #2222

Are you sure you want to change the base?

fix: When using the toolbox-search, auto select the found dropdown item #2222

Conversation

constantinIliescu commented Feb 26, 2024 • edited by maribethb

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

BeksOmega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeksOmega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeksOmega commented Apr 9, 2024

constantinIliescu commented Apr 12, 2024

BeksOmega commented Apr 12, 2024

constantinIliescu commented Feb 26, 2024 •

edited by maribethb