831794 - Variables View: allow users to override getter properties to plain value properties

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Mihai Sucan [:msucan]]

Given a property that has getters and setters you can only edit these. Please provide a UI mechanism that allows the user to entirely override the property with a value, instead of the getter/setter pair.

Comment 1

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

Since it's more or less related to the same implementation quirks, this bug should also allow correct editing of getters and setters.

Comment 2

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

Attachment: v1

This patch makes the getters and setters editable, and also allows the user to override such properties to plain value properties.

Comment 3

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

Attachment: v1.1

Found a case in which removing both the getter and the setter from a property would make it impossible to edit it's value. While technically correct, it breaks user expectations. Fixed this by morphing it into a plain value instead of a property without both a getter and a setter. Added test.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 707518 [details] [diff] [review]
v1.1

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

It confused me initially that I couldn't use the same syntax to (re)declare getters/setters as in object literals, but I guess using a full function should seem reasonable to most users.

One glitch I observed was that the yellow highlight when the value changes wasn't being applied to the getter and setter DOM elements. Another thing probably for a different bug: the close button doesn't do anything when the value is undefined and it seems like it should be hidden in that case.

We should also have the inverse feature as well: turn a value property to a getter/setter. Followup material?

::: browser/devtools/shared/VariablesView.jsm
@@ +872,5 @@
> + *        The user inputted string.
> + * @return string
> + *         The string to be evaluated.
> + */
> +VariablesView.SIMPLE_VALUE_EVAL_MACRO = function(aItem, aCurrentString) {

WHAT'S WITH THE ALL CAPS METHODS? THEY LOOK FUNNY.

@@ +921,5 @@
> +    case "":
> +    case "null":
> +    case "undefined":
> +      let mirrorType = type == "get" ? "set" : "get";
> +      let lookupType = type == "get" ? "__lookupSetter__" : "__lookupGetter__";

Maybe rename it to mirrorLookupType to avoid confusion with the value swap?

Comment 5

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

(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 707518 [details] [diff] [review]
> v1.1
> 
> Review of attachment 707518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It confused me initially that I couldn't use the same syntax to (re)declare
> getters/setters as in object literals, but I guess using a full function
> should seem reasonable to most users.
> 

I.. could wrap the statements in a function declaration block if they're not wrapped already.

> One glitch I observed was that the yellow highlight when the value changes
> wasn't being applied to the getter and setter DOM elements.

But that would change anyway with bug 830818, right? I'm guessing that the evaling doesn't work in that case, so nothing *actually* changes.

> probably for a different bug: the close button doesn't do anything when the
> value is undefined and it seems like it should be hidden in that case.

It does actually do something ("Object.defineProperty.." etc.), however it doesn't change anything. Good catch, I'll fix this here.

> We should also have the inverse feature as well: turn a value property to a
> getter/setter. Followup material?
> 

From what I talked to Mihai, such behavior is better left for the webconsole, since it's going to be pretty clumsy in the variable view. But if anyone complains, definitely, let think about it then.

> ::: browser/devtools/shared/VariablesView.jsm
> @@ +872,5 @@
> > + *        The user inputted string.
> > + * @return string
> > + *         The string to be evaluated.
> > + */
> > +VariablesView.SIMPLE_VALUE_EVAL_MACRO = function(aItem, aCurrentString) {
> 
> WHAT'S WITH THE ALL CAPS METHODS? THEY LOOK FUNNY.
> 

DUNNO MAN I GOT EXCITED.

> @@ +921,5 @@
> > +    case "":
> > +    case "null":
> > +    case "undefined":
> > +      let mirrorType = type == "get" ? "set" : "get";
> > +      let lookupType = type == "get" ? "__lookupSetter__" : "__lookupGetter__";
> 
> Maybe rename it to mirrorLookupType to avoid confusion with the value swap?

Ok.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof [:vp] from comment #5)
> (In reply to Panos Astithas [:past] from comment #4)
> > It confused me initially that I couldn't use the same syntax to (re)declare
> > getters/setters as in object literals, but I guess using a full function
> > should seem reasonable to most users.
> > 
> 
> I.. could wrap the statements in a function declaration block if they're not
> wrapped already.

Sounds tricky to get right. Let's see if anyone complains with what we have now.

> > One glitch I observed was that the yellow highlight when the value changes
> > wasn't being applied to the getter and setter DOM elements.
> 
> But that would change anyway with bug 830818, right? I'm guessing that the
> evaling doesn't work in that case, so nothing *actually* changes.

Hm, then how come the rest of the variable box is highlighted then? Dunno, it looks clunky.

> > We should also have the inverse feature as well: turn a value property to a
> > getter/setter. Followup material?
> > 
> 
> From what I talked to Mihai, such behavior is better left for the
> webconsole, since it's going to be pretty clumsy in the variable view. But
> if anyone complains, definitely, let think about it then.

ΟΚ.

Comment 7

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

Attachment: v1.2

Addressed comments.

Comment 8

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

Comment on attachment 707792 [details] [diff] [review]
v1.2

Sorry, I didn't see your reply before posting the patch. Wrapping the statements in a function declaration works pretty ok with this implementation, let me know if you find anything bizarre.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 707792 [details] [diff] [review]
v1.2

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

::: browser/devtools/shared/VariablesView.jsm
@@ +944,5 @@
> +        "})";
> +
> +    default:
> +      // Wrap statements inside a function declaration if not already wrapped.
> +      if (aCurrentString.indexOf("function") != 0) {

We should strip whitespace from the beginning of the string to make sure we don't inadvertently wrap existing functions.

@@ +945,5 @@
> +
> +    default:
> +      // Wrap statements inside a function declaration if not already wrapped.
> +      if (aCurrentString.indexOf("function") != 0) {
> +        let header = "function(" + (type == "set" ? "value" : "") + ")";

For getters this is fine, but for setters it's not terribly obvious that |value| will be the name of the call parameter. I don't have a better suggestion anyway. Maybe that's a good middle ground, I don't know. And I'm sure that we can make it more discoverable somehow with UI tricks (help bubble with an example that has the missing bits de-emphasized?).

@@ +948,5 @@
> +      if (aCurrentString.indexOf("function") != 0) {
> +        let header = "function(" + (type == "set" ? "value" : "") + ")";
> +        let body = "";
> +        // If there's a return statement explicitly written, use the standard
> +        // block syntax, otherwise prefer an expression closure.

It looks like this doesn't allow me to modify a setter like this:

{ this._prop = value + 'a'; }

Comment 10

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

(In reply to Panos Astithas [:past] from comment #9)
> 
> We should strip whitespace from the beginning of the string to make sure we
> don't inadvertently wrap existing functions.
> 

User inputted string is already trimmed by the time it enters this function. See also the "@param The trimmed user inputted string".

> @@ +945,5 @@
> > +
> > +    default:
> > +      // Wrap statements inside a function declaration if not already wrapped.
> > +      if (aCurrentString.indexOf("function") != 0) {
> > +        let header = "function(" + (type == "set" ? "value" : "") + ")";
> 
> For getters this is fine, but for setters it's not terribly obvious that
> |value| will be the name of the call parameter. I don't have a better
> suggestion anyway. Maybe that's a good middle ground, I don't know. And I'm
> sure that we can make it more discoverable somehow with UI tricks (help
> bubble with an example that has the missing bits de-emphasized?).
> 

Jshint, for example, used to force |value| as the only name for the param in setters. Other (statically typed) languages which support setters have |value| as the way to refer to what's being set. I think it's a pretty rationale choice. If not, you can always type the full setter function :)

> @@ +948,5 @@
> > +      if (aCurrentString.indexOf("function") != 0) {
> > +        let header = "function(" + (type == "set" ? "value" : "") + ")";
> > +        let body = "";
> > +        // If there's a return statement explicitly written, use the standard
> > +        // block syntax, otherwise prefer an expression closure.
> 
> It looks like this doesn't allow me to modify a setter like this:
> 
> { this._prop = value + 'a'; }

Yeah, I could special case that as well.

Comment 11

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

Attachment: v1.3

One could now use { this._prop = value + 'a'; } (for example) to modify a setter.

Comment 12

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

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

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/af82229821f8