Closed Bug 788890 Opened 12 years ago Closed 12 years ago

GCLI screenshot command with no filename should copy to clipboard

Categories

(DevTools :: Console, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: rileylabrecque, Assigned: Optimizer)

References

Details

Attachments

(1 file, 10 obsolete files)

Copying to the clipboard for one click upload with other applications, or pasting into your image editor of choice to annotate would be super useful.

I'm not exactly sure how this would be achieved since the screenshot command has multiple arguments.
QA Contact: scrapmachines
Created the gist containing the implementation
https://gist.github.com/3655030

@Joe, please provide feedback on the approach. Providing a space as a default value can also provide us a way to skip the file name and still choose node/delay etc.
Also suggest the various kinds of test that would be required to test this addition.
Assignee: nobody → scrapmachines
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
QA Contact: scrapmachines
The gist file as in the previous comment's url
Attachment #658845 - Flags: feedback?(jwalker)
Attached patch Updated with optional options (obsolete) — Splinter Review
Updated the gist too.
Now you can simply do screenshot --[any option] for copying to clipboard
Attachment #658845 - Attachment is obsolete: true
Attachment #658845 - Flags: feedback?(jwalker)
Attachment #658847 - Flags: feedback?(jwalker)
(In reply to Girish Sharma [:Optimizer] from comment #2)
> Created attachment 658845 [details] [diff] [review]
> Screenshot with copy to clipboard feature
> 
> The gist file as in the previous comment's url

Looks good to me. 2 things:
- The 'Options' string needs localization. It will be displayed in 'help' and in generated dialogs shortly
- I think we should run this past victorporof before committing it since he wrote it in the first place. Maybe r? him.

Thanks.
Attachment #658847 - Flags: feedback?(jwalker) → feedback+
Also, as you noted in irc, we'll need to update the tests.
Attached patch Patch with tests (obsolete) — Splinter Review
Added tests for input check and screenshot capturing test.
Attachment #658847 - Attachment is obsolete: true
Attachment #659006 - Flags: review?(vporof)
Attachment #659006 - Flags: review?(jwalker)
Comment on attachment 659006 [details] [diff] [review]
Patch with tests

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

Changes in the command look pretty good to me. Test also looks fine, but I'll leave jwalker dive into the nity gritty details.
Couldn't apply this to fx-team tip, this needs a small rebase.

::: browser/devtools/commandline/CmdScreenshot.jsm
@@ +99,4 @@
>  
>      let ctx = canvas.getContext("2d");
>      ctx.drawWindow(window, left, top, width, height, "#fff");
> +    let data = canvas.toDataURL("image/png", "");

If we leave this here then a toDataUrl will be called on each screenshot, regardless if the user wants it to get in the clipboard or not. Move this after the 'if (filename == " ")' check below, above the channel instantiation.

@@ +103,5 @@
>  
> +    try {
> +      if (filename == " ") {
> +        let io = Cc["@mozilla.org/network/io-service;1"]
> +                   .getService(Ci.nsIIOService);

Spacing nit:
let io = Cc["@mozilla.org/network/io-service;1"]
  .getService(Ci.nsIIOService);

Same goes throughout the patch.

@@ +119,5 @@
> +
> +        let trans = Cc["@mozilla.org/widget/transferable;1"]
> +                      .createInstance(Ci.nsITransferable);
> +        trans.addDataFlavor(channel.contentType);
> +        trans.setTransferData(channel.contentType, wrapped, -1);

I think length should be 0 (kFlavorHasDataProvider) for non-string data.

See chrome://global/content/nsTransferable.js (I think it may be a good idea to reuse the stuff in there actually, if we can).

@@ +128,5 @@
> +        return "Copied to clipboard.";
> +      }
> +    }
> +    catch (ex) {
> +      return "Error occured while copying.";

ocurred -> occurred
Also, I think the message here should say "Error occurred while copying to clipboard."

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +61,5 @@
>  screenshotFilenameManual=The name of the file (should have a '.png' extension) to which we write the screenshot.
>  
> +# LOCALIZATION NOTE (screenshotGroupOptions) A label for the optional options of
> +# the screenshot command.
> +screenshotGroupOptions=Options

"Available Options" if we're going to be consistent with other commands.
Attachment #659006 - Flags: review?(vporof) → feedback+
(In reply to Victor Porof [:vp] from comment #7)
> Comment on attachment 659006 [details] [diff] [review]
> Patch with tests
> 
> Review of attachment 659006 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes in the command look pretty good to me. Test also looks fine, but
> I'll leave jwalker dive into the nity gritty details.
> Couldn't apply this to fx-team tip, this needs a small rebase.
> 
> ::: browser/devtools/commandline/CmdScreenshot.jsm
> @@ +99,4 @@
> >  
> >      let ctx = canvas.getContext("2d");
> >      ctx.drawWindow(window, left, top, width, height, "#fff");
> > +    let data = canvas.toDataURL("image/png", "");
> 
> If we leave this here then a toDataUrl will be called on each screenshot,
> regardless if the user wants it to get in the clipboard or not. Move this
> after the 'if (filename == " ")' check below, above the channel
> instantiation.

We need the data for both the options, clipboard and file so it will have to remain there only.

> @@ +103,5 @@
> >  
> > +    try {
> > +      if (filename == " ") {
> > +        let io = Cc["@mozilla.org/network/io-service;1"]
> > +                   .getService(Ci.nsIIOService);
> 
> Spacing nit:
> let io = Cc["@mozilla.org/network/io-service;1"]
>   .getService(Ci.nsIIOService);
> 
> Same goes throughout the patch.

Noted.

> @@ +119,5 @@
> > +
> > +        let trans = Cc["@mozilla.org/widget/transferable;1"]
> > +                      .createInstance(Ci.nsITransferable);
> > +        trans.addDataFlavor(channel.contentType);
> > +        trans.setTransferData(channel.contentType, wrapped, -1);
> 
> I think length should be 0 (kFlavorHasDataProvider) for non-string data.
> 
> See chrome://global/content/nsTransferable.js (I think it may be a good idea
> to reuse the stuff in there actually, if we can).

Actually this method of copying an image onto clipboard was explained in some bug's comment. I could not find any other reference anywhere else so had no exact idea about whats going on :P.

> @@ +128,5 @@
> > +        return "Copied to clipboard.";
> > +      }
> > +    }
> > +    catch (ex) {
> > +      return "Error occured while copying.";
> 
> ocurred -> occurred
> Also, I think the message here should say "Error occurred while copying to
> clipboard."
> 

Noted.

> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +61,5 @@
> >  screenshotFilenameManual=The name of the file (should have a '.png' extension) to which we write the screenshot.
> >  
> > +# LOCALIZATION NOTE (screenshotGroupOptions) A label for the optional options of
> > +# the screenshot command.
> > +screenshotGroupOptions=Options
> 
> "Available Options" if we're going to be consistent with other commands.

"Options" was suggested by Joe, so I went with it. I will wait for Joe's response on this.
Attached patch Patch v2.0 with tests (obsolete) — Splinter Review
Addressed spacing and typo nits.
Attachment #659006 - Attachment is obsolete: true
Attachment #659006 - Flags: review?(jwalker)
Attachment #659472 - Flags: review?(vporof)
Attachment #659472 - Flags: review?(jwalker)
Comment on attachment 659006 [details] [diff] [review]
Patch with tests

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

::: browser/devtools/commandline/CmdScreenshot.jsm
@@ +24,4 @@
>      {
>        name: "filename",
>        type: "string",
> +      defaultValue: " ",

" " is a magic value, please could you give it a constant:

    const MAGIC_CLIPBOARD_FILENAME = " ";

and then

    defaultValue: MAGIC_CLIPBOARD_FILENAME,

and clearly use MAGIC_CLIPBOARD_FILENAME in the if statement below.

@@ +103,5 @@
>  
> +    try {
> +      if (filename == " ") {
> +        let io = Cc["@mozilla.org/network/io-service;1"]
> +                   .getService(Ci.nsIIOService);

I'm personally not keen on using the same number of spaces for continuation indents as normal indents, because it can make if statements confusing:

if (long.test.statement == some.other.long.value && more.stuff == so.it.wraps &&
  more.executeCheck()) {
  do.work();
}

Maybe if it's routine in tilt code to do this then we should discuss it, but please could you hold off from removing the indents for now Optimizer?

@@ +124,5 @@
> +
> +        let clipid = Ci.nsIClipboard;
> +        let clip = Cc["@mozilla.org/widget/clipboard;1"].getService(clipid);
> +        clip.setData(trans, null, clipid.kGlobalClipboard);
> +        return "Copied to clipboard.";

This string needs to be in the properties file

@@ +128,5 @@
> +        return "Copied to clipboard.";
> +      }
> +    }
> +    catch (ex) {
> +      return "Error occured while copying.";

And also the string should be in the properties file.

@@ +168,2 @@
>  
>      return "Saved to " + filename;

And this one!

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +61,5 @@
>  screenshotFilenameManual=The name of the file (should have a '.png' extension) to which we write the screenshot.
>  
> +# LOCALIZATION NOTE (screenshotGroupOptions) A label for the optional options of
> +# the screenshot command.
> +screenshotGroupOptions=Options

I think this is the first use of a title for a parameter group, so I don't think there is prior art here, unless I missed something???.
And I'd like to keep this string short if possible.
Attachment #659006 - Attachment is obsolete: false
Added all the strings to properties file, and changes the " " to a const
Attachment #659006 - Attachment is obsolete: true
Attachment #659476 - Flags: review?(jwalker)
Attachment #659476 - Flags: review?(jwalker) → review+
Attachment #659472 - Attachment is obsolete: true
Attachment #659472 - Flags: review?(vporof)
Attachment #659472 - Flags: review?(jwalker)
I'm not actually convinced this feature is wanted.

As someone who use this command a lot, I would much prefer if calling "screenshot" with no argument would just create a file in the Download directory called "screenshot-800x600.png".

This is what Mac and Linux do (I don't know about Windows).
Paul: I see, In Windows hitting Print Screen goes directly to the Clipboard.

So it's not uncommon for power users to use applications like the following to Upload directly from the Clipboard (I.E. directly after you press Print Screen.)
Puush, http://puush.me/
ShareX, http://code.google.com/p/sharex/
or Snagit. http://www.techsmith.com/snagit.html

I guess it depends on the typical usage case of the feature? I listed my two most popular usage cases in my original report.

My most common usage case will undoubtedly be sending full page captures to page creators to highlight issues with their design in Gecko. Uploading them through such applications listed above.

My second most popular usage case would be grabbing a screenshot and going directly into photoshop. Pasting it in, and then annotating it for a designer.

If do I want a screenshot in my Downloads Directory, appending " ss" Isn't hard. But having the command go directly to a file on the disk would effectively kill this feature.
This could be a mac/windows thing. On a mac cmd+shift+4 does a screenshot to the downloads directory, one windows print-screen uses the clipboard.

Personally I find the mac behaviour to be more obvious because you can see the file.

However the windows behaviour I find is to be more useful because it's faster to get the screeny into any app that uses the clipboard.

There are other arguments, but none that I can see to be an obvious decider.

I considered having the command do 2 different things depending on the environment, but I think that would probably be confusing.

>> screenshot --clipboard
>> screenshot --c<TAB>

Is verbose but with tab completion, it's 4 extra chars

Or as soon as we have short params it's only a " -c" extra.

>> screenshot ss

Would rely on a non-obvious auto-extension feature, so I think most people would do

>> screenshot s.png

I don't have a sure answer, but I'm kind of leaning towards having a parameter to use the clipboard. (i.e. "screenshot --clipboard")
I second Riley.

This feature is mainly targeted for developers (the whole gcli feature itself). The most common use case of this command would be to debug styles, glitches, or highlight some specific part of the site. For that purpose, the user might want to edit the screenshot first before displaying it to someone or saving it.

Having the ability to copy the screenshot will reduce 2 steps of 1. locating the file, 2. opening it in an editor. The first step sometimes really becomes messy, and on top of it, if we were to make the default behavior to save the screenshot using an auto generated name, the step of locating the file could become horrible. I might have to open up more than one file to get to the one I want.

Anyhow, even with this bug fixed, there will always a way to save the screenshot to file, just by using some extra characters.
(In reply to Girish Sharma [:Optimizer] from comment #15)
> I second Riley.
> 
> This feature is mainly targeted for developers (the whole gcli feature
> itself). The most common use case of this command would be to debug styles,
> glitches, or highlight some specific part of the site. For that purpose, the
> user might want to edit the screenshot first before displaying it to someone
> or saving it.
> 
> Having the ability to copy the screenshot will reduce 2 steps of 1. locating
> the file, 2. opening it in an editor. The first step sometimes really
> becomes messy, and on top of it, if we were to make the default behavior to
> save the screenshot using an auto generated name, the step of locating the
> file could become horrible. I might have to open up more than one file to
> get to the one I want.
> 
> Anyhow, even with this bug fixed, there will always a way to save the
> screenshot to file, just by using some extra characters.

I don't think anyone is arguing that we shouldn't allow save to clipboard, just what should happen with plain unadorned 'clipboard'.

The point of the debate in comment 14 is 'how inconvenienced is the group that lost out', and there's not a lot in it.

I'm also slightly swayed by the slightly simpler explanation of the clipboard-param option

Clipboard-Param Option:
> "The screenshot command saves a screenshot, by default to a file in the download directory or to the clipboard with the --clipboard argument"

No-Param Option:
> "The screenshot command saves a screenshot to the clipboard. If a filename is given, the the screenshot is saved to that file rather than the clipboard"

Victor - looks like you might have the casting vote?
(In reply to Joe Walker [:jwalker] from comment #16)
> (In reply to Girish Sharma [:Optimizer] from comment #15)

This command is already a bit weird, which may partly be my fault. For example, consider the example Kevin gave in his gcli presentation video:

>> screenshot heading.png 0 false h1

What is '0'? What flag does 'false' switch on/off?
This is somewhat alleviated by the fact that gcli displays hints/param names while typing a command, but I think that adding another param here for clipboard would be amazingly weird, because you may end up with:

>> screenshot this_is_ignored 0 false h1 true

That aside, I think the actual issue in this case is the clash between two param types, string (filename) vs. boolean (clipboard or not). I'm not sure if gcli supports different types for the same param, but even if it did (which wouldn't be such a good idea imo), certainly, the options:

>> screenshot " " 0 false h1 true
vs.
>> screenshot true 0 false h1 true
vs.
>> screenshot "somefile.png" 0 false h1 true

..are all confusing, because one has no idea what to type in there. Regardless on how descriptive the docs may be, choosing this format would make users scratch their head, and also create an ugly precedent.

Since we're not in a hurry here, I think it may be a good idea to rethink this command a bit. How about having multiple flavors:

>> screenshot
- automatically saves the currently visible region of the page to "title-WxH.png", in the Downloads (or Desktop even?) folder

>> screenshot fullpage [delay]
- same as before, but with the entire document, and one can add an optional delay

>> screenshot node <selector> [delay]
- d'oh

>> screenshot clipboard
- automatically copies the entire document (like 'screenshot fullpage') to the clipboard, no need for any other params here imo

Since copying a node to clipboard has very specific usage scenarios (if I read the previous comments correctly, the most likely usage is "copy all this webpage so I can paste it in my favorite pixel blender").

If we're not in a hurry here, I think these are trivial changes that would make this command more intuitive.
(In reply to Victor Porof [:vp] from comment #17)

We could even add an optional file argument in all the examples above, but I'm not entirely sure how useful this feature actually is:

>> screenshot [file]
>> screenshot fullpage [file] [delay]
>> screenshot node <selector> [file] [delay]
>> screenshot clipboard
(In reply to Victor Porof [:vp] from comment #17)
> (In reply to Joe Walker [:jwalker] from comment #16)
> > (In reply to Girish Sharma [:Optimizer] from comment #15)
> 
> This command is already a bit weird, which may partly be my fault. For
> example, consider the example Kevin gave in his gcli presentation video:
> 
> >> screenshot heading.png 0 false h1
> 
> What is '0'? What flag does 'false' switch on/off?
> This is somewhat alleviated by the fact that gcli displays hints/param names
> while typing a command, but I think that adding another param here for
> clipboard would be amazingly weird, because you may end up with:

Optimizer has already solved all these problems by using param groups.

The above is now:

>> screenshot heading.png --delay 0 --fullpage --selector h1

Or, because most of those are defaults:

>> screenshot heading.png --selector h1
(In reply to Joe Walker [:jwalker] from comment #19)

In that case, it seems that --clipboard is likely the best option.
Okay, I am not at all in any side for having an option for clipboard or not. We can have an option, but then the discussion would be : What should be the default behavior ?

I really want to use just "screenshot" and have some meaningful output. There should be something of a default behavior which happens when the command is used without params. Right now, it copies to clipboard which I personally prefer.
(In reply to Girish Sharma [:Optimizer] from comment #21)
> Okay, I am not at all in any side for having an option for clipboard or not.
> We can have an option, but then the discussion would be : What should be the
> default behavior ?
> 
> I really want to use just "screenshot" and have some meaningful output.
> There should be something of a default behavior which happens when the
> command is used without params. Right now, it copies to clipboard which I
> personally prefer.

Would you be happy with "screenshot" with no params saving to a default filename? On a mac the default filename is "Screen Shot yyyy-mm-dd at hh.mm.ss.png".
makes default behavior of saving the file with "Screen Shot yyyy-mm-dd at HH.MM.SS.png" as name. Adds clipboard option, makes the output clickable to locate the file.

Tested on Windows, all tests pass.
Attachment #659476 - Attachment is obsolete: true
Attachment #659762 - Flags: review?(jwalker)
yyyy-mm-dd might be confusing for some locales. I'd prefer "Screenshot (%S).png", where %S is just 'new Date().toLocaleString()'.
(In reply to Dão Gottwald [:dao] from comment #24)
> yyyy-mm-dd might be confusing for some locales. I'd prefer "Screenshot
> (%S).png", where %S is just 'new Date().toLocaleString()'.

1. Joe suggested this as mac also uses this default name .
2. ':' is not allowed in file names on Windows.
3. There will be sorting problems among the screenshots.
(In reply to Girish Sharma [:Optimizer] from comment #25)
> (In reply to Dão Gottwald [:dao] from comment #24)
> > yyyy-mm-dd might be confusing for some locales. I'd prefer "Screenshot
> > (%S).png", where %S is just 'new Date().toLocaleString()'.
> 
> 1. Joe suggested this as mac also uses this default name .

So? Does this mean yyyy-mm-dd won't cause confusion? Did you test what OS X does for different locales?

Potential l10n issues aside, I also dislike the weird grammatical structure of "Screen Shot yyyy-mm-dd". This doesn't seem to make sense when you read it out. Hence the proposed parentheses.

> 2. ':' is not allowed in file names on Windows.

Right, so you'd replace it with something else.

> 3. There will be sorting problems among the screenshots.

While it seems rather unlikely to me that people will build large screenshot archives using this method, I also expect these people to be advanced enough to sort files by date.
(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Girish Sharma [:Optimizer] from comment #25)
> > (In reply to Dão Gottwald [:dao] from comment #24)
> > > yyyy-mm-dd might be confusing for some locales. I'd prefer "Screenshot
> > > (%S).png", where %S is just 'new Date().toLocaleString()'.
> > 
> > 1. Joe suggested this as mac also uses this default name .
> 
> So? Does this mean yyyy-mm-dd won't cause confusion? Did you test what OS X
> does for different locales?
> 
> Potential l10n issues aside, I also dislike the weird grammatical structure
> of "Screen Shot yyyy-mm-dd". This doesn't seem to make sense when you read
> it out. Hence the proposed parentheses.
> 

I have made it location friendly so that both the parts yy-mm-dd and HH.MM.SS can be adjusted according to RTL/LTR or any other kind of localization. Moreover yyyy-mm-dd is standard. its the dd-mm-yyyy vs mm-dd-yyyy thats confusing.

I don't have Mac so can't test. Joe will have to answer to that :)

Overall, It was not really my call. I wanted even much simpler :).
(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Girish Sharma [:Optimizer] from comment #25)
> > 2. ':' is not allowed in file names on Windows.
> 
> Right, so you'd replace it with something else.

Note though that '.' might not be a great choice. This happens to be the separator dd.mm.yy in German.
(In reply to Girish Sharma [:Optimizer] from comment #27)
> I have made it location friendly so that both the parts yy-mm-dd and
> HH.MM.SS can be adjusted according to RTL/LTR or any other kind of
> localization.

No, you made the text surrounding the date and time translatable, not the date and time themselves.
(In reply to Dão Gottwald [:dao] from comment #29)
> (In reply to Girish Sharma [:Optimizer] from comment #27)
> > I have made it location friendly so that both the parts yy-mm-dd and
> > HH.MM.SS can be adjusted according to RTL/LTR or any other kind of
> > localization.
> 
> No, you made the text surrounding the date and time translatable, not the
> date and time themselves.

Yes, to avoid any issues with creating the file name (: or / or something like that), I made it standard. I am happy to change it to whatever is finalized.
Added the ability to capture to clipboard or fullpage even with a delay
Attachment #659762 - Attachment is obsolete: true
Attachment #659762 - Flags: review?(jwalker)
Attachment #659798 - Flags: review?(jwalker)
> I'd prefer "Screenshot (%S).png", where %S is just 'new Date().toLocaleString()'.

Like Optimizer, I'm not sure that string makes much sense. In the UK it's "Mon Sep 10 19:30:59 2012" - we don't write dates like that, in fact does anyone?

The idea of swapping : for something legal is not going to pass my review. All it takes is a locale that uses / or \ or * or + or something else that we've not thought of, or for the format of the locale string to change in the future, etc, etc.

Also we should remember that this is a utility, and we've been fairly exacting already. If it's important to people, we can file a follow-up, but we have *way* more important things than this problem.
Attachment #659798 - Flags: review?(jwalker) → review+
Blocks: 790026
Please correct the grammar in "Screen Shot yyyy-mm-dd", despite what OS X is doing. Also write "Screenshot" for consistency with the other strings.
(In reply to Dão Gottwald [:dao] from comment #33)
> Please correct the grammar in "Screen Shot yyyy-mm-dd", despite what OS X is

Grammar as in ?

> doing. Also write "Screenshot" for consistency with the other strings.

I guess "Screenshot " vs "Screen Shot" makes sense.
(In reply to Girish Sharma [:Optimizer] from comment #34)
> (In reply to Dão Gottwald [:dao] from comment #33)
> > Please correct the grammar in "Screen Shot yyyy-mm-dd", despite what OS X is
> 
> Grammar as in ?

Such that it reads naturally. "Screenshot, ....png" or "Screenshot (...).png" will work.
(In reply to Dão Gottwald [:dao] from comment #35)
> (In reply to Girish Sharma [:Optimizer] from comment #34)
> > (In reply to Dão Gottwald [:dao] from comment #33)
> > > Please correct the grammar in "Screen Shot yyyy-mm-dd", despite what OS X is
> > 
> > Grammar as in ?
> 
> Such that it reads naturally. "Screenshot, ....png" or "Screenshot
> (...).png" will work.

May be this would satisfy both Joe and Dao:

Screenshot [yyy-mm-dd at HH.MM.SS].png

It will sort properly, follows a good format for date and time and does not clash with file name limitations; But still, different locales can do it differently as it will only suit well in English.
Not sure why you'd use square rather than round brackets...
(In reply to Girish Sharma [:Optimizer] from comment #36)
> (In reply to Dão Gottwald [:dao] from comment #35)
> > (In reply to Girish Sharma [:Optimizer] from comment #34)
> > > (In reply to Dão Gottwald [:dao] from comment #33)
> > > > Please correct the grammar in "Screen Shot yyyy-mm-dd", despite what OS X is
> > > 
> > > Grammar as in ?
> > 
> > Such that it reads naturally. "Screenshot, ....png" or "Screenshot
> > (...).png" will work.
> 
> May be this would satisfy both Joe and Dao:
> 
> Screenshot [yyy-mm-dd at HH.MM.SS].png
> 
> It will sort properly, follows a good format for date and time and does not
> clash with file name limitations; But still, different locales can do it
> differently as it will only suit well in English.

Why use brackets at all? "Screenshot 2012-09-11 at 13.50.29" sounds fine to me, and matches exactly what OS X does (except the whitespace between 'screen' and 'shot' and avoidance of AM/PM).
(In reply to Victor Porof [:vp] from comment #38)
> Why use brackets at all? "Screenshot 2012-09-11 at 13.50.29" sounds fine to
> me

A comma or brackets don't affect how it sounds. They just make it easier to parse the phrase as intended. As is, it's grammatically incomplete, like a broken ellipsis. The date and time would normally be preceded by a preposition, but since this is dropped, they should be separated using a comma or parentheses. Alternatively, you could consistently go all the way in the opposite direction and remove all phrasal remnants (i.e. "at").
Being consistent with what the platform does is important. It means we naturally fit in, we should value that.

Secondly, this is a filename, so I'm not really much interested in debates about grammar.

Finally we've got big problems to solve, like our developer tools don't have a network monitor, or a profiler and have many bugs, so even if I'm wrong in the above reasons, I think we're better to get this wrong than discuss this at length.
(In reply to Joe Walker [:jwalker] from comment #40)
> Being consistent with what the platform does is important. It means we
> naturally fit in, we should value that.

There's some value. I think it's pretty low. Otherwise we should investigate what OS X does in other locales and what other OSes do (Gnome has built-in screenshot facilities, not sure about Windows). Victor also mentioned that OS X adds AM/PM, which the current patch doesn't. So the consistency argument seems like a strawman.

I have no interest in "debates about grammar" or in otherwise discussing this at length. I made some minor suggestions and I think you should pick them up. :)
If any one is going to push this to try first, please push all these three bugs together and in the following order :

bug 788890 , then bug 790026 and then bug 790294

and please land them also in the same order.

(the other two bugs still need r+ )
Gnome generates this file name:

Screenshot from 2012-09-12 09:58:31.png
Attached patch patch with comments addressed (obsolete) — Splinter Review
This is the same as the previous patch with screenshotGeneratedFilename changed to "Screenshot from %1$S %2$S".

Since the Gnome string doesn't have the quirks of the OS X string and since it satisfies the same "consistency with the OS" argument, if you want to make it, just taking this is probably the simplest way. Does anyone have a concern with this?
Attachment #659798 - Attachment is obsolete: true
Comment on attachment 660345 [details] [diff] [review]
patch with comments addressed

>--- a/browser/devtools/commandline/CmdScreenshot.jsm
>+++ b/browser/devtools/commandline/CmdScreenshot.jsm
>@@ -12,6 +12,10 @@ Cu.import("resource://gre/modules/XPCOMU
> XPCOMUtils.defineLazyModuleGetter(this, "LayoutHelpers",
>                                   "resource:///modules/devtools/LayoutHelpers.jsm");
> 
>+// String used as an indication to generate default file name in the following
>+// format: "Screen Shot yyyy-mm-dd at HH.MM.SS.png"
>+const FILENAME_DEFAULT_VALUE = " ";

I didn't update this comment; it's kind of bogus, as it seems to expect en-US. It would be best to rephrase this altogether.
(In reply to Dão Gottwald [:dao] from comment #45)
> Comment on attachment 660345 [details] [diff] [review]
> patch with comments addressed
> 
> >--- a/browser/devtools/commandline/CmdScreenshot.jsm
> >+++ b/browser/devtools/commandline/CmdScreenshot.jsm
> >@@ -12,6 +12,10 @@ Cu.import("resource://gre/modules/XPCOMU
> > XPCOMUtils.defineLazyModuleGetter(this, "LayoutHelpers",
> >                                   "resource:///modules/devtools/LayoutHelpers.jsm");
> > 
> >+// String used as an indication to generate default file name in the following
> >+// format: "Screen Shot yyyy-mm-dd at HH.MM.SS.png"
> >+const FILENAME_DEFAULT_VALUE = " ";
> 
> I didn't update this comment; it's kind of bogus, as it seems to expect
> en-US. It would be best to rephrase this altogether.

The comment thing, yes, maybe point to the localization file for reference.
I'm sorry Dão, but I'm serious about not wanting to spend time on the filename issue, since Optimizers version works without future updates, I'd like to use that.
No need to spend extra time on this. It's trivial, I just swapped the string, this should be ready to land. Comment 45 shouldn't block anyone.
Attachment #659798 - Attachment is obsolete: false
Attached patch debug oth failing fixed (obsolete) — Splinter Review
I am really sorry, but the all the debug oth 's are failing one test.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_screenshot.js | arg.selector.value - Got [object HTMLImageElement @ 0x9c58500 (native @ 0x9b2bb40)], expected [object HTMLImageElement]

This can be easily fixed.

but can anyone assure me that this is the only failing thing. I still don't know why the machine would crash after a failing test.

try : https://tbpl.mozilla.org/?tree=Try&rev=2dd709a66250

Unfortunately, the value property of args in helper.check doesn't support RegExp matching. So the only solution is to remove that check.

Attaching the same.
Attachment #660715 - Flags: review?(jwalker)
Attachment #660715 - Attachment is obsolete: true
Attachment #660715 - Flags: review?(jwalker)
Girish, could you please explain why you reverted to the previous string? Does it have any rational advantage that I missed?
(In reply to Dão Gottwald [:dao] from comment #51)
> Girish, could you please explain why you reverted to the previous string?
> Does it have any rational advantage that I missed?

If you mean why did I changed from [object HTML...] to content.document..., then two points:

1. The internal check in the test.ok uses '==' instead of '===' because of which my local test was passing (line 64 of the test attached)

2. On debug builds, elemenet.toString() is more verbose so it was not matching up.

otherwise if you mean that why did I use toString on the first hand... trial and error :P
(In reply to Girish Sharma [:Optimizer] from comment #52)
> (In reply to Dão Gottwald [:dao] from comment #51)
> > Girish, could you please explain why you reverted to the previous string?
> > Does it have any rational advantage that I missed?
> 
> If you mean why did I changed from [object HTML...] to content.document...,

Sorry, I wasn't referring to your attempt to fix the test failure, but to the screenshotGeneratedFilename string.
Dão, I accepted Girish's original patch, and I stand by my assessment that we should not be spending lots of time on this.
I could put forward my side of the debate as to why I think using Mac styling is better than Gnome styling, but for something this small, I'd rather be wrong but making progress, than right but bogged down.
Attachment #659798 - Attachment is obsolete: true
Attachment #660345 - Attachment is obsolete: true
(In reply to Joe Walker [:jwalker] from comment #54)
> Dão, I accepted Girish's original patch, and I stand by my assessment that
> we should not be spending lots of time on this.

You seem to assume I want to spend lots of time on this. I do not. I made simple, easy-to-address suggestions and I don't appreciate defensive attitude that they seem to trigger. If there's something wrong with my suggestions, I'm all ears, but ignoring them because they're minor is wrong. Bugzilla is filled with bugs about trivial enhancement requests that are being addressed. Us paying attention to details isn't new. Again, this does not necessarily mean to slow thing down, as long as we work together constructively and don't waste time on discussing why it might be better to not address enhancement requests.
(In reply to Dão Gottwald [:dao] from comment #55)
> (In reply to Joe Walker [:jwalker] from comment #54)
> > Dão, I accepted Girish's original patch, and I stand by my assessment that
> > we should not be spending lots of time on this.
> 
> You seem to assume I want to spend lots of time on this. I do not. I made
> simple, easy-to-address suggestions and I don't appreciate defensive
> attitude that they seem to trigger. If there's something wrong with my
> suggestions, I'm all ears, but ignoring them because they're minor is wrong.

We have a solution that is good enough for the many Mac users. I prefer consistency with a majority platform to be more important than consistency with a minor platform, and we're talking about filenames - the rules of grammar are not so important to me here.

So, having thought about it, I disagree with your proposal. I'm not just ignoring anyone that disagrees with me.
I know that these discussions take up lots of time and even more mental and emotional energy. I'm wanting to avoid these latter problems most of all. So having debated the issue for a while, I thought - "we're getting bogged down, how can we avoid this".

> Bugzilla is filled with bugs about trivial enhancement requests that are
> being addressed. Us paying attention to details isn't new. Again, this does
> not necessarily mean to slow thing down, as long as we work together
> constructively and don't waste time on discussing why it might be better to
> not address enhancement requests.

I'm not against attention to detail. I am against killing forward momentum.

So here's a suggestion - raise a follow-up bug. That way we're not holding up forward momentum so much. But I have to warn you that based on the points presented so far, I'm still going to disagree, however your chance of engaging me in debate is much higher when we're not holding things up while debating.
(In reply to Joe Walker [:jwalker] from comment #56)
> We have a solution that is good enough for the many Mac users. I prefer
> consistency with a majority platform to be more important than consistency
> with a minor platform,

As far as I can tell, the only real value of consistency with the platform in this case is sorting. This is broken for the string you want to use, due to the different time format.

> and we're talking about filenames - the rules of
> grammar are not so important to me here.

It's fine if you don't share my concerns. However, at that point the interesting question is whether you consider my proposed string objectively worse. Because if you just feel indifferent about the points I raised, the obvious path forward would be to pick the string that the least people have concerns with. (I'm not a native English speaker and make lots of mistakes, which may weaken my input. On the other hand, I studied linguistics, so it's probably not entirely irrelevant. Note that I didn't say file names should be full-fledged grammatically correct phrases -- grammar is not a religion to me. I said the proposed file name reads unnatural to me.)

> I know that these discussions take up lots of time and even more mental and
> emotional energy. I'm wanting to avoid these latter problems most of all. So
> having debated the issue for a while, I thought - "we're getting bogged
> down, how can we avoid this".

> I'm not against attention to detail. I am against killing forward momentum.

The problem I see here is that we're having a costly meta discussion. We're not actually discussing the proposed change.

> So here's a suggestion - raise a follow-up bug.

I can do that, but honestly I think that's more overhead than necessary here.
https://tbpl.mozilla.org/?tree=Try&rev=774d88983bbe

Yet to complete WinXP, but all tests passes that were failing earlier.

Ready to be pushed.
Attachment #660751 - Attachment is obsolete: true
Attachment #660984 - Flags: review?(jwalker)
Please follow this order of pushing : 

bug 788890 , then bug 790026 and then bug 790294
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/c4ebd4e89975
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
I'm answering this because you think this is personal, and to demonstrate that it's not. I still think, however that we shouldn't be having this conversation as it's just not worth it.

(In reply to Dão Gottwald [:dao] from comment #57)
> As far as I can tell, the only real value of consistency with the platform
> in this case is sorting. This is broken for the string you want to use, due
> to the different time format.

The only value of consistency isn't sorting. I don't know how apps that upload your screenshots like GrapUp and CloudApp work, but there is a better chance of integration with that type of thing if we do it the standard platform way. Platform consistency is of value even if it's nothing more than 'it just looks the same'.

Also sorting not totally broken with the string that I want to use for 2 reasons;
1. This is locale specific. It works fine on my mac, and I suspect, many macs across Europe and Asia that use the 24 hour format.
2. Even in locales that don't use the 24 hour clock the sorting is still nearly right.

I considered changing to the AM/PM system, but that doesn't really solve the sorting problem.

So I do think that your proposal is objectively worse. I'm sorry.
https://hg.mozilla.org/mozilla-central/rev/c4ebd4e89975
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Depends on: 792533
Comment on attachment 660984 [details] [diff] [review]
Fianl patch. Passes tests.

This patch has landed
Attachment #660984 - Flags: review?(jwalker) → review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: