Highlight matches in CB #8098

somebody1234 · 2023-10-18T07:54:45Z

Pull Request Description

Closes Highlight matched letters in CB #8062

Important Notes

This PR uses a completely separate implementation from the regexes used for the existing filtering. AFAICT, this is unavoidable, since this needs to retrieve matched text.
I've come across a couple minor bugs related to the Component Browser:
- When deleting text (backspacing), the full list of unfiltered entries momentarily reappear because the filtering is reset.
- Things like h.m match HTTP_Method - if this is not intentional, it seems like it might be more difficult to fix, unfortunately.
  - Note that highlighting (I assume correctly?) does not highlight any part of HTTP_Method.

Screencast

highlight_matches.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

~~The documentation has been updated, if necessary.~~
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- ~~Unit tests have been written where possible.~~
- ~~If GUI codebase was changed, the GUI was tested when built using ./run ide build.~~

somebody1234 · 2023-10-18T07:58:30Z

... actually, this implementation does not work when the module name is hidden (so, for submodules)

farmaazon · 2023-10-18T10:01:10Z

Things like h.m match HTTP_Method - if this is not intentional, it seems like it might be more difficult to fix, unfortunately.

Note that highlighting (I assume correctly?) does not highlight any part of HTTP_Method.

Yes. The thing is that we always try to match the qualified path of given entry, even of those parts which are not visible. In this case, HTTP_Method is inside HTTP module, so h matches first letter of HTTP module and m matches sixth letter of HTTP_Method. The module is not displayed, so we should highlight HTTP_Method

somebody1234 · 2023-10-18T10:03:02Z

@farmaazon yeah, I realized that later. it should now work correctly in the current version

app/gui2/src/components/ComponentBrowser/component.ts

app/gui2/src/components/ComponentBrowser.vue

farmaazon · 2023-10-18T10:53:17Z

app/gui2/src/components/ComponentBrowser.vue

+  const suffix = match[match.length - 1]
+  if (suffix) yield { text: suffix, type: 'no-match' }
+}
+


I'd highly prefer to have the algorithm for obtaining matched letters somewhere as API for Filtering class, even as part of filter method. If the filtering/scoring algorithm will change (and it might), one would not have to remember about another place in the code to fix.

I think the filter method should return matched ranges, and label field of Compnent type could have information about matched letters in the label only. The code should be unit tested.

@farmaazon wdym by this?

label field of Component type could have information about matched letters in the label only

(do you mean the match field should have information about matched letters in the label only?)

somebody1234 · 2023-10-18T13:26:42Z

... oops, the new version doesn't quite work

somebody1234 · 2023-10-18T13:28:48Z

... oh, it's also missing unit tests

app/gui2/src/components/ComponentBrowser/filtering.ts

app/gui2/src/util/database/__tests__/reactiveDb.test.ts

farmaazon · 2023-10-19T07:33:30Z

app/gui2/src/util/qualifiedName.ts

@@ -64,7 +70,7 @@ export function qnJoin(left: QualifiedName, right: QualifiedName): QualifiedName
 * The element is considered a top element if there is max 1 segment in the path.
 */
 export function qnIsTopElement(name: QualifiedName): boolean {
-  return (name.match(/\./g)?.length ?? 0) <= 2
+  return !/[.].*?[.].*?[.]/.test(name)


I'm ok with this change, only I'm curious why it is here? It is more performant, or something?

i haven't confirmed whether it's more important, but: it doesn't have to store the match strings, nor create an array, nor are there any conditionals caused by optional chaining
so at the very least i don't think it's less performant

farmaazon · 2023-10-19T07:34:51Z

app/gui2/src/components/ComponentBrowser/component.ts

+interface ComponentLabel {
+  label: string
+  matchedAlias?: Opt<string>
+  matchedRanges?: MatchRange[]


We will also want to highlight letters in matched alias at some point.

yeah - this is why (i'm pretty sure) matchedRanges is present even when matchedAlias is present. i can add tests to verify the ranges are correct when the matchedAlias is present?

tests for matchedAlias have now been added (which did catch a bug)

app/gui2/src/components/ComponentBrowser/__tests__/filtering.test.ts

somebody1234 · 2023-10-19T08:32:35Z

@Frizi i think this should be ready for your review now

Frizi · 2023-10-19T11:20:47Z

app/gui2/src/util/database/__tests__/reactiveDb.test.ts

+  const actualConsoleError = console.error
+  console.error = () => {}
+  try {
+    const consoleError = vi.spyOn(console, 'error')
+    // Invalid index
+    new ReactiveIndex(db, (_id, _entry) => [[1, 1]])
+    db.set(1, 1)
+    db.set(2, 2)
+    expect(consoleError).toHaveBeenCalledOnce()
+    expect(consoleError).toHaveBeenCalledWith(
+      'Attempt to repeatedly write the same key-value pair (1,1) to the index. Please check your indexer implementation.',
+    )
+  } finally {
+    console.error = actualConsoleError
+  }


Instead of this manual spy restore, it's better to enable automatic mock restoring before each test.

https://vitest.dev/config/#restoremocks

…ocks`

Highlight matches in CB

ec74239

somebody1234 added CI: No changelog needed Do not require a changelog entry for this PR. -gui labels Oct 18, 2023

somebody1234 requested a review from Frizi as a code owner October 18, 2023 07:54

somebody1234 marked this pull request as draft October 18, 2023 07:58

somebody1234 added 2 commits October 18, 2023 18:22

Make all match segments optional

f5510c5

Properly nest optional parts of regex

0888b6a

somebody1234 marked this pull request as ready for review October 18, 2023 09:03

farmaazon requested changes Oct 18, 2023

View reviewed changes

Refactor as suggested by review

bc02446

somebody1234 marked this pull request as draft October 18, 2023 14:34

somebody1234 added 3 commits October 19, 2023 15:16

Tighter integration of match indices with filtering

14c20b5

Fix regressions in existing tests

0e198e4

Add new tests

66c333c

somebody1234 requested a review from farmaazon October 19, 2023 06:27

somebody1234 marked this pull request as ready for review October 19, 2023 06:28

farmaazon approved these changes Oct 19, 2023

View reviewed changes

somebody1234 added 2 commits October 19, 2023 18:20

Address review

01eb0f0

Improve tests using review feedback

b594879

somebody1234 requested a review from farmaazon October 19, 2023 08:29

farmaazon approved these changes Oct 19, 2023

View reviewed changes

Refactor allRanges out to separate file

21f8fc4

Frizi approved these changes Oct 19, 2023

View reviewed changes

Switch from manual monkeypatching to mockImplementation + `restoreM…

e68257d

…ocks`

somebody1234 added the CI: Ready to merge This PR is eligible for automatic merge label Oct 19, 2023

mergify bot merged commit 895326d into develop Oct 19, 2023

mergify bot deleted the wip/sb/cb-highlight-matches branch October 19, 2023 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight matches in CB #8098

Highlight matches in CB #8098

somebody1234 commented Oct 18, 2023 •

edited

Loading

somebody1234 commented Oct 18, 2023

farmaazon commented Oct 18, 2023

somebody1234 commented Oct 18, 2023

farmaazon Oct 18, 2023 •

edited

Loading

somebody1234 Oct 18, 2023

somebody1234 Oct 18, 2023

somebody1234 commented Oct 18, 2023

somebody1234 commented Oct 18, 2023

farmaazon Oct 19, 2023

somebody1234 Oct 19, 2023

farmaazon Oct 19, 2023

somebody1234 Oct 19, 2023

somebody1234 Oct 19, 2023

somebody1234 commented Oct 19, 2023

Frizi Oct 19, 2023

Highlight matches in CB #8098

Highlight matches in CB #8098

Conversation

somebody1234 commented Oct 18, 2023 • edited Loading

Pull Request Description

Important Notes

Screencast

Checklist

somebody1234 commented Oct 18, 2023

farmaazon commented Oct 18, 2023

somebody1234 commented Oct 18, 2023

farmaazon Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somebody1234 commented Oct 18, 2023

somebody1234 commented Oct 18, 2023

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

somebody1234 commented Oct 19, 2023

Choose a reason for hiding this comment

somebody1234 commented Oct 18, 2023 •

edited

Loading

farmaazon Oct 18, 2023 •

edited

Loading