Opened 9 years ago

Closed 9 years ago

#5724 closed enhancement (fixed)

Screenshot enhancement : output filename specification in silent mode

Reported by: Shisui Owned by: axeld
Priority: normal Milestone:
Component: Applications/Screenshot Version:
Keywords: gsoc2010 Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

This enhancement allows the user to use parametric filenames. It offers more flexibility than automatic file numbering in silent mode (screenshot -s). Recognized parameters include those parsed by strftime() and additional ones related to the screenshot size and output type.

For example :

screenshot -s "screenshot \$wx\$h %m-%d-%y %H:%M:%S.png"

Produces a file named "screenshot 1024x768 04-14-2010 21:30:32.png" in the current directory.

(the backslashes are required if the shell interpret $ as variables delimiter)

Known parameters (in addition of strftime() format parameters) are $w and $h for the width and height of the screenshot, and $t for the format type of the produced file (e.g: PNG, BMP, ..)

Parameters are also parsed in window mode.

Attachments (2)

screenshotEnhancement-parametricFilename.patch (10.2 KB ) - added by Shisui 9 years ago.
Patch implementing the parametric filenames functionality
screenshotEnhancement-filenameInSilentMode.patch (3.5 KB ) - added by Shisui 9 years ago.
Allows the user to specify an output filename in silent mode.

Download all attachments as: .zip

Change History (16)

comment:1 by axeld, 9 years ago

To be honest, I find this pretty much useless and overkill. But maybe that's just me, other opinions welcome.

comment:2 by Shisui, 9 years ago

It's useful for scripting, for example, the user has a greater control over the generated filename.

comment:3 by axeld, 9 years ago

Besides that there is very limited use of scripting Screenshot, most of the data is available in the shell already, so you could easily build such names there directly, without having special support in Screenshot.

The only thing that would make sense to add to Screenshot in this regard is being able to change the filename in the first place, though.

The patch itself looks good, I just don't see much use in the functionality it adds.

BTW, "static" is a qualifier like "virtual"; it should be indented in the same way. Also, '{' goes to the end of the previous line, the only exceptions are functions.

comment:4 by Shisui, 9 years ago

The patch also enables filename/directory selection in silent mode. Even with this, if someone creates a desktop shortcut to make a screenshot, the only possibilities he gets are creating screenshot#.png in the prefered directory, or a fixed filename in a specified directory. I don't say that adding a timestamp in the filename is a everyday use of Screenshot, but it can be useful, it's a "plus" I believe. Anyway, I updated the patch according to the coding guidelines mistakes you raised, thanks :)

comment:5 by axeld, 9 years ago

As I said, the filename/directory specification definitely makes sense.

The rest, however, does not IMO, since you can create the very same name using bash without any extra feature in Screenshot, and that also includes the timestamp (which is a bit superfluous, since we have last modified and creation time).

comment:6 by axeld, 9 years ago

BTW thanks for the updated patch. However, there are still lots of coding style issues with it, for example:

(line 140)

	void* item; 
	for ( int32 i = 0; ; i++ ) { 
		item = fFilenameParameters.ItemAt(i); 

Since "item" is not used outside the loop, why not declare it inside? There shouldn't be spaces between ().

(line 447)

	.Add(new BButton("", TR("Options"), new 
BMessage(kShowOptions))) 

What happened here?

(line 819 and others)

ScreenshotWindow::_AddFileParameterRule(FileParameter index, 
                   const char* name) 

Only one tab for indentation in this case.

(line 849)

	case kImageWidth: 
		rule->replacementValue << 
			screenshotParent->fScreenshot->Bounds().IntegerWidth() + 1; 

The operator always goes to the start of the next line.

It also doesn't fix the '{' style everywhere. Please consult http://www.haiku-os.org/development/coding-guidelines for more information.

in reply to:  6 comment:7 by Shisui, 9 years ago

(line 447) What happened here?

Good question ... Mistyping or vim line wrap rule I think, I missed it while reading the patch.

I'm not yet used to Haiku coding guidelines, I just discovered checkstyle.py, It already gives me headaches! :) I tried to fix what it reported.

by Shisui, 9 years ago

Patch implementing the parametric filenames functionality

comment:8 by axeld, 9 years ago

Adapting to a coding style always takes a bit of time, knowing it is a good start, though :-)

Anyway, would you mind doing an alternative version of your patch that only implements adding support for a different file name? That part of the patch is definitely an improvement, while I personally wouldn't want to apply the rest.

comment:9 by Shisui, 9 years ago

Summary: Screenshot enhancement : parametric filenamesScreenshot enhancement : output filename specification in silent mode

In fact the soft patch was already done, given your comments, but I forget to send it, sorry, here it is.

by Shisui, 9 years ago

Allows the user to specify an output filename in silent mode.

comment:10 by axeld, 9 years ago

Great, thanks! I've applied it in hrev36312.

I've made a few changes, and here is why:

  • There is no reason to make the filename parameter a BString, so I changed it to a const char*.
  • Furthermore, a BString is empty by default, so there is no need to set it to "" manually.
  • Also, it should have been "const BString&" in order to save an extra copy when passing the object around.

In any case, I keep this ticket open for a while to collect other opinions on the matter.

comment:11 by axeld, 9 years ago

Owner: changed from julun to axeld
Status: newin-progress

comment:12 by Shisui, 9 years ago

Keywords: gsoc2010 added

Thanks for those advices :) (I can't see why I didn't keep a const char* for this basic use of strings..)

comment:13 by Wim, 9 years ago

I tend to agree with Axel that adding parametric filename functionality doesn't make sense.

comment:14 by Wim, 9 years ago

Resolution: fixed
Status: in-progressclosed

Closing the ticket.

In addition, the screenshotEnhancement-parametricFilename.patch can't be applied cleanly anyway with the latest screenshot update.

Note: See TracTickets for help on using tickets.