Last Comment Bug 683499 - GCLI needs an 'edit' command (for CSS only to start with)
: GCLI needs an 'edit' command (for CSS only to start with)
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 13
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
: 683502 (view as bug list)
Depends on: 585563 709223
Blocks: GCLICMD
  Show dependency treegraph
 
Reported: 2011-08-31 07:38 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-06-27 11:34 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A prototype edit command (695 bytes, text/plain)
2011-11-10 04:14 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details
A prototype edit command (1.59 KB, text/plain)
2011-11-10 04:20 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details
Working patch (2.83 KB, patch)
2012-01-27 06:54 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Patch with line specifier (3.26 KB, patch)
2012-01-30 05:38 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Rebased according to Joe's changes to bug 709223 (3.25 KB, patch)
2012-02-01 04:01 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
[in-fx-team] Line number is now line number instead of index (3.26 KB, patch)
2012-02-02 11:28 PST, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-08-31 07:38:14 PDT
This command allows the user to edit the web page, and see updates to their changes as the type. It does not attempt to save resources.
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-10 04:14:34 PST
Created attachment 573480 [details]
A prototype edit command
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-10 04:20:35 PST
Created attachment 573481 [details]
A prototype edit command
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-10 04:22:13 PST
For now we're only thinking about CSS. We'll add editing of HTML in another bug, but I expect that we'll share the basic 'edit' command.
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-10 06:31:21 PST
Moving conversation from email. Conversation so far:

Cedric:
> How do you envision the syntax of the edit command?

Joe:
> How about the resource type returns 'new-stylesheet' as one of it's options,
> and that the edit command uses that as it's defaultValue?
> That way the format of the command is very consistent
>    'edit <thing to edit>'.

Cedric:
> Makes sense.
> 
> Perhaps we could have a generic (additional) argument --new.
> If you do:
> 
> > edit --new foo.css
> It would create a new stylesheet that is already (pre) named with name foo.css ?
> 
> One could directly choose the full path where to save as well :
> 
> > edit --new ~/websites/project42/css/theme-v2.css
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-10 06:32:49 PST
If 'edit some-non-existant-file.css' automatically created the file, then we wouldn't need the --new would we?
Comment 6 Cedric Vivier [:cedricv] 2011-11-10 06:58:05 PST
(In reply to Joe Walker from comment #5)
> If 'edit some-non-existant-file.css' automatically created the file, then we
> wouldn't need the --new would we?

That's right. And if it references an existing file:// URL, it could automatically import the file to the page (before letting the user edit it).
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-11 07:37:51 PST
For what it's worth these are the strings to go with the example, which would live in /devtools/browser/locales/en-US/chrome/browser/devtools/gclicommands.properties

nodeParseSyntax=Syntax error in CSS query
nodeParseMultiple=Too many matches (%S)
nodeParseNone=No matches

editDesc=Tweak a page resource
editManual=Edit one of the resources that is part of this page (or maybe any generic web resource?)
editResourceDesc=URL to edit
editPretend=Pretend this is an popup editor containing
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 09:58:46 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 9 Rob Campbell [:rc] (:robcee) 2012-01-10 08:42:39 PST
marking as P1 as per joe.

filter on pegasus
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-25 03:38:50 PST
Cedric - Mike offered to take this on - I'm assuming that this is OK with you?

Mike, I've written a 'resource' type which allows you to easily pick from the resources in a page. It probably makes sense to use that. I'll update this demo along with it:
https://github.com/joewalker/gcli/blob/master/lib/demo/commands/experimental.js#L288
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-26 04:21:44 PST
Bug 709223 brings in a resource type to help filtering css.
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-27 02:48:19 PST
We also need to wait on bug 585563 as it holds the machinery needed to focus stylesheets in the style editor.
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-27 06:54:00 PST
Created attachment 592123 [details] [diff] [review]
Working patch

Done
Comment 14 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-27 07:26:09 PST
Comment on attachment 592123 [details] [diff] [review]
Working patch

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

Thanks Mike - I like it.

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +132,5 @@
> +         name: 'resource',
> +         include: 'text/css'
> +       },
> +       description: gcli.lookup("editResourceDesc")
> +     }

How easy would it be to add this?:

  {
    name: "line",
    type: {
      name: "number",
      min: 1,
      step: 10
    },
    description: "Line to Jump To"
  }

This is probably too much but we might even do:

  {
    name: "line",
    type: {
      name: "number",
      min: 1,
      max: function() {
        var text = args.resource._domSheet.ownerNode.innerHTML;
        return text.match(/\r/g).length;
      }
      step: 10
    }
  }

@@ +137,5 @@
> +   ],
> +   exec: function(args, context) {
> +     let hud = HUDService.getHudReferenceById(context.environment.hudId);
> +     let StyleEditor = hud.gcliterm.document.defaultView.StyleEditor;
> +     StyleEditor.openChrome(args.resource._domSheet);

I think I should create a less hacky way to access the sheet.
arg.resource.element?
This would make sense for script resources too.
What do you think?
Comment 15 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-27 08:05:05 PST
Yes, that would make perfect sense
Comment 16 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-30 05:38:23 PST
Created attachment 592670 [details] [diff] [review]
Patch with line specifier

(In reply to Joe Walker from comment #14)
> Comment on attachment 592123 [details] [diff] [review]
> Working patch
> 
> Review of attachment 592123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Mike - I like it.
> 
> ::: browser/devtools/webconsole/GcliCommands.jsm
> @@ +132,5 @@
> > +         name: 'resource',
> > +         include: 'text/css'
> > +       },
> > +       description: gcli.lookup("editResourceDesc")
> > +     }
> 
> How easy would it be to add this?:
> 
>   {
>     name: "line",
>     type: {
>       name: "number",
>       min: 1,
>       step: 10
>     },
>     description: "Line to Jump To"
>   }
> 

Done

> This is probably too much but we might even do:
> 
>   {
>     name: "line",
>     type: {
>       name: "number",
>       min: 1,
>       max: function() {
>         var text = args.resource._domSheet.ownerNode.innerHTML;
>         return text.match(/\r/g).length;
>       }
>       step: 10
>     }
>   }
> 

If only that would work with all stylesheets. Apart from args.resource not been available at this point, most stylesheets are not inline but are linked so we have no direct access to their text.

> @@ +137,5 @@
> > +   ],
> > +   exec: function(args, context) {
> > +     let hud = HUDService.getHudReferenceById(context.environment.hudId);
> > +     let StyleEditor = hud.gcliterm.document.defaultView.StyleEditor;
> > +     StyleEditor.openChrome(args.resource._domSheet);
> 
> I think I should create a less hacky way to access the sheet.
> arg.resource.element?
> This would make sense for script resources too.
> What do you think?

That sounds sensible.
Comment 17 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-01 04:01:27 PST
Created attachment 593378 [details] [diff] [review]
Rebased according to Joe's changes to bug 709223
Comment 18 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-01 05:20:12 PST
Comment on attachment 593378 [details] [diff] [review]
Rebased according to Joe's changes to bug 709223

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

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +154,5 @@
> +   ],
> +   exec: function(args, context) {
> +     let hud = HUDService.getHudReferenceById(context.environment.hudId);
> +     let StyleEditor = hud.gcliterm.document.defaultView.StyleEditor;
> +     StyleEditor.openChrome(args.resource.element, args.line ? args.line - 1 : 0);

This is a nit-pick, but worth mentioning:
Rather than say args.line ? args.line - 1 : 0 we could add a default value to the parameter:

> name: "line",
> defaultValue: 1,
> ...

This is better for 2 reasons: GCLI won't be so insistent about getting a value, and also you won't have to check if it's set, so you can do:

> StyleEditor.openChrome(args.resource.element, args.line - 1);

Maybe this is too late, but shouldn't line numbers be 1-based, not 0-based?
Comment 19 Panos Astithas [:past] 2012-02-01 05:36:15 PST
(In reply to Joe Walker from comment #18)
> Maybe this is too late, but shouldn't line numbers be 1-based, not 0-based?

I believe this is a result of the SourceEditor's 0-based numbering. 1-based makes more sense to me and the debugger API is 1-based as well.
Comment 20 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-02 11:28:25 PST
Created attachment 593919 [details] [diff] [review]
[in-fx-team] Line number is now line number instead of index

No longer index based and uses a default value of 1 by popular demand.
Comment 21 Dave Camp (:dcamp) 2012-02-02 14:37:19 PST
Comment on attachment 593919 [details] [diff] [review]
[in-fx-team] Line number is now line number instead of index

Bouncing request to Joe
Comment 22 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-02 14:44:52 PST
Happy to put this in my landing queue just behind bug 709223.
Comment 23 Mihai Sucan [:msucan] 2012-02-17 08:53:08 PST
Comment on attachment 593919 [details] [diff] [review]
[in-fx-team] Line number is now line number instead of index

Landed:
https://hg.mozilla.org/integration/fx-team/rev/a6e809d5a446
Comment 24 Tim Taubert [:ttaubert] 2012-02-17 17:09:47 PST
https://hg.mozilla.org/mozilla-central/rev/a6e809d5a446
Comment 25 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-04-29 11:46:53 PDT
*** Bug 683502 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.