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

Command Line option to load campaign file on startup #1165

Closed
JamzTheMan opened this issue Jan 21, 2020 · 13 comments
Closed

Command Line option to load campaign file on startup #1165

JamzTheMan opened this issue Jan 21, 2020 · 13 comments
Assignees
Labels
documentation needed Missing, out-of-date or bad documentation feature Adding functionality that adds value tested This issue has been QA tested by someone other than the developer.

Comments

@JamzTheMan
Copy link
Member

JamzTheMan commented Jan 21, 2020

Is your feature request related to a problem? Please describe.
When MapTool starts, I would like the ability to supply a path to the Campaign and have MapTool automatically load it.

Describe the solution you'd like
Add a -load="" to CLI options

Describe alternatives you've considered
Currently I have to use UI interface. This slows down development when MapTool is launched via IDE, especially when done repeatedly and campaigns are in different locations.

Additional context
Having CLI option let's you set multiple IDE configs to different campaigns as well as script the launch of MapTool with campaigns using new test.functions to load multiple MapTool instances with specified campaigns.

@Phergus Phergus added the feature Adding functionality that adds value label Jan 21, 2020
@Phergus Phergus added the documentation needed Missing, out-of-date or bad documentation label Jan 21, 2020
@Azhrei
Copy link
Member

Azhrei commented Jan 21, 2020

Okay, but still don't need a -l option. Any command line argument without an option is a file name to be processed. It's the same as every command line utility ever. 🙂

@JamzTheMan
Copy link
Member Author

So your only issue is it should be no command vs -load? (I suppose it really should have been -f / -file which brings it more into line with other command lines)

I can see if the Apache lib supports a no command arg. We already support about a dozen command line args using it and I'm not going to write special logic for this one case.

@Azhrei
Copy link
Member

Azhrei commented Jan 22, 2020

It does. I commented on the PR and almost closed it, but decided it was your code, so your choice. But I wouldn't be the one to merge it.

@Azhrei
Copy link
Member

Azhrei commented Jan 22, 2020

Sorry, that sounds kinda snarky. I just mean that I have a strong opinion on command line syntax and I don't like forcing people to do things a certain way when I have really strong opinions. (Even though I will advocate strongly in such cases. 🙂)

@Phergus
Copy link
Contributor

Phergus commented Jan 22, 2020

I agree that use -f/-file is probably a better choice.

While I would like to see it support a standalone path/filename, as it should then also work with file association, I'm not invested in the idea really.

@JamzTheMan
Copy link
Member Author

@Phergus Oops, -f is already used for fullscreen
I can make it -file but the single letter has to be something else, suggestions?

@Azhrei I see no way in Apache command cli lib to specify no flag for Options. If you want to refactor it the whole CLI options for MT, go for it.

I wanted something that saved me a few seconds per launch and this was a quick 2 minute code fix using existing approach. I don't have time to refactor the whole thing to "bring it inline with other CLI tools". Besides, MapTool is NOT a command line tool, this is a flag to pass an argument to a UI application, I doubt it's really going to bug people to add a single flag to invoke this.

@JamzTheMan
Copy link
Member Author

Update, the lib is case sensitive (like most CLI tools) so -F/-file IS an option and does not interfere with -f/-fullscreen.

Thoughts?

@Azhrei
Copy link
Member

Azhrei commented Jan 22, 2020

Actually, I put it in the PR comment.

The current code creates a DefaultParser over and over, when it should create one just once. The parse() method then returns a CommandLine object. With just one such object, all options can be checked and any remaining arguments are available.

I'd be happy to make the change, but I didn't want to step on your toes. Since you want this sooner instead of later, I'll work on it tonight and get the PR submitted.

@JamzTheMan
Copy link
Member Author

I only need in in my dev branch and not in a "live" release so go ahead.
BUT, I'd also still want the -l (or -F) as an option. I'm ok with it being a "default" and not require a flag, I personally like specifying flags to be explicit. Several CLI tools let you do it either way.

@JamzTheMan
Copy link
Member Author

If you want to tackle the refactor, just merge and pull this associated PR and then submit a new one to "enhance" it to allow for no flag and refactor the parser.

@Phergus
Copy link
Contributor

Phergus commented Jan 22, 2020

Sorry. Had to go make/eat dinner. Works for me.

@Phergus
Copy link
Contributor

Phergus commented Jan 22, 2020

Tested under Win10:

  • From command line - maptool -F=C:\path\to\file.cmpgn
  • From command line - maptool -file=C:\path\to\file.cmpgn
  • Via shortcut - same syntax as above
  • File association - double-click on campaign file opens campaign in MapTool.

@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label Jan 22, 2020
Phergus added a commit that referenced this issue Jan 23, 2020
@Phergus
Copy link
Contributor

Phergus commented Jan 23, 2020

Documented command line options on wiki.

@Phergus Phergus closed this as completed Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation needed Missing, out-of-date or bad documentation feature Adding functionality that adds value tested This issue has been QA tested by someone other than the developer.
Projects
None yet
Development

No branches or pull requests

3 participants