Variables View in scratchpad should enable searching

RESOLVED FIXED in Firefox 24

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: vporof, Assigned: bbenvie)

Tracking

unspecified
Firefox 24
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Set searchEnabled to true on instance. Optionally provide custom searchPlaceholder.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Posted patch WIP1 (obsolete) — Splinter Review
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?
Summary: Variables View in scratchpad should enable sorting → Variables View in scratchpad should enable searching
Posted patch WIP2 (obsolete) — Splinter Review
Adds search to the Scratchpad VariablesView.
Attachment #748907 - Attachment is obsolete: true
Posted patch WIP3 (obsolete) — Splinter Review
Changes `VariablesView.prototype.rawObject` setter to always sort instead of complicating things by adding a new method.
Attachment #748973 - Attachment is obsolete: true
Anything to do here, Brandon? This has been on the bench for a few weeks.
Posted patch WIP4 (obsolete) — Splinter Review
Rebase and set searchEnabled to true in the constructor flags instead of separately.
Attachment #748975 - Attachment is obsolete: true
Posted patch WIP5 (obsolete) — Splinter Review
Title the patch.

https://tbpl.mozilla.org/?tree=Try&rev=d7b85a7e867e
Attachment #763781 - Attachment is obsolete: true
Posted file WIP6 (obsolete) —
Add search placeholder text.
Attachment #763787 - Attachment is obsolete: true
Posted patch WIP6Splinter Review
Mark it as a patch. <:|
Attachment #763819 - Attachment is obsolete: true
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 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!
Attachment #763822 - Flags: review?(vporof)
(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.
Blocks: 825039
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..
Attachment #763822 - Flags: review?(vporof) → review+
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.
Whiteboard: [land-in-fx-team]
(In reply to Brandon Benvie [:bbenvie] from comment #13)

Right, I forgot about that! Carry on.
https://hg.mozilla.org/integration/fx-team/rev/5b90583b9739
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5b90583b9739
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.