Last Comment Bug 762452 - file filtering behaves strangely
: file filtering behaves strangely
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 14 Branch
: All All
: P1 normal (vote)
: Firefox 16
Assigned To: Victor Porof [:vporof][:vp]
:
: James Long (:jlongster)
Mentors:
http://addyosmani.github.com/todomvc/...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-07 05:28 PDT by Kevin Dangoor
Modified: 2012-08-07 06:36 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
v1 (3.13 KB, patch)
2012-06-16 04:56 PDT, Victor Porof [:vporof][:vp]
mihai.sucan: feedback+
Details | Diff | Splinter Review
v2 (11.89 KB, patch)
2012-06-16 09:07 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review
v3 (13.09 KB, patch)
2012-06-19 08:12 PDT, Victor Porof [:vporof][:vp]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Kevin Dangoor 2012-06-07 05:28:11 PDT
STR:

1. open http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.html
2. open debugger
3. In filter box, type "todo"
4. *** Note that selected file is still backbone-localstorage.js, which doesn't make sense
5. add an "s" to the filter box
6. it jumps to todos.js
7. add "@rema" to filter box
8. it jumps to remaining: function()
9. add "i" to filter box
10. *** it goes back to the beginning of the file
11. add "n" to the filter box
12. it just back to remaining: function
13. add "i" to the filter box and it jumps back to the top
Comment 1 Panos Astithas [:past] 2012-06-07 07:55:40 PDT
(In reply to Kevin Dangoor from comment #0)
> STR:
> 
> 1. open
> http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.
> html
> 2. open debugger
> 3. In filter box, type "todo"
> 4. *** Note that selected file is still backbone-localstorage.js, which
> doesn't make sense

Yes, we match on the URL, not the listbox label. That's an easy fix.

> 5. add an "s" to the filter box
> 6. it jumps to todos.js
> 7. add "@rema" to filter box
> 8. it jumps to remaining: function()
> 9. add "i" to filter box
> 10. *** it goes back to the beginning of the file
> 11. add "n" to the filter box
> 12. it just back to remaining: function
> 13. add "i" to the filter box and it jumps back to the top

These jumps to the top are weird though.
Comment 2 Kevin Dangoor 2012-06-07 08:49:51 PDT
(In reply to Panos Astithas [:past] from comment #1)
> Yes, we match on the URL, not the listbox label. That's an easy fix.

Aha. That makes sense. Working from the listbox label will certainly be less surprising.

> > 5. add an "s" to the filter box
> > 6. it jumps to todos.js
> > 7. add "@rema" to filter box
> > 8. it jumps to remaining: function()
> > 9. add "i" to filter box
> > 10. *** it goes back to the beginning of the file
> > 11. add "n" to the filter box
> > 12. it just back to remaining: function
> > 13. add "i" to the filter box and it jumps back to the top
> 
> These jumps to the top are weird though.

Right, the file selection was a little surprising, but the bouncing back and forth was clearly a bug.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-06-12 08:37:59 PDT
I think we'll want this for aurora.
Comment 4 Victor Porof [:vporof][:vp] 2012-06-16 04:56:26 PDT
Created attachment 633796 [details] [diff] [review]
v1

Alright, so this works properly now, but it's a bit baffling to me.
Mihai, can you please take a look at the diff:

+      let currentOffset = editor.getCaretOffset();
       let offset = editor.find(token, { ignoreCase: true });
-      if (offset > -1) {
+
+      if (offset > -1 && offset !== currentOffset) {
         editor.setCaretPosition(0);
         editor.setCaretOffset(offset);
       }

Basically, as expected, the previous behavior would give me the same caret offset as the previous offset when I searched for something and found it (hence, the caret should've remained at the same position), but subsequent calls to setCaretPosition with the same param would make it jump to frenetically. Was this the expected behavior of the function?
Comment 5 Victor Porof [:vporof][:vp] 2012-06-16 04:58:08 PDT
(In reply to Kevin Dangoor from comment #0)

Just a note, in bug 762454 "@" was replaced with "#".
Comment 6 Mihai Sucan [:msucan] 2012-06-16 08:11:31 PDT
Comment on attachment 633796 [details] [diff] [review]
v1

This fix looks good to me. The setCaretPosition(0) call should be removed - just do setCaretOffset(offset). Or even better: setSelection(offset, offset + token.length).
Comment 7 Victor Porof [:vporof][:vp] 2012-06-16 09:07:54 PDT
Created attachment 633819 [details] [diff] [review]
v2

Thanks msucan! Made the change.
Comment 8 Panos Astithas [:past] 2012-06-18 08:38:53 PDT
Comment on attachment 633819 [details] [diff] [review]
v2

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

::: browser/devtools/debugger/test/browser_dbg_scripts-searching-01.js
@@ +178,4 @@
>    for (let i = 0; i < text.length; i++) {
>      EventUtils.sendChar(text[i]);
>    }
> +  dump("Editor caret position: " + gEditor.getCaretPosition().toSource() + "\n");

Nit: turn the dump() into info() if it's still useful.

::: browser/devtools/debugger/test/browser_dbg_scripts-searching-02.js
@@ +144,4 @@
>    for (let i = 0; i < text.length; i++) {
>      EventUtils.sendChar(text[i]);
>    }
> +  dump("Editor caret position: " + gEditor.getCaretPosition().toSource() + "\n");

Ditto.
Comment 9 Victor Porof [:vporof][:vp] 2012-06-18 08:39:22 PDT
Thanks!
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 17:04:46 PDT
No need to track this, just go ahead and nominate the patch for uplift when you're ready.
Comment 11 Victor Porof [:vporof][:vp] 2012-06-19 08:12:06 PDT
Created attachment 634425 [details] [diff] [review]
v3

Addressed comments.
Comment 12 Panos Astithas [:past] 2012-06-19 09:14:06 PDT
https://hg.mozilla.org/integration/fx-team/rev/543030f7b43b
Comment 13 Panos Astithas [:past] 2012-06-21 00:57:24 PDT
https://hg.mozilla.org/mozilla-central/rev/543030f7b43b
Comment 14 Panos Astithas [:past] 2012-06-22 03:25:05 PDT
Comment on attachment 634425 [details] [diff] [review]
v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: the script filtering/selection box will misbehave in certain cases
Testing completed (on m-c, etc.): On m-c and fx-team
Risk to taking this patch (and alternatives if risky): it's a small patch (leaving tests aside) and concerns a tool that only developers will ever use
String or UUID changes made by this patch: none
Comment 16 Ioana (away) 2012-08-06 07:28:28 PDT
I tried to verify this fix on Firefox 15 beta 3 (20120731150526) and it seems it's only fixed on Linux x64. On all the other OSs (Win 7 x86/x64, Linux x32, Mac 10.7 x86/x64), the filtering works fine until you add the "@", at which point it stops working.
Comment 17 Victor Porof [:vporof][:vp] 2012-08-06 08:08:30 PDT
(In reply to Ioana Budnar [QA] from comment #16)
> I tried to verify this fix on Firefox 15 beta 3 (20120731150526) and it
> seems it's only fixed on Linux x64. On all the other OSs (Win 7 x86/x64,
> Linux x32, Mac 10.7 x86/x64), the filtering works fine until you add the
> "@", at which point it stops working.

We're not using @ anymore. Bug 762454.
Comment 18 Victor Porof [:vporof][:vp] 2012-08-06 08:12:40 PDT
Also, the testpage (http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.html) has changed since comment #1 so the STR don't apply exactly the same way anymore.
Comment 19 Ioana (away) 2012-08-07 01:24:06 PDT
(In reply to Victor Porof [:vp] from comment #18)
> Also, the testpage
> (http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.
> html) has changed since comment #1 so the STR don't apply exactly the same
> way anymore.

Victor, I took that into account when verifying this fix. When entering "todos" in the filtering textbox, two scripts fit the filter and you have to manually choose "todos.js". After doing that, you can just follow the rest of the STR.

Either way, filtering with "@" works on Linux x64 (I used Ubuntu 12.04) and it doesn't work on the other OSs. This is an inconsistency that needs fixing. If you don't want to fix it here, let me know and I could reopen bug 762454 and mark this one as verified.
Comment 20 Victor Porof [:vporof][:vp] 2012-08-07 04:46:00 PDT
(In reply to Ioana Budnar [QA] from comment #19)
> (In reply to Victor Porof [:vp] from comment #18)
> 
> Either way, filtering with "@" works on Linux x64 (I used Ubuntu 12.04) and
> it doesn't work on the other OSs. This is an inconsistency that needs
> fixing. If you don't want to fix it here, let me know and I could reopen bug
> 762454 and mark this one as verified.

That's... unlikely. If "@" would work then the tests on tbpl should be permaorange on Linux. What FF version are you testing with?
Comment 21 Ioana (away) 2012-08-07 06:36:13 PDT
"@" worked for me no Ubuntu 12.04 64-bit:
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0
BuildID: 20120731150526

I can't reproduce it too often though.

Note You need to log in before you can comment on or make changes to this bug.