Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying the screenshot path on the command line #3698

Merged
merged 3 commits into from Feb 16, 2022

Conversation

criezy
Copy link
Member

@criezy criezy commented Jan 31, 2022

This allow specifying a path where to save screenshots on the command line.

See pull request #3675 for another pull request that attempt this. That pull request was however incorrect with no feedback from the author since it was open. So I decided to rewrite it.

See also https://bugs.scummvm.org/ticket/13245.

There are two questions I have in this pull request:

  1. Should we only commit the first commit or all three?
  2. Is there a better way to handle the terminating '\' or Windows, and does the code I have work for all cases.

Description of the changes

The first commit is simple and corresponds to what was done in the earlier pull request but without the bugs (and with an update to the documentation). It adds a command line option that, when used, adds a screenshotpath to the transient domain of ConfMan. Since the SDL backend already queries ConfMan for this key, this just works™

The other two commits are maybe more controversial and the reason why I am proposing this as a pull request instead of pushing the code directly. Since the transient domain in ConfMan is cleared when starting the launcher, this means that the path is only used if a game is also specified on the command line, and only until returning to the launcher. It seems to me that it would make more sense to use it even when starting with the launcher (i.e. no game is specified on the command line), and until the user quits ScummVM (so potentially for multiple games). This is what is implemented in those two commits. And this is also more or less how the --logfile=PATH option behaves (since the log file is opened when starting ScummVM at a point where the transient domain has not yet been cleared). This however diverges from the behaviour for most other options we can pass on the command line.

Handling of path separator on Windows

Also I am not completely sure how Windows handles paths with a mixture of '/' and '\'. So I was a bit cautious with my changes to the OSystem_Win32 class in 9e9a3a7. But maybe the code could be simplified.

And also if the user specifies a path that ends with a '/', it is now replaced by a '\', which was not the case before. Could that be an issue? Or is it actually better? If needed there is a simple way to get back the path as it exactly was before my changes by adding the following to OSystem_Win32::initBackend() after it calls OSystem_SDL::initBackend() and removing the handling of the backslash from OSystem_Win32::getScreenshotsPath():

	// Invoke parent implementation of this method
	OSystem_SDL::initBackend();

	// Make sure that if the user specified a screenshot path we have the correct path separator at the end
	_userScreenshotPath = ConfMan.get("screenshotpath");
	if (!_userScreenshotPath.empty() && !_userScreenshotPath.hasSuffix("\\") && !_userScreenshotPath.hasSuffix("/"))
		_userScreenshotPath += "\\";
}

However to me this seems less clean than what I did.

Note that since the ConfMan transient domain is cleared when opening
the launcher, this only work when specifying a game on the command
line as well, and only until returning to the launcher.
…and macOS backends

Instead we call the OSystem_SDL implementation of getScreenshotsPath(),
as done in the POSIX backend. This change means that if we change how
we handle a user-specified screenshot path in the SDL backend, the
Windows and macOS backends will still get the correct path.
There are two ways the user can specify a screenshot path: by
editing the config file manually, or by passing it on the command
line. In the later case it is added to the transient domain that
is cleared when opening the launcher, so it only worked when also
specifying a game to start on the command line. With this change
a screenshot path specified on the command line will be used until
quitting ScummVM.

This could be confusing if the user had the ability to specify the
path in the ScummVM Options, as then we would probably want to use
the new specified path immediately. But since the path does not
appear in the options, this change should work fine.
@criezy criezy changed the title Cmd screenshotpath Allow specifying the screenshot path on the command lime Jan 31, 2022
@criezy criezy changed the title Allow specifying the screenshot path on the command lime Allow specifying the screenshot path on the command line Jan 31, 2022
@criezy criezy mentioned this pull request Jan 31, 2022
@criezy
Copy link
Member Author

criezy commented Feb 16, 2022

Since there is not a lot of feedback on this, I will merge it as it is.

@criezy criezy merged commit f3dc1df into scummvm:master Feb 16, 2022
@criezy criezy deleted the cmd-screenshotpath branch February 16, 2022 22:24
@lwcorp
Copy link
Contributor

lwcorp commented Feb 17, 2022

I see you've added --screenshotpath=PATH but can you also add a short version like -s although not -s since it's already taken?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants