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

CMD: Detect and add game(s) with commandline, Feature request #7885 #926

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@yinsimei
Contributor

yinsimei commented Mar 22, 2017

GCoS 2017. Move the cmd commit to a branch to work in master without affecting the PR.

Show outdated Hide outdated base/commandLine.cpp Outdated
Show outdated Hide outdated base/commandLine.cpp Outdated
Show outdated Hide outdated base/commandLine.cpp Outdated
Show outdated Hide outdated base/commandLine.cpp Outdated
Show outdated Hide outdated base/commandLine.cpp Outdated
@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Mar 27, 2017

Member

Overall this looks quite good, although there are some changes that need to be made.

I am also wondering if we could avoid some code duplication between the GUI and the command line for adding games to the config file and for the mass add detection code.

Member

criezy commented Mar 27, 2017

Overall this looks quite good, although there are some changes that need to be made.

I am also wondering if we could avoid some code duplication between the GUI and the command line for adding games to the config file and for the mass add detection code.

@wjp

This comment has been minimized.

Show comment
Hide comment
@wjp

wjp Mar 27, 2017

Member

"Feature request #703" is old SF/Allura numbering. You should mention 7885 instead ( https://bugs.scummvm.org/ticket/7885 ).

Member

wjp commented Mar 27, 2017

"Feature request #703" is old SF/Allura numbering. You should mention 7885 instead ( https://bugs.scummvm.org/ticket/7885 ).

@yinsimei yinsimei changed the title from CMD: Detect and add game(s) with commandline, Feature request #703 to CMD: Detect and add game(s) with commandline, Feature request #7885 Mar 27, 2017

@yinsimei

This comment has been minimized.

Show comment
Hide comment
@yinsimei

yinsimei Mar 27, 2017

Contributor

I am also wondering if we could avoid some code duplication between the GUI and the command line for adding games to the config file and for the mass add detection code.

Yes, I've noticed the duplication. Do you think it will be a good idea to have a massDetectGames in MetaEngine?

Contributor

yinsimei commented Mar 27, 2017

I am also wondering if we could avoid some code duplication between the GUI and the command line for adding games to the config file and for the mass add detection code.

Yes, I've noticed the duplication. Do you think it will be a good idea to have a massDetectGames in MetaEngine?

@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Apr 23, 2017

Member

Thank you.
I have now merged this change after squashing the two commits.

Member

criezy commented Apr 23, 2017

Thank you.
I have now merged this change after squashing the two commits.

@criezy criezy closed this Apr 23, 2017

@yinsimei yinsimei deleted the yinsimei:cmd branch May 10, 2017

@@ -68,6 +69,13 @@ static const char HELP_STRING[] =
" -z, --list-games Display list of supported games and exit\n"
" -t, --list-targets Display list of configured targets and exit\n"
" --list-saves=TARGET Display a list of saved games for the game (TARGET) specified\n"
" -a, --add Add a game from current or specified directory\n"
" Use --path=PATH before -a, --add to specify a directory.\n"
" --massadd Add all games from current or specified directory and all sub directories\n"

This comment has been minimized.

@tobiatesan

tobiatesan Jun 24, 2017

Contributor

This is very useful, but I find --massadd is super unusual as a command line switch. Could we change this to --all (perhaps with -A) or something before 1.10?

@yinsimei @criezy @sev-

@tobiatesan

tobiatesan Jun 24, 2017

Contributor

This is very useful, but I find --massadd is super unusual as a command line switch. Could we change this to --all (perhaps with -A) or something before 1.10?

@yinsimei @criezy @sev-

This comment has been minimized.

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

We've discussed the issue a bit with @criezy, see: http://logs.scummvm.org/log.php?log=scummvm.log.07Jul2017&format=text

TL;DR, I don't find this very intuitive and I don't especially like the idea of interaction (if you want interaction, you'd use a GUI after all...)

The semantics I'd like to propose is:

-p <dir> --add                adds all games
-p <dir> --detect             enumerates dectected games in <dir>
-p <dir> --game <id> --add     adds just game <id>

How do you like it?

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

We've discussed the issue a bit with @criezy, see: http://logs.scummvm.org/log.php?log=scummvm.log.07Jul2017&format=text

TL;DR, I don't find this very intuitive and I don't especially like the idea of interaction (if you want interaction, you'd use a GUI after all...)

The semantics I'd like to propose is:

-p <dir> --add                adds all games
-p <dir> --detect             enumerates dectected games in <dir>
-p <dir> --game <id> --add     adds just game <id>

How do you like it?

This comment has been minimized.

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

(If you like the current semantics, I think the help should at least mention that -a is interactive -- especially since it's interactive only some of the time; that might give an headache to some script writer and/or cabinet modder.)

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

(If you like the current semantics, I think the help should at least mention that -a is interactive -- especially since it's interactive only some of the time; that might give an headache to some script writer and/or cabinet modder.)

This comment has been minimized.

@bgK

bgK Jul 7, 2017

Member

That sounds good and actually usable by third party launchers.

Maybe the addGameToConf function could additionally display the target of added games so that it can be used for subsequent calls to scummvm.

@bgK

bgK Jul 7, 2017

Member

That sounds good and actually usable by third party launchers.

Maybe the addGameToConf function could additionally display the target of added games so that it can be used for subsequent calls to scummvm.

This comment has been minimized.

@yinsimei

yinsimei Jul 7, 2017

Contributor

So, if I understand well, we'll use --add in the place of --massadd and add a --game option for --add ?

@yinsimei

yinsimei Jul 7, 2017

Contributor

So, if I understand well, we'll use --add in the place of --massadd and add a --game option for --add ?

This comment has been minimized.

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

@yinsimei yes, that's essentially the idea. And --add would error out if there are multiple games and no --game option is passed (possibly enumerating them or just advising to use --detect) it's hot here
@bgK good idea, I like it.

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

@yinsimei yes, that's essentially the idea. And --add would error out if there are multiple games and no --game option is passed (possibly enumerating them or just advising to use --detect) it's hot here
@bgK good idea, I like it.

This comment has been minimized.

@criezy

criezy Jul 7, 2017

Member

And --add would error out if there are multiple games and no --game option is passed

@tobiatesan I though we agreed during the discussion on IRC today that --add without --game would add all the games found (like the current --massadd)? And that is also what one of your earlier comment above says.

@criezy

criezy Jul 7, 2017

Member

And --add would error out if there are multiple games and no --game option is passed

@tobiatesan I though we agreed during the discussion on IRC today that --add without --game would add all the games found (like the current --massadd)? And that is also what one of your earlier comment above says.

This comment has been minimized.

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

@criezy yes, sorry, that's correct. I honestly have no idea why I wrote the bit you quoted.

@tobiatesan

tobiatesan Jul 7, 2017

Contributor

@criezy yes, sorry, that's correct. I honestly have no idea why I wrote the bit you quoted.

tobiatesan added a commit to tobiatesan/scummvm that referenced this pull request Jul 10, 2017

CMD: Fix batch adding of games
This implements the behaviour as discussed in PR926:
scummvm#926 (comment)

Essentially:

[-p <dir>] --add                adds all games in <dir> or working dir
[-p <dir>] --detect             enumerates dectected games with their
ids
[-p <dir>] --game <id> --add    adds just game <id>

tobiatesan added a commit to tobiatesan/scummvm that referenced this pull request Jul 10, 2017

CMD: Fix batch adding of games
This implements the behaviour as discussed in PR926:
scummvm#926 (comment)

Essentially:

[-p <dir>] --add                adds all games in <dir> or working dir
[-p <dir>] --detect             enumerates dectected games with their
ids
[-p <dir>] --game <id> --add    adds just game <id>

tobiatesan added a commit to tobiatesan/scummvm that referenced this pull request Jul 10, 2017

CMD: Fix batch adding of games
This implements the behaviour as discussed in PR926:
scummvm#926 (comment)

Essentially:

[-p <dir>] --add                adds all games in <dir> or working dir
[-p <dir>] --detect             enumerates dectected games with their
ids
[-p <dir>] --game <id> --add    adds just game <id>

tobiatesan added a commit to tobiatesan/scummvm that referenced this pull request Aug 5, 2017

CMD: Fix batch adding of games
This implements the behaviour as discussed in PR926:
scummvm#926 (comment)

Essentially:

[-p <dir>] --add                adds all games in <dir> or working dir
[-p <dir>] --detect             enumerates dectected games with their
ids
[-p <dir>] --game <id> --add    adds just game <id>

criezy added a commit that referenced this pull request Aug 6, 2017

CMD: Fix batch adding of games
This implements the behaviour as discussed in PR926:
#926 (comment)

Essentially:

[-p <dir>] --add                adds all games in <dir> or working dir
[-p <dir>] --detect             enumerates dectected games with their
ids
[-p <dir>] --game <id> --add    adds just game <id>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment