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

implemetation of AtomCommandline to preserve args #1256

Merged
merged 1 commit into from Mar 29, 2015

Conversation

deepak1556
Copy link
Member

Inital work for #1248 . @zcbenz If its desirable to have our own commandline class , then i can implement other methods from base::commandline for our needs and eliminate its usage. If this is too heavy for a trivial stuff, feel free to close it. would love to hear any better approaches :)

EDIT: sorry didnt mean eliminate base::commandLine but just atomCommandLine will wrap around it, so all commandLine calls to base will happen through this new class.

@bwin
Copy link
Contributor

bwin commented Mar 17, 2015

Thanks for working on this.

@zcbenz
Copy link
Member

zcbenz commented Mar 19, 2015

We don't need to replicate base::CommandLine's interface, our purpose is to keep a copy of argv and then pass it to node::CreateEnvironment, so a simple singleton class that keeps a references to argv should be enough in our case.

new_args.insert(new_args.end(), *it);
args.swap(new_args);
AtomCommandLine::Reset();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zcbenz then this hack would be present to stitch atom-shell swtich arguments from https://github.com/atom/atom-shell/blob/master/atom/app/atom_main_delegate.cc#L71 before sending to node::Environment ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we will use the original argv, this hack will not be needed anymore.

@deepak1556 deepak1556 force-pushed the process_arg_patch branch 3 times, most recently from 6e8653a to 177a59f Compare March 24, 2015 19:50
}

current_process_commandline_ = new AtomCommandLine(NO_PROGRAM);
current_process_commandline_->InitFromArgv(argc, argv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make AtomCommandLine's data members static so we don't have to create a current_process_commandline_ instance.

@deepak1556 deepak1556 force-pushed the process_arg_patch branch 2 times, most recently from d6695aa to a328dac Compare March 28, 2015 13:21
@deepak1556
Copy link
Member Author

@zcbenz made the changes and added tests, havnt tested on windows though. Would be nice if someone did :)

@zcbenz
Copy link
Member

zcbenz commented Mar 29, 2015

It works on Windows, thanks!

zcbenz added a commit that referenced this pull request Mar 29, 2015
implemetation of AtomCommandline to preserve args
@zcbenz zcbenz merged commit 27d2dbf into electron:master Mar 29, 2015
@zcbenz zcbenz mentioned this pull request Mar 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants