Bug 93166 – Web Inspector: Select the shortest match in the FilteredItemSelectionDialog

• Component: Web Inspector (Deprecated)
• Status: RESOLVED DUPLICATE

[johnjbarton]

When I'm using the fabulous FilteredItemSelectionDialog I often mis-select my file. I believe this happens when I know which file I want so I quickly type the name and hit enter. But I end up in another file.

For example, open devtools on (source, not built) devtools, open the Sources panel, control O to bring up the dialog, place the mouse on the center of the dialog, escape the dialog, enter it again with control O, then type Script.js. I get all kinds of different answers. (Linux FWIW).

Just to avoid this, I propose we default to the shortest match rather than the first match.

[johnjbarton]

Created attachment 156478 [details]
Patch

[johnjbarton]

To test this seems to require creating a dialog and operating it as far as I can tell.

[Vsevolod Vlasov]

I don't think this is a proper way to fix this.
This problem is a result of the http://trac.webkit.org/changeset/116244 which made filtering less restrictive.
Initially filtering was working similarly to eclipse and supported camel case search (e.g. AdSC for AdvancedSearchController).
http://trac.webkit.org/changeset/116244 made it work similarly to Sublime and made camel case search almost unusable (too many false positive results).
I would revert r116244 but I'd like to hear other opinions on that.
John, what do you think?

[johnjbarton]

(In reply to comment #3)
> I don't think this is a proper way to fix this.
> This problem is a result of the http://trac.webkit.org/changeset/116244 which made filtering less restrictive.

(FFR, BUG was https://bugs.webkit.org/show_bug.cgi?id=85586)

> Initially filtering was working similarly to eclipse and supported camel case search (e.g. AdSC for AdvancedSearchController).
> http://trac.webkit.org/changeset/116244 made it work similarly to Sublime and made camel case search almost unusable (too many false positive results).
> I would revert r116244 but I'd like to hear other opinions on that.
> John, what do you think?

I really like the emacs solution to this dilemma: 

If the user makes the effort to Capitalize, then the user means "match this using Capital letters". Lower case implies case-insensitive match. A subtle issue is whether any Cap forces all case sensitive or just on the one letter.

If we implement such a rule, then we solve your complaint (and the specific example above) and retain the advantage sought by bug 85586.

What do you think?

[Vsevolod Vlasov]

Does it solve your complaint as well?
Can you give me some examples of what you currently dislike and how it will be fixed with this approach?

[johnjbarton]

(In reply to comment #5)
> Does it solve your complaint as well?

No, but it turns out that the current code already implements the Caps-selected-by-caps rule. So perhaps you can try your example and decide that CamelCase support is actually ok or what about it is not good?

> Can you give me some examples of what you currently dislike and how it will be fixed with this approach?

You mean besides the case in my original post? That case is typical of the problem: often files share a common base name so you can't select the base name just by typing it explicitly and hitting enter. My patch addresses selection, not filtering: select the best match, not the first one.

[johnjbarton]

One example provided by Jan Keromnes relates to this issue:

Open devtools on (source) devtools, control O then type 

extensionA

The only item is:

DevToolsExtensionAPI.js

But it's puzzling to a user who is looking for 

ExtensionAPI.js

Two solutions:

1) Only match case on explicit upper case letters.
   In the case above we have three hits, ExtensionAPI.js, ExtensionAuditPanel.js and DevtoolsExtensionAPI.js

2) Change the highlighting to only highlight the letters that the user typed, not the regex generated under the covers. 
   In the case above the highlighting would be
DevToolsExtensionAPI.js
_e_______xtensionA_____

rather than the current
DevToolsExtensionAPI.js
_evToolsExtensionA_____

I think this would direct the users attention to the lower case 'e'.

Not a major issue for me and a separate bug in any case.

[johnjbarton]

Created attachment 157564 [details]
Patch

[johnjbarton]

Here is a better version, and a test for it.

Now we select the shortest regex match, so we favor correct consecutive characters, and we break ties by selecting the shortest strings. Eg if the choices are

AStringClass.js
String.js
NotMatched.js
ZStringClass.js

and the search is "String" we pick String.js.

For scale-ability we only select among the items in the viewport. I think this is ok, it will only matter for cases with lots of nearly identical entries.

I'd look into the issue of CamelCase matching if we had a test case....

[johnjbarton]

Created attachment 162305 [details]
Patch

[johnjbarton]

I believe the test I add in this patch would help in testing Bug 95481

[WebKit Review Bot]

Comment on attachment 162305 [details]
Patch

Attachment 162305 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13764611

New failing tests:
inspector/styles/protocol-css-regions-commands.html

[johnjbarton]

(In reply to comment #12)
...
> New failing tests:
> inspector/styles/protocol-css-regions-commands.html

I don't understand this result. The test passes for me; the test has nothing to do with FilteredItemSelectionDialog; the test is very new, from Bug 93443.

What to do next? Perhaps just try the commit-queue again?

[johnjbarton]

(In reply to comment #13)
> What to do next? Perhaps just try the commit-queue again?

I learned that the green ovals means the patch passes the 'early warning system' and the comment #12 is just bogus because another patch landed that gave flaky results.  So I just need to wait for review.

[Vsevolod Vlasov]

I still think we need to fix the the regexp used to find matching entries itself. I have a patch for that ready and consider this issue invalid.

[Vsevolod Vlasov]

Comment on attachment 162305 [details]
Patch

Clearing r?

[Vsevolod Vlasov]

https://bugs.webkit.org/show_bug.cgi?id=97332

[johnjbarton]

(In reply to comment #15)
> I still think we need to fix the the regexp used to find matching entries itself. I have a patch for that ready and consider this issue invalid.

I would appreciate a technical explanation for why this issue is invalid. The regexp itself is a different issue. I believe users may be split as we are on which regexp approach is best but that should be addressed on another bug.

[Vsevolod Vlasov]

1. I think selecting a match other than the first one is misleading.
2. I don't think that the shortest match is necessarily the best one.
3. This patch adds some complexity to the code which will be redundant once we change the regular expression to match from the beginning of the string (I consider this as the main reason of false positives).

[johnjbarton]

(In reply to comment #19)
> 1. I think selecting a match other than the first one is misleading.

The order of the items in the match list has no significance. The first one is not special in any way. Thus selecting the first one is effectively random.

> 2. I don't think that the shortest match is necessarily the best one.

The shortest match is not always the one the user seeks, but it is much more likely to be the one the user seeks than any other item and in particular it is more likely than the random first item.

The shortest match algorithm allows users to filter the list by words they recall. For example "script" will bring up all files related to "script". This allows users to use partial knowledge to get a browse-able list they can refine or click on.

> 3. This patch adds some complexity to the code which will be redundant once we change the regular expression to match from the beginning of the string (I consider this as the main reason of false positives).

Unfortunately Bug 97332 is not a good direction. The entries in the item list are *not* false positives. They are potential selections filtered by the bits of knowledge the user recalls. 

Compare the Chrome Omnibox to the location bar from olden days. In the olden days we had to type "http://blah.blah.etc" to navigate. Now we type things we recall and get a completion dialog prompting our memory. Bug 97322 takes from the future into the past.

I hope you will reconsider.

[johnjbarton]

Reopening to attach new patch.

[johnjbarton]

Created attachment 166286 [details]
Bug fixes, in case this is ever reconsidered; also req. revert 97332

[Vsevolod Vlasov]

We are working on something similar to that in https://bugs.webkit.org/show_bug.cgi?id=108346. I am closing this one as a duplicate now.

*** This bug has been marked as a duplicate of bug 108346 ***