GCLI needs a 'tilt' command

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Console
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwalker, Assigned: vporof)

Tracking

({dev-doc-complete})

unspecified
Firefox 13
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tilt])

Attachments

(1 attachment, 3 obsolete attachments)

I'm not sure what parameters we might need.
(Assignee)

Comment 1

5 years ago
We could have a param for the target node. If there's no target node specified, we're basically also fixing (or providing an alternative to 718058).
Whiteboard: [tilt]
Worth reading:
  https://github.com/mozilla/gcli/blob/master/docs/writing-commands.md

I think you're going to need something like this:

/**
 * 'tilt' command
 */
gcli.addCommand({
  name: "tilt",
  description: gcli.lookup("tiltDesc"),
  manual: gcli.lookup("tiltManual"),
  params: [
    {
      name: "node",
      type: "node",
      defaultValue: null,
      description: gcli.lookup("tiltNodeDesc"),
      manual: gcli.lookup("tiltNodeManual")
    }
  ],
  exec: function Command_inspect(args, context) {
    if (args.node == null) {
      // Open tilt without a highlighted node
    }
    else {
      // Open tilt with a highlighted node
      // args.node will be a DOM element
    }
  }
});

It's possible that we might want a set of commands though ...

> tilt open div#id
> tilt close
> tilt rotate 90
> tilt zoom 2

But maybe that's going too far. I guess it depends on what tilt can do.
(Assignee)

Comment 3

5 years ago
(In reply to Joe Walker from comment #2)
> 
> But maybe that's going too far. I guess it depends on what tilt can do.

Tilt can basically do everything the inspector does + the 3D extravaganza. I think having commands for rotate/translate/zoom/transform is an excellent idea! We could also add support for reset, focus, delete etc. Basically, the way I see it, everything you can do with Tilt using the mouse or keyboard could be done using a command. The implementation should be extremely straight forward, as Tilt provides an API for all of this.

Is writing these commands in GcliCommands.jsm a good idea, or can/should these properties be added somewhere else? I would prefer the former.
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(In reply to Victor Porof from comment #3)
> (In reply to Joe Walker from comment #2)
> > 
> > But maybe that's going too far. I guess it depends on what tilt can do.
> 
> Tilt can basically do everything the inspector does + the 3D extravaganza. I
> think having commands for rotate/translate/zoom/transform is an excellent
> idea! We could also add support for reset, focus, delete etc. Basically, the
> way I see it, everything you can do with Tilt using the mouse or keyboard
> could be done using a command. The implementation should be extremely
> straight forward, as Tilt provides an API for all of this.
> 
> Is writing these commands in GcliCommands.jsm a good idea, or can/should
> these properties be added somewhere else? I would prefer the former.

The down side of putting all the files in one place is that you need frequent rebasing as other people add to GcliCommands.jsm.

Generally I add simple commands to GcliCommands.jsm (which is everything committed so far) but 'pref' which is 3 commands and a new type, went into its own file. See here for work in progress:

https://bugzilla.mozilla.org/attachment.cgi?id=595861
(Assignee)

Comment 5

5 years ago
Created attachment 597356 [details] [diff] [review]
v1

I'll need to close the console when starting Tilt. The reason for it is: when resizing from a smaller to larger dimension, the canvas looses detail and Tilt doesn't assign new resolutions for this because it would cause flickering; also, I bet most users will close the webconsole after using the Tilt command, and since Tilt is immune to reflows and repaints, they'll be left with a smaller inaccurate version of what they actually want to visualize.

Is this something that's ok to do?
Attachment #597356 - Flags: feedback?(jwalker)
(Assignee)

Comment 6

5 years ago
(In reply to Victor Porof from comment #5)
> Created attachment 597356 [details] [diff] [review]
> v1
> 
> I'll need to close the console when starting Tilt.

However, this isn't necessary when the webconsole is in it's own window. Is there an easy way to determine that?
(Assignee)

Updated

5 years ago
Depends on: 726634
(Assignee)

Comment 7

5 years ago
Created attachment 597410 [details] [diff] [review]
v2

Added the remaining commands.
Attachment #597410 - Flags: review?(jwalker)
Comment on attachment 597410 [details] [diff] [review]
v2

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

::: browser/devtools/webconsole/GcliTiltCommands.jsm
@@ +18,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Joe Walker <jwalker@mozilla.com> (original author)

I think you need to go here

@@ +35,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +
> +let EXPORTED_SYMBOLS = [ "GcliCommands" ];

I don't think you're actually exporting anything here, so probably [ ] is better.

@@ +127,5 @@
> +  manual: gcli.lookup("tiltRotateManual"),
> +  params: [
> +    {
> +      name: "x",
> +      type: { name: 'number', min: -360, max: 360 },

It might be worth adding step:10, so up/down arrow move to the right place faster

@@ +168,5 @@
> +  manual: gcli.lookup("tiltZoomManual"),
> +  params: [
> +    {
> +      name: "zoom",
> +      type: { name: 'number', min: -Number.MAX_VALUE },

Hmm - I'm not sure if this value for min is helpful (unless I missed something), and could make things downright confusing if we ever use min as a default or a slider boundary or something
(Assignee)

Comment 9

5 years ago
(In reply to Joe Walker from comment #8)
> Comment on attachment 597410 [details] [diff] [review]
> v2
> 
> Review of attachment 597410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/GcliTiltCommands.jsm
> @@ +18,5 @@
> > + * Portions created by the Initial Developer are Copyright (C) 2011
> > + * the Initial Developer. All Rights Reserved.
> > + *
> > + * Contributor(s):
> > + *   Joe Walker <jwalker@mozilla.com> (original author)
> 
> I think you need to go here
> 

Hello copy paste :) Fixed.

> @@ +35,5 @@
> > + *
> > + * ***** END LICENSE BLOCK ***** */
> > +
> > +
> > +let EXPORTED_SYMBOLS = [ "GcliCommands" ];
> 
> I don't think you're actually exporting anything here, so probably [ ] is
> better.
> 

Agreed. But then, why is there a EXPORTED_SYMBOLS = [ "GcliCommands" ]; in GcliCommands.jsm?

> @@ +127,5 @@
> > +  manual: gcli.lookup("tiltRotateManual"),
> > +  params: [
> > +    {
> > +      name: "x",
> > +      type: { name: 'number', min: -360, max: 360 },
> 
> It might be worth adding step:10, so up/down arrow move to the right place
> faster
> 

Agreed.

> @@ +168,5 @@
> > +  manual: gcli.lookup("tiltZoomManual"),
> > +  params: [
> > +    {
> > +      name: "zoom",
> > +      type: { name: 'number', min: -Number.MAX_VALUE },
> 
> Hmm - I'm not sure if this value for min is helpful (unless I missed
> something), and could make things downright confusing if we ever use min as
> a default or a slider boundary or something

In that case, I can't input a negative value for zoom, which is needed for this command to work properly. Is there a way to assure that 'number' accepts negative values?
(Assignee)

Comment 10

5 years ago
Also, is there an easy answer to comment #6?
(In reply to Victor Porof from comment #6)
> (In reply to Victor Porof from comment #5)
> > I'll need to close the console when starting Tilt.
> 
> However, this isn't necessary when the webconsole is in it's own window. Is
> there an easy way to determine that?

I don't know, off the top of my head. IIRC there is a way, but I'd need to look through the code. Mihai actually wrote it, if you have trouble finding it.
(In reply to Victor Porof from comment #9)
> (In reply to Joe Walker from comment #8)
> > Comment on attachment 597410 [details] [diff] [review]
...
> > @@ +168,5 @@
> > > +  manual: gcli.lookup("tiltZoomManual"),
> > > +  params: [
> > > +    {
> > > +      name: "zoom",
> > > +      type: { name: 'number', min: -Number.MAX_VALUE },
> > 
> > Hmm - I'm not sure if this value for min is helpful (unless I missed
> > something), and could make things downright confusing if we ever use min as
> > a default or a slider boundary or something
> 
> In that case, I can't input a negative value for zoom, which is needed for
> this command to work properly. Is there a way to assure that 'number'
> accepts negative values?

We need to fix bug 727535 that I just raised.
I suggest taking min: -Number.MAX_VALUE out and pointing anyone that complains to this bug. It should be quick to fix.
(Assignee)

Comment 13

5 years ago
Created attachment 597516 [details] [diff] [review]
v3

Fixed stuff. Also talked with msucan and made the necessary changes for comment #6.
Attachment #597356 - Attachment is obsolete: true
Attachment #597410 - Attachment is obsolete: true
Attachment #597516 - Flags: review?(jwalker)
Attachment #597356 - Flags: feedback?(jwalker)
Attachment #597410 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Blocks: 727529
(Assignee)

Comment 14

5 years ago
I seem to get errors when testing:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js |
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 1106
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 367
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 286
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 287
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 149
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 150
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 66
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: onLoad :: line 2204

Which is on the line
test.ok(assignC.getPredictions().length < 20); // could break ...

So at least it was predictable :) Joe?
(Assignee)

Comment 15

5 years ago
Created attachment 599652 [details] [diff] [review]
v3.1

Removed the troublesome getPredictions().length test line. Tests pass.
Attachment #597516 - Attachment is obsolete: true
Attachment #597516 - Flags: review?(jwalker)
Attachment #599652 - Flags: review?(jwalker)
Attachment #599652 - Flags: review?(jwalker) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [tilt] → [tilt][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/83a2116800d5
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/83a2116800d5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [tilt][fixed-in-fx-team] → [tilt]
Target Milestone: --- → Firefox 13
I'd say that "ammount" is a typo (used several times). No need to change entity names to fix it.
(Assignee)

Comment 19

5 years ago
(In reply to Francesco Lodolo [:flod] from comment #18)
> I'd say that "ammount" is a typo (used several times). No need to change
> entity names to fix it.

Yes, typo. Follow-up bug?
(Assignee)

Comment 20

5 years ago
I think it would be nice if the MDN doc would list these, since they're pretty fun to play with.
Keywords: dev-doc-needed
(In reply to Victor Porof from comment #19)
> (In reply to Francesco Lodolo [:flod] from comment #18)
> > I'd say that "ammount" is a typo (used several times). No need to change
> > entity names to fix it.
> 
> Yes, typo. Follow-up bug?

Bug 731523 (not sure how dependencies should be set)
(In reply to Francesco Lodolo [:flod] from comment #21)
> Bug 731523 (not sure how dependencies should be set)

I think you did the right thing in leaving out the dependency btw. While that bug does depend on this bug in that you can't apply that patch without this patch, we don't generally list all such dependencies (which would clearly be a very very long list) - so we can assume dependence on landed patches.
The existence of this command is documented here:

https://developer.mozilla.org/en/Tools/GCLI

I will leave it to "help tilt" to be more specific.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 768998
You need to log in before you can comment on or make changes to this bug.