869984 - Variables View in scratchpad should enable searching

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

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

Set searchEnabled to true on instance. Optionally provide custom searchPlaceholder.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: WIP1

This patch:

* `VariablesView.prototype.setRawObject` method which allows setting options
* Updates `set VariablesView.prototype.rawObject` to use the above method
* Updates the Scratchpad to sort the properties of the inspected rawObject.

@vp the title of this bug says to sort, but the content of your comment describes how to enable searching. Perhaps you meant both?

Comment 2

[Brandon Benvie [:benvie]]

Attachment: WIP2

Adds search to the Scratchpad VariablesView.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: WIP3

Changes `VariablesView.prototype.rawObject` setter to always sort instead of complicating things by adding a new method.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Anything to do here, Brandon? This has been on the bench for a few weeks.

Comment 5

[Brandon Benvie [:benvie]]

Attachment: WIP4

Rebase and set searchEnabled to true in the constructor flags instead of separately.

Comment 6

[Brandon Benvie [:benvie]]

Attachment: WIP5

Title the patch.

https://tbpl.mozilla.org/?tree=Try&rev=d7b85a7e867e

Comment 7

[Brandon Benvie [:benvie]]

Attachment: WIP6

Add search placeholder text.

Comment 8

[Brandon Benvie [:benvie]]

Attachment: WIP6

Mark it as a patch. <:|

Comment 9

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

Comment on attachment 763822 [details] [diff] [review]
WIP6

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +1534,5 @@
>          let container = window.document.querySelector("#variables");
> +        this.variablesView = new VariablesView(container, {
> +          searchEnabled: true,
> +          searchPlaceholder: this._scratchpad.strings
> +                             .GetStringFromName("propertiesFilterPlaceholder")

At some point, if you wish, you might want to convert those strings to the L10N helper [0] in ViewHelpers.jsm [1], it makes things look prettier.

[0] https://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#207

[1] https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1509

Comment 10

[Brandon Benvie [:benvie]]

Comment on attachment 763822 [details] [diff] [review]
WIP6

(In reply to Victor Porof [:vp] from comment #9)
> At some point, if you wish, you might want to convert those strings to the
> L10N helper [0] in ViewHelpers.jsm [1], it makes things look prettier.

Good idea, filed bug 884403.

Also, I'm putting you as r?. Let me know if you'd like me to find someone else to review it!

Comment 11

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

(In reply to Brandon Benvie [:bbenvie] from comment #10)
> 
> Also, I'm putting you as r?. Let me know if you'd like me to find someone
> else to review it!

OH I AM SO ANGRY RIGHT NOW.
Sure man, feel free to ask me as many r? as you want.

Comment 12

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

Comment on attachment 763822 [details] [diff] [review]
WIP6

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +89,5 @@
>     *        if you want the variables view to work in sync mode.
>     */
>    set rawObject(aObject) {
>      this.empty();
> +    this.addScope().addItem().populate(aObject, { sorted: true });

Why this change? I don't have anything against it though..

Comment 13

[Brandon Benvie [:benvie]]

Thanks for the review!

(In reply to Victor Porof [:vp] from comment #12)
> Comment on attachment 763822 [details] [diff] [review]
> WIP6
> 
> Review of attachment 763822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +89,5 @@
> >     *        if you want the variables view to work in sync mode.
> >     */
> >    set rawObject(aObject) {
> >      this.empty();
> > +    this.addScope().addItem().populate(aObject, { sorted: true });
> 
> Why this change? I don't have anything against it though..

The title of this bug originally was "...sorting" and that was the first thing I did. Also, you mentioned in IRC that the VariablesView should already be sorted by default, which it isn't when setting a rawObject, so I figured the intent was for it to be like this.

Comment 14

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

(In reply to Brandon Benvie [:bbenvie] from comment #13)

Right, I forgot about that! Carry on.

Comment 15

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

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

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5b90583b9739