963919 - When triggering file filtering via a keyboard shortcut, don't automatically fill the searchbox with the identifier under the caret position

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Victor Porof [:vporof][:vp]]

If the caret is inside "foo" in the source editor and you Cmd+P, you'll end up searching for a file that contains "foo" in its name. This is probably *never* what you actually want. We shouldn't autocomplete this.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: dbg-cmd-p.patch

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v2

There was actually another problem as well: operators weren't being added anymore when hitting keyboard shortcuts like Cmd+F or Cmd+L and no identifier was underneath the cursor. Regression from bug 911916.

This should fix it. More tests. More.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8365523 [details] [diff] [review]
v2

Review of attachment 8365523 [details] [diff] [review]:
-----------------------------------------------------------------

Nice fix. There is only one thing that doesn't make sense to me, the different treatment between Ctrl-F & Ctrl-Alt-F. Also, I noticed that even with this patch pressing Ctrl-P when a string is selected will display both the file matches and the help panel. Do you think we could fix that in this bug, or should we file a separate one?

::: browser/devtools/debugger/debugger-toolbar.js
@@ +1066,3 @@
>     *        The operator to use for filtering.
>     */
>    _doSearch: function(aOperator = "", aText = "") {

Not an oversight of this patch, but since you are here, could you add a method comment about aText please?

@@ +1079,2 @@
>      }
> +    if ([SEARCH_GLOBAL_FLAG, SEARCH_FUNCTION_FLAG].indexOf(aOperator) != -1) {

Why SEARCH_GLOBAL_FLAG, but not SEARCH_TOKEN_FLAG? They are both token-matching searches that only differ in scope. I could understand allowing neither or both, but this seems counter-intuitive.

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #3)
> Comment on attachment 8365523 [details] [diff] [review]
> v2
> 
> Review of attachment 8365523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice fix. There is only one thing that doesn't make sense to me, the
> different treatment between Ctrl-F & Ctrl-Alt-F.

When searching for a string inside a file, I found it that it's usually *not* what you're hovering with the cursor. However, when searching across multiple files, it's usually the opposite (for me).

I don't know what's the optimal solution that would satisfy all our users here. I'm somewhat inclining to disable both, because the likely answer is "we don't know". What do you think?

> Also, I noticed that even
> with this patch pressing Ctrl-P when a string is selected will display both
> the file matches and the help panel. Do you think we could fix that in this
> bug, or should we file a separate one?
> 

I think that should be a separate bug, since it's not related to this one in any direct way.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof [:vp] from comment #4)
> When searching for a string inside a file, I found it that it's usually
> *not* what you're hovering with the cursor. However, when searching across
> multiple files, it's usually the opposite (for me).

Interestingly, it's the other way around for me. My favorite keyboard shortcut in Sublime Text is Cmd-Opt-G, which does a search for the current identifier, usually to find call sites (and the fact that Mac & Linux have different keybindings for this, annoys me to no end). I sometimes want to search for the current identifier everywhere too, but that is less common in my experience.

> I don't know what's the optimal solution that would satisfy all our users
> here. I'm somewhat inclining to disable both, because the likely answer is
> "we don't know". What do you think?

I'm +0.5 on enabling both, but I don't really have a strong opinion on this. What I think would make for a better UX is to enable it, but have the identifier pre-selected in the search box, so it can be deleted with a single keystroke if that was not the user's intention.

> I think that should be a separate bug, since it's not related to this one in
> any direct way.

Filed bug 964205.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v3

Addressed comments.

Comment 7

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/e2aa30eb2649

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2aa30eb2649