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.

Sign up for GitHub

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

Jump to bottom

Add indexing of dropdown entries #2004

Closed
wants to merge 5 commits into from
Closed

Add indexing of dropdown entries #2004

wants to merge 5 commits into from

Conversation

koenvanwijk
Copy link

@koenvanwijk koenvanwijk commented Oct 4, 2023 •
edited by BeksOmega

The basics

The details

Resolves

Fixes #1940

Proposed Changes

The search should find the block and select the dropdown item.
image

Reason for Changes

The search fails to find blocks with dropdown.

Test Coverage

The manual test show the blocks are found and that the option is selected.

Documentation

No update of the documentation is required. Although an remark could be made.

Additional Information

@koenvanwijk koenvanwijk requested a review from a team as a code owner October 4, 2023 12:47
@koenvanwijk koenvanwijk requested review from NeilFraser and removed request for a team October 4, 2023 12:47
@BeksOmega
Copy link
Contributor

We have a partial fix for this on the OSD branch: #1971

It adds the indexing but doesn't fix the blocks. We're planning to merge that early next week (we're doing a design sprint this week) so I'll come back to this after that is done so we can get it rebased and integrated!

@ludizhan
Copy link

ludizhan commented Oct 6, 2023

FYI I got assigned to the original issue #1940 and was looking into the part 2 of fix. I was looking into how to represent blocks in json and was about to raise questions around this.
If you would like to take over the issue, maybe comment on the original issue first would be better so that we don't do the duplicate work.

@koenvanwijk
Copy link
Author

@ludizhan Please have a look at the implementation of the toolbox where the option of a dropdown is set, I am fine if you take it in the delivery of #1940.

@ludizhan ludizhan mentioned this pull request Oct 7, 2023
Closed
1 task
@ludizhan
Copy link

ludizhan commented Oct 7, 2023

Thanks, created #2013 for the osd branch.

@BeksOmega BeksOmega assigned BeksOmega and unassigned NeilFraser Nov 27, 2023
@BeksOmega BeksOmega requested review from BeksOmega and removed request for NeilFraser November 27, 2023 21:28
@BeksOmega
Copy link
Contributor

We ended up having to close the other PR, so circling back on this! I gave it a test and there are a few issues, but generally looks good.

Are you still interested in working on this @koenvanwijk ?

@koenvanwijk
Copy link
Author

koenvanwijk commented Nov 28, 2023 via email

BeksOmega
BeksOmega requested changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Thanks for still be interested in this! Appreciate your patience =)

I saw an issue yesterday where searching for variable names would create new variables in the workspace. But I'm not sure if that's actually an issue or if I just rebased your code incorrectly.

Could you rebase this and then make the changes requested? Then I'll give this another manual test!

plugins/toolbox-search/src/block_searcher.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to this file are unnecessary because the other PR added indexing.

plugins/toolbox-search/src/toolbox_search.ts Outdated
@@ -161,9 +161,45 @@ export class ToolboxSearchCategory extends Blockly.ToolboxCategory {
this.flyoutItems_ = query ?
this.blockSearcher.blockTypesMatching(query).map(
(blockType) => {
// check the block if it has dropdowns
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 remove the inline comments throughout? I think the code is self-explanatory!

@koenvanwijk
Copy link
Author

koenvanwijk commented Nov 28, 2023 via email

@BeksOmega
Copy link
Contributor

Hm ok so I looked at this again, and I think we should actually reorganize this a bit. I think the BlockSearcher should handle saving the field values in the trigrams map, instead of re-searching for the dropdown fields in the ToolboxSearchCategory. That way we can also easily match the alt text (which is currently being indexed, but not selected when searching), and it's more performant.

So the trigrams map will change to be:

private trigramsToBlocks = new Map<string, Set<Blockly.serialization.blocks.State>>();

And indexDropdownOption will save the field value as part of the State.

@savakarrohan
Copy link

savakarrohan commented Dec 19, 2023 •
edited

As an end user of the toolbox-search package, I've noticed that it doesn't support multiple workspaces.
Any tips? Should I submit an issue?
Could you direct me to which part of code too? Would love to open up a pull request too!

console.js:213  Error: Shortcut named "startSearch" already exists.
    at ShortcutRegistry$$module$build$src$core$shortcut_registry.register (shortcut_registry.ts:55:17)
    at r.registerShortcut (toolbox_search.ts:95:5)
    at new r (toolbox_search.ts:42:10)
    at Toolbox$$module$build$src$core$toolbox$toolbox.createToolboxItem_ (toolbox.ts:426:27)
    at Toolbox$$module$build$src$core$toolbox$toolbox.renderContents_ (toolbox.ts:394:12)
    at Toolbox$$module$build$src$core$toolbox$toolbox.render (toolbox.ts:377:10)
    at Toolbox$$module$build$src$core$toolbox$toolbox.init (toolbox.ts:150:10)
    at init$$module$build$src$core$inject (inject.ts:239:15)
    at Object.inject$$module$build$src$core$inject [as inject] (inject.ts:65:3)
    at BlocklyLayout.onAfterAttach (layout.ts:62:23)

@BeksOmega
Copy link
Contributor

As an end user of the toolbox-search package, I've noticed that it doesn't support multiple workspaces. Any tips? Should I submit an issue? Could you direct me to which part of code too? Would love to open up a pull request too!

console.js:213  Error: Shortcut named "startSearch" already exists.
    at ShortcutRegistry$$module$build$src$core$shortcut_registry.register (shortcut_registry.ts:55:17)
    at r.registerShortcut (toolbox_search.ts:95:5)
    at new r (toolbox_search.ts:42:10)
    at Toolbox$$module$build$src$core$toolbox$toolbox.createToolboxItem_ (toolbox.ts:426:27)
    at Toolbox$$module$build$src$core$toolbox$toolbox.renderContents_ (toolbox.ts:394:12)
    at Toolbox$$module$build$src$core$toolbox$toolbox.render (toolbox.ts:377:10)
    at Toolbox$$module$build$src$core$toolbox$toolbox.init (toolbox.ts:150:10)
    at init$$module$build$src$core$inject (inject.ts:239:15)
    at Object.inject$$module$build$src$core$inject [as inject] (inject.ts:65:3)
    at BlocklyLayout.onAfterAttach (layout.ts:62:23)

Yes please open an issue for this with reproduction steps!

ictin added a commit to ictin/blockly-samples that referenced this pull request Jan 3, 2024
@ictin
Fix: when using the toolbox-search, auto select the found dropdown item
21ec048 
Fixes: google#1940

Uses the Block State as suggested here:
google#2004 (comment)
@BeksOmega
Copy link
Contributor

Heya @koenvanwijk are you still interested in working on this? I'll check back in a week and close it if not!

@koenvanwijk
Copy link
Author

I found a colleague that works on it. Expect a pull request next week.

@koenvanwijk
Copy link
Author

koenvanwijk commented Jan 8, 2024 via email

@BeksOmega
Copy link
Contributor

I'm going to go ahead and close this PR =) Looking forward to reviewing a new one with the change!

@BeksOmega BeksOmega closed this Jan 12, 2024
@koenvanwijk koenvanwijk deleted the Add-indexing-of-dropdown-items branch January 15, 2024 11:12
@ictin ictin mentioned this pull request Jan 31, 2024
Closed
constantinIliescu added a commit to constantinIliescu/blockly-samples that referenced this pull request Feb 26, 2024
@constantinIliescu
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 constantinIliescu mentioned this pull request Feb 26, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BeksOmega BeksOmega BeksOmega requested changes

Assignees

@BeksOmega BeksOmega

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update toolbox-search to search dropdown field options
5 participants
@koenvanwijk @BeksOmega @ludizhan @savakarrohan @NeilFraser