Closed Bug 963919 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: vporof, Assigned: vporof)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch dbg-cmd-p.patch (obsolete) — Splinter Review
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8365516 - Flags: review?(past)
Attached patch v2Splinter Review
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.
Attachment #8365516 - Attachment is obsolete: true
Attachment #8365516 - Flags: review?(past)
Attachment #8365523 - Flags: review?(past)
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.
Attachment #8365523 - Flags: review?(past) → review+
(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.
Flags: needinfo?(past)
(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.
Flags: needinfo?(past)
Attached patch v3Splinter Review
Addressed comments.
Attachment #8366546 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e2aa30eb2649
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.