943586 - Autocomplete array indexes

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mihai Sucan [:msucan]]

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.

Comment 1

[Sami Jaktholm]

Attachment: 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.

Comment 2

[Girish Sharma [:Optimizer]]

Nice! +1

Comment 3

[Mihai Sucan [:msucan]]

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.

Comment 4

[Sami Jaktholm]

Attachment: 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.

Comment 5

[Mihai Sucan [:msucan]]

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.)

Comment 6

[Sami Jaktholm]

Attachment: 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

Comment 7

[Mihai Sucan [:msucan]]

(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! :)

Comment 8

[Mihai Sucan [:msucan]]

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!

Comment 9

[Mihai Sucan [:msucan]]

Try push was green.

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

Thanks!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67ccb76be87c