Autocomplete array indexes

RESOLVED FIXED in Firefox 30

Status

()

Firefox
Developer Tools: Console
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: msucan, Assigned: Sami Jaktholm)

Tracking

Trunk
Firefox 30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [autocomplete])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Autocomplete array indexes. Or at least allow me to write |someArray[2].foo| and see completion suggestions for |foo| in the array element I want. We can detect \[\d+\]$ at the end of the property name and just go into the array-like object.
(Assignee)

Comment 1

4 years ago
Created attachment 8380170 [details] [diff] [review]
webconsole-array-member-autocompletion.patch

A patch that implements the second idea of the bug: autocompletion for members of arrays. For example 'list[0][1].' will show autocompletion suggestions for the object [0][1] of list.

The patch contains few basic tests for the functionality. When ran locally the patch causes no new devtools test failures. This only supports numeric indices but I think it can be quite simple extend the support for keyed collections too.
Attachment #8380170 - Flags: review?(mihai.sucan)
Nice! +1
(Reporter)

Comment 3

4 years ago
Comment on attachment 8380170 [details] [diff] [review]
webconsole-array-member-autocompletion.patch

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

Thanks for your patch. Great to see someone taking this bug. This is almost working.

When I write myArray['hello'][1] I get suggestions for myArray[1]  - it ignores ['hello'].

Also, instead of adding a new test to the browser/devtools/webconsole folder, can you please write a test in toolkit/devtools/webconsole ? No need to rely on all of the heavy devtools UI - we only care about the input suggestions that are received through the protocol.

::: toolkit/devtools/webconsole/utils.js
@@ +838,5 @@
>      if (!prop) {
>        return null;
>      }
>  
> +    if (/\[\d+\]/.test(prop)) {

This check is too relaxed. How about input like myArray['hello'][1] ? Or myArray[1]['world ' + foo]

@@ +885,5 @@
> +
> +  // Then traverse the list of indices to get the actual element.
> +  let result;
> +  let arrayIndicesRegex = /\[\d+\]/g;
> +  while ((result = arrayIndicesRegex.exec(aProp)) !== null) {

Same as above.
Attachment #8380170 - Flags: review?(mihai.sucan)
(Assignee)

Comment 4

4 years ago
Created attachment 8384143 [details] [diff] [review]
webconsole-array-member-autocompletion-v2.patch

Here's a patch that only adds suggestions for (purely) numerical indices and replaces the mochitests of previous patch with xpcshell tests.

I thought about ways to add support for more complex statements but I'm not that familiar with Firefox internals to be able to implement that. So for the patch I decided to limit the autocompletion for numerical indexes only. At least it won't give incorrect suggestions (hopefully). If anyone is interested to extend this, feel free to pick this up.
Attachment #8380170 - Attachment is obsolete: true
Attachment #8384143 - Flags: review?(mihai.sucan)
(Reporter)

Comment 5

4 years ago
Comment on attachment 8384143 [details] [diff] [review]
webconsole-array-member-autocompletion-v2.patch

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

Thanks for the update patch. This is close to landable. Minor comments below.

::: toolkit/devtools/webconsole/test/unit/test_js_property_provider.js
@@ +2,5 @@
> +// Any copyright is dedicated to the Public Domain.
> +// http://creativecommons.org/publicdomain/zero/1.0/
> +
> +let devtools = Components.utils.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> +let JSPropertyProvider = devtools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;

nit: "use strict"; for the test.

(really nice test!)

::: toolkit/devtools/webconsole/utils.js
@@ +837,5 @@
>      if (!prop) {
>        return null;
>      }
>  
> +    if (/(\[\d+\])+$/.test(prop)) {

This doesn't need to be a capturing regex. Please change to /\[\d+\]$/.

@@ +860,5 @@
>    return getMatchedPropsInDbgObject(obj, matchProp);
>  }
>  
>  /**
> + * Finds the object aProp of aObj if aProp points to an element of an array. For

nit: maybe change this paragraph to 'Get the array member of aObj for the given aProp.'

@@ +875,5 @@
> + */
> +function getArrayMemberProperty(aObj, aProp) {
> +  // First get the array.
> +  let obj = aObj;
> +  let propWithoutIndices = aProp.split("[")[0];

split() does more than we need here. Please use indexOf("[") then substr().

@@ +883,5 @@
> +  }
> +
> +  // Then traverse the list of indices to get the actual element.
> +  let result;
> +  let arrayIndicesRegex = /\[[^\]]+\]/g;

This regex doesn't need the /g flag. Also, this regex has an error.

If you write: |myArray[][1].| we can see completion results for myArray[1] because the regex includes the +, it requires something inside the brackets.

Maybe /\[[^\]]*\]/ works.

(please update the test to cover this case as well.)
Attachment #8384143 - Flags: review?(mihai.sucan)
(Reporter)

Updated

4 years ago
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
Created attachment 8386226 [details] [diff] [review]
webconsole-array-member-autocompletion-v3.patch

Thanks for the review. Here's a patch that addresses your comments.

The arrayIndicesRegex actually requires the 'g' modifier, at least according to MDN[1] (doesn't seem to work without it)...

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec#Example.3A_Finding_successive_matches
Attachment #8384143 - Attachment is obsolete: true
Attachment #8386226 - Flags: review?(mihai.sucan)
(Reporter)

Comment 7

4 years ago
(In reply to Sami Jaktholm from comment #6)
> Created attachment 8386226 [details] [diff] [review]
> webconsole-array-member-autocompletion-v3.patch
> 
> Thanks for the review. Here's a patch that addresses your comments.

Thanks for the update!


> The arrayIndicesRegex actually requires the 'g' modifier, at least according
> to MDN[1] (doesn't seem to work without it)...
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/RegExp/exec#Example.3A_Finding_successive_matches

That's an exec() trick I didnt know of. One always learns. Thanks! :)
(Reporter)

Comment 8

4 years ago
Comment on attachment 8386226 [details] [diff] [review]
webconsole-array-member-autocompletion-v3.patch

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

Patch looks good. r+

Try push: https://tbpl.mozilla.org/?tree=Try&rev=91e1e1504721

Once that's green, we can land the patch. Thanks for your good work!
Attachment #8386226 - Flags: review?(mihai.sucan) → review+
(Reporter)

Comment 9

4 years ago
Try push was green.

Landed: https://hg.mozilla.org/integration/fx-team/rev/67ccb76be87c

Thanks!
Whiteboard: [autocomplete] → [autocomplete][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/67ccb76be87c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [autocomplete][fixed-in-fx-team] → [autocomplete]
Target Milestone: --- → Firefox 30

Updated

3 years ago
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.