1032433 - Add "Add-Property" Option to VariablesView

Status: RESOLVED WONTFIX

(DevTools :: Object Inspector, enhancement)

Description

[Connor Brem [:cbrem] [@cbrem]]

The VariablesView widget can optionally have a "new" method, which allows a user to add a property to the object under inspection. However, the Webconsole currently provides no "new" method.
As the Webconsole already allows users to delete and rename properties on objects under inspection, it seems logical to also allow them to add properties.

Comment 1

[Connor Brem [:cbrem] [@cbrem]]

Attachment: bug1032433_v0.patch

Here's a prototype for this feature. The button image is not production-quality, and will be replaced eventually.

Comment 2

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

Comment on attachment 8452022 [details] [diff] [review]
bug1032433_v0.patch

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

This is a fantastic start!

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +892,5 @@
>          }
>          return;
>  
>        case e.DOM_VK_INSERT:
> +        //TODO

What needs to be done here? I'm curious.

@@ +2150,5 @@
>   *        The variable's name.
>   * @param object aDescriptor
>   *        The variable's descriptor.
> + * @param boolean isRoot
> + *        Is the Variable at the root of the VariablesView?

We don't actually need this, since not all variable instances without titles are roots, and vice-versa. You should just verify !this._nameString, or better yet, this.focusable.

@@ +2168,5 @@
>      delete aDescriptor.set;
>    }
>  
> +  this._isRoot = isRoot;
> +  this._parent = aScope;

We already have the parent. See how we do Scope.call(...) below? That sets the parent as `this.ownerView`.

@@ +2585,5 @@
>      }
>  
> +    // TODO:
> +    //   1. maybe should not allow user to add another property while they're already
> +    //      editing one (maybe while this.editing?)

I agree this should be the case. I don't see any way in which that would actually be useful.

@@ -2566,2 @@
>      if (ownerView.new) {
> -      let addPropertyNode = this._addPropertyNode = this.document.createElement("toolbarbutton");

Why are you not setting this._addPropertyNode anymore?

@@ +2595,5 @@
>  
> +      if (this._isRoot) {
> +        // If this is the top-most variable in this scope, its title will not
> +        // be displayed. Therefore, append its add-property button to the
> +        // containing scope's title.

Why? Wouldn't the containing scope title have a `new` function attached, so it will get the (+) button anyway? Take a look at how the app-manager's manifest editor handles the addition of new properties to objects. Talk to :jryans if you have questions about that.

Comment 3

[Connor Brem [:cbrem] [@cbrem]]

Comment on attachment 8452022 [details] [diff] [review]
bug1032433_v0.patch

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +892,5 @@
>          }
>          return;
>  
>        case e.DOM_VK_INSERT:
> +        //TODO

Nothing needs to be done there :) I just forgot to remove the TODO.

@@ +2150,5 @@
>   *        The variable's name.
>   * @param object aDescriptor
>   *        The variable's descriptor.
> + * @param boolean isRoot
> + *        Is the Variable at the root of the VariablesView?

I was running into an issue when using this._nameString where newly-created properties (before the user enters a key or value) have no name, and therefore cannot be distinguished from the root.
I'll try using this.focusable, though.

@@ +2168,5 @@
>      delete aDescriptor.set;
>    }
>  
> +  this._isRoot = isRoot;
> +  this._parent = aScope;

Thanks! I'll use that instead.

@@ +2585,5 @@
>      }
>  
> +    // TODO:
> +    //   1. maybe should not allow user to add another property while they're already
> +    //      editing one (maybe while this.editing?)

I'll look into this in the next version of the patch I put up.

@@ +2595,5 @@
>  
> +      if (this._isRoot) {
> +        // If this is the top-most variable in this scope, its title will not
> +        // be displayed. Therefore, append its add-property button to the
> +        // containing scope's title.

At least as I understand it, the containing Scope is a special case -- because it must contain a button which adds a Property to the first Variable that it contains, whereas all other Variables will contain buttons that add Properties to themselves (i.e. the Variables).
The way that I solved this special case was by having the first Variable inside the containing Scope "reach up" and put a + button on the Scope. Whereas all other Variables put + buttons on themselves.

Also, this code won't run for the containing Scope, since it's in Variable.prototype.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Victor Porof [:vporof][:vp] from comment #2)
> @@ +2595,5 @@
> >  
> > +      if (this._isRoot) {
> > +        // If this is the top-most variable in this scope, its title will not
> > +        // be displayed. Therefore, append its add-property button to the
> > +        // containing scope's title.
> 
> Why? Wouldn't the containing scope title have a `new` function attached, so
> it will get the (+) button anyway? Take a look at how the app-manager's
> manifest editor handles the addition of new properties to objects. Talk to
> :jryans if you have questions about that.

App Manager's manifest editor actually fails to handle this today: there's no way to add new top-level properties currently (only new ones further down the tree), which is kind of sad.  It's also being removed relatively soon, so fixing the manifest editor isn't worth it anymore.

But anyway, some solution is needed generally to enable the top-level container to add properties to do this correctly.  Maybe Connor's idea is a good way to go here.

Comment 5

[Brian Grinstead [:bgrins]]

In the context of Bug 1243802 (expandable objects inline in the console output), we aren't likely to need this for the web console.  The Variables View widget is still going to be used in the Debugger and other places, so maybe it's still worth taking it on for those places

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

won't be fixed in the VariablesView