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: When using the toolbox-search, auto select the found dropdown item #2222

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions plugins/toolbox-search/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

### Bug Fixes
* Improved search for drop down
* Improved unit tests
* Last trigram from a word was never indexed

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

**Note:** Version bump only for package @blockly/toolbox-search
Expand Down
68 changes: 48 additions & 20 deletions plugins/toolbox-search/src/block_searcher.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -27,29 +27,44 @@ export class BlockSearcher {
const blockCreationWorkspace = new Blockly.Workspace();
blockTypes.forEach((blockType) => {
const block = blockCreationWorkspace.newBlock(blockType);
this.indexBlockText(blockType.replaceAll('_', ' '), blockType);
block.inputList.forEach((input) => {
input.fieldRow.forEach((field) => {
this.indexDropdownOption(field, blockType);
this.indexBlockText(field.getText(), blockType);
const blockState = Blockly.serialization.blocks.save(block);
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);
});
});
});
}
Comment on lines +31 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Agree, I will fix it

});
}

/**
* Check if the field is a dropdown, and index every text in the option
*
* @param field We need to check the type of field
* @param blockType The block type to associate the trigrams with.
* @param blockState The block state to associate the trigrams with.
*/
private indexDropdownOption(field: Blockly.Field, blockType: string) {
private indexDropdownOption(
field: Blockly.Field,
blockState: Blockly.serialization.blocks.State,
) {
if (field instanceof Blockly.FieldDropdown) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

field.getOptions(true).forEach((option) => {
const state = {...blockState};
state.fields = {...blockState.fields};
if (typeof option[0] === 'string') {
this.indexBlockText(option[0], blockType);
if (state.fields == undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

state.fields = {};
}
Comment on lines +58 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

if (field.name) {
state.fields[field.name] = option[1];
}
this.indexBlockText(option[0], state);
this.indexBlockText(option[1], state);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

} else if ('alt' in option[0]) {
this.indexBlockText(option[0].alt, blockType);
this.indexBlockText(option[0].alt, state);
}
});
}
Expand All @@ -59,10 +74,10 @@ export class BlockSearcher {
* Filters the available blocks based on the current query string.
*
* @param query The text to use to match blocks against.
* @returns A list of block types matching the query.
* @returns A list of block states matching the query.
*/
blockTypesMatching(query: string): string[] {
return [
blockTypesMatching(query: string): Blockly.serialization.blocks.State[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const result = [
...this.generateTrigrams(query)
.map((trigram) => {
return this.trigramsToBlocks.get(trigram) ?? new Set<string>();
Expand All @@ -72,20 +87,33 @@ export class BlockSearcher {
})
.values(),
];
const resultState = result.map((item) => JSON.parse(item));
return resultState;
Comment on lines 89 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

}

private addBlockTrigram(trigram: string, blockState: string) {
const blockSet = this.trigramsToBlocks.get(trigram) ?? new Set<string>();
blockSet.add(blockState);
this.trigramsToBlocks.set(trigram, blockSet);
}

/**
* Generates trigrams for the given text and associates them with the given
* block type.
* block state.
*
* @param text The text to generate trigrams of.
* @param blockType The block type to associate the trigrams with.
* @param blockState The block state to associate the trigrams with.
*/
private indexBlockText(text: string, blockType: string) {
private indexBlockText(
text: string,
blockState: Blockly.serialization.blocks.State,
) {
blockState.id = undefined;
blockState.extraState = undefined;
blockState.data = undefined;
Comment on lines +112 to +113
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const stateString = JSON.stringify(blockState);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

this.generateTrigrams(text).forEach((trigram) => {
const blockSet = this.trigramsToBlocks.get(trigram) ?? new Set<string>();
blockSet.add(blockType);
this.trigramsToBlocks.set(trigram, blockSet);
this.addBlockTrigram(trigram, stateString);
});
}

Expand All @@ -101,7 +129,7 @@ export class BlockSearcher {
if (normalizedInput.length <= 3) return [normalizedInput];

const trigrams: string[] = [];
for (let start = 0; start < normalizedInput.length - 3; start++) {
for (let start = 0; start <= normalizedInput.length - 3; start++) {
BeksOmega marked this conversation as resolved.
Show resolved Hide resolved
trigrams.push(normalizedInput.substring(start, start + 3));
}

Expand Down
19 changes: 13 additions & 6 deletions plugins/toolbox-search/src/toolbox_search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* in its flyout.
*/
import * as Blockly from 'blockly/core';
import {block} from '../node_modules/blockly/core/tooltip';

Check warning on line 12 in plugins/toolbox-search/src/toolbox_search.ts

View workflow job for this annotation

GitHub Actions / lint

'block' is defined but never used
import {BlockSearcher} from './block_searcher';

/* eslint-disable @typescript-eslint/naming-convention */
Expand Down Expand Up @@ -171,13 +172,19 @@
*/
private matchBlocks() {
const query = this.searchField?.value || '';

this.flyoutItems_ = query
? this.blockSearcher.blockTypesMatching(query).map((blockType) => {
return {
kind: 'block',
type: blockType,
};
? this.blockSearcher.blockTypesMatching(query).map((blockState) => {
if (blockState.fields) {
return {
kind: 'block',
type: blockState.type,
fields: blockState.fields,
};
} else
return {
kind: 'block',
type: blockState.type,
};
Comment on lines +177 to +187
Copy link
Contributor

Choose a reason for hiding this comment

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

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};

})
: [];

Expand Down