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

RESOLVED FIXED in Firefox 21

Status

P3
normal
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: msucan, Assigned: vporof)

Tracking

Trunk
Firefox 21
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: Variables View: allow users to override properties with getters → Variables View: allow users to override getter properties to plain value properties
(Assignee)

Updated

6 years ago
Assignee: nobody → vporof
(Assignee)

Comment 1

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

Comment 2

6 years ago
Created attachment 706871 [details] [diff] [review]
v1

This patch makes the getters and setters editable, and also allows the user to override such properties to plain value properties.
Attachment #706871 - Flags: review?(past)
(Assignee)

Updated

6 years ago
Depends on: 828987
(Assignee)

Updated

6 years ago
Blocks: 830759
(Assignee)

Comment 3

6 years ago
Created attachment 707518 [details] [diff] [review]
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.
Attachment #706871 - Attachment is obsolete: true
Attachment #706871 - Flags: review?(past)
Attachment #707518 - Flags: review?(past)
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?
Attachment #707518 - Flags: review?(past) → review+
(Assignee)

Comment 5

6 years ago
(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.
(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.

ΟΚ.
(Assignee)

Comment 7

6 years ago
Created attachment 707792 [details] [diff] [review]
v1.2

Addressed comments.
Attachment #707792 - Flags: review+
(Assignee)

Comment 8

6 years ago
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.
Attachment #707792 - Flags: review+ → review?(past)
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'; }
Attachment #707792 - Flags: review?(past)
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
Created attachment 708181 [details] [diff] [review]
v1.3

One could now use { this._prop = value + 'a'; } (for example) to modify a setter.
Attachment #707792 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/af82229821f8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.