GCLI needs an 'edit' command (for CSS only to start with)

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Console
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwalker, Assigned: miker)

Tracking

unspecified
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
Blocks: 693259
Blocks: 689605
No longer blocks: 671406, 693259
OS: Windows 7 → All
Hardware: x86_64 → All
Created attachment 573480 [details]
A prototype edit command
Created attachment 573481 [details]
A prototype edit command
Attachment #573480 - Attachment is obsolete: true
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.
Summary: GCLI needs an 'edit' command → GCLI needs an 'edit' command (for CSS only to start with)
Assignee: nobody → cedricv
Status: NEW → ASSIGNED
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
If 'edit some-non-existant-file.css' automatically created the file, then we wouldn't need the --new would we?
(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).
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
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Blocks: 711051
No longer blocks: 689605
marking as P1 as per joe.

filter on pegasus
Priority: -- → P1
No longer blocks: 711051
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
Assignee: cedricv → mratcliffe
Depends on: 709223
Target Milestone: --- → Firefox 12
Bug 709223 brings in a resource type to help filtering css.
We also need to wait on bug 585563 as it holds the machinery needed to focus stylesheets in the style editor.
Depends on: 585563
Created attachment 592123 [details] [diff] [review]
Working patch

Done
Attachment #573481 - Attachment is obsolete: true
Attachment #592123 - Flags: review?(dcamp)
Whiteboard: [has-patch]
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?
Yes, that would make perfect sense
Target Milestone: Firefox 12 → Firefox 13
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.
Attachment #592123 - Attachment is obsolete: true
Attachment #592123 - Flags: review?(dcamp)
Attachment #592670 - Flags: review?(dcamp)
Created attachment 593378 [details] [diff] [review]
Rebased according to Joe's changes to bug 709223
Attachment #592670 - Attachment is obsolete: true
Attachment #592670 - Flags: review?(dcamp)
Attachment #593378 - Flags: review?(dcamp)
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?
(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.
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.
Attachment #593378 - Attachment is obsolete: true
Attachment #593378 - Flags: review?(dcamp)
Attachment #593919 - Flags: review?(dcamp)

Comment 21

6 years ago
Comment on attachment 593919 [details] [diff] [review]
[in-fx-team] Line number is now line number instead of index

Bouncing request to Joe
Attachment #593919 - Flags: review?(dcamp) → review?(jwalker)
Attachment #593919 - Flags: review?(jwalker) → review+
Happy to put this in my landing queue just behind bug 709223.
Whiteboard: [has-patch] → [land-in-fx-team]
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
Attachment #593919 - Attachment description: Line number is now line number instead of index → [in-fx-team] Line number is now line number instead of index

Updated

6 years ago
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a6e809d5a446
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Duplicate of this bug: 683502
Blocks: 768998
You need to log in before you can comment on or make changes to this bug.