Skip to content

Switch to getopt for command line parsing #169

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.

Sign up for GitHub

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

Draft
augiedoggie wants to merge 1 commit into
base: master
Choose a base branch
from
+123 −88
Draft

Switch to getopt for command line parsing #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 6 additions & 31 deletions src/App.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ App::~App()


void
App::Init()
App::Init(bool suppressWindow)
{
BPath settingsPath;
find_directory(B_USER_SETTINGS_DIRECTORY, &settingsPath);
Expand All @@ -88,6 +88,8 @@ App::Init()
EditorWindow::SetPreferences(fPreferences);

Languages::LoadLanguages();

fSuppressInitialWindow = suppressWindow;
}


Expand Down Expand Up @@ -200,36 +202,12 @@ App::QuitRequested()
void
App::ReadyToRun()
{
if(CountWindows() == 0 && fSuppressInitialWindow == false) {
if(fSuppressInitialWindow == false && CountWindows() == 0) {
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

Reversing the order is meaningful here?

Copy link
Contributor Author

@augiedoggie augiedoggie Jan 26, 2026

Choose a reason for hiding this comment

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

At first this was done accidentally because I removed it in one change and then put it back in a later change. Technically this would be more efficient because the first test is a simple boolean variable comparison and so it wouldn't need to proceed with a function call and whatever else CountWindows() does.

PostMessage(WINDOW_NEW);
}
}


void
App::ArgvReceived(int32 argc, char** argv)
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

What's the reason behind removing this?

Copy link
Contributor Author

@augiedoggie augiedoggie Jan 26, 2026

Choose a reason for hiding this comment

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

I think handling command line arguments in two places complicates the startup process and possibly introduces code duplication, so it would be nice to do it in one place or the other. If the arguments are processed inside main() then we can do validity checks on the arguments before we even create the BApplication object. This makes it easier to return proper exit codes when run from the command line. It can also prevent the situation where you create the BApplication, have errors during ArgvReceived, and have to destroy/Quit() the BApplication right away if there are no other windows.

{
BMessage* message = CurrentMessage();
BString cwd = message->GetString("cwd", "~");
std::unique_ptr<BWindowStack> windowStack;
for(int32 i = 1; i < argc; ++i) {
std::string arg = argv[i];
// FIXME: this should be handled in main.cpp probably
if(arg == "-w" || arg == "--wait" || arg == "-h" || arg == "--help")
continue;
int32 line, column;
std::string filename = ParseFileArgument(argv[i], &line, &column);
if(filename.find('/') != 0) {
BPath absolute(cwd.String(), filename.c_str(), true);
filename = absolute.Path();
}
entry_ref ref;
BEntry(filename.c_str()).GetRef(&ref);
_ActivateOrCreateWindow(message, ref, line, column, windowStack);
}
}


void
App::RefsReceived(BMessage* message)
{
Expand All @@ -248,8 +226,8 @@ App::RefsReceived(BMessage* message)
trackerMessage.AddRef("refs", &ref);
continue;
}
const int32 line = message->GetInt32("be:line", -1);
const int32 column = message->GetInt32("be:column", -1);
const int32 line = message->GetInt32("be:line", i, -1);
const int32 column = message->GetInt32("be:column", i, -1);
_ActivateOrCreateWindow(message, ref, line, column, windowStack);
}
}
Expand All @@ -263,9 +241,6 @@ void
App::MessageReceived(BMessage* message)
{
switch(message->what) {
case SUPPRESS_INITIAL_WINDOW: {
fSuppressInitialWindow = true;
} break;
case ACTIVATE_WINDOW: {
BWindow* window = nullptr;
if(message->FindPointer("window", (void**) &window) == B_OK && window != nullptr) {
Expand Down
4 changes: 1 addition & 3 deletions src/App.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ class Styler;


enum {
SUPPRESS_INITIAL_WINDOW = 'Siwn',
WINDOW_NEW_WITH_QUIT_REPLY = 'NWwn',
ACTIVATE_WINDOW = 'actw'
};
Expand All @@ -38,12 +37,11 @@ class App : public BApplication {
App();
~App();

void Init();
void Init(bool suppressWindow);

void AboutRequested();
bool QuitRequested();
void ReadyToRun();
void ArgvReceived(int32 argc, char** argv);
void RefsReceived(BMessage* message);
void MessageReceived(BMessage* message);

Expand Down
170 changes: 116 additions & 54 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,75 +5,137 @@

#include "App.h"

#include <string>

#include <Roster.h>

#include <iostream>
#include <getopt.h>

#include "EditorWindow.h"
#include "Utils.h"


void
_PrintUsage()
{
std::cerr
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

When this is output from --help it should be printed to stdout (std::cout). If it's a result of unknown option or other error case it should go to std::cerr.

<< "Usage: Koder [options] file..." << std::endl
Copy link
Owner

@KapiX KapiX Jan 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Using std::endl is doubly wrong here. std::endl besides adding a newline also flushes the stream but:

  1. std::cerr is unbuffered and will flush after any output that is sent to it.
  2. Flushing after every newline usually only degrades performance, with no benefit.

Flushing is useful if you are outputting text before very long operation (the OS might not show the output without flush until after the operations ends).
Change endls to "\n".

Copy link
Contributor Author

@augiedoggie augiedoggie Jan 27, 2026

Choose a reason for hiding this comment

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

Ok, I can't remember if I usually use endl of '\n' in my own apps, it's probably a 50/50 split.

Of course, if you'd like I can switch back to the c-style fprintf too, don't hesitate to ask for it.

<< "Options:" << std::endl
<< " -h, --help\t\tPrints this message." << std::endl
<< " -w, --wait\t\tWait for the window to quit before returning." << std::endl
<< "\t\t\tOpening in window stacks is not supported in this mode." << std::endl
<< "\t\t\tCurrently accepts only one filename." << std::endl;
}


void
_RunApp(bool suppressWindow)
{
App* app = new App();
app->Init(suppressWindow);
app->Run();
delete app;
}


int
main(int argc, char** argv)
{
std::string arg1 = argc > 1 ? argv[1] : "";
if(argc > 1 && (arg1 == "-h" || arg1 == "--help")) {
fprintf(stderr, "Usage: Koder [options] file...\n");
fprintf(stderr, "Options:\n");
fprintf(stderr, " -h, --help\t\tPrints this message.\n");
fprintf(stderr, " -w, --wait\t\tWait for the window to quit before "
"returning.\n"
"\t\t\tOpening in window stacks is not supported in this mode.\n"
"\t\t\tCurrently accepts only one filename.\n");
// start the app immediately if we're being launched from the GUI in case we need to handle
// an initial BMessage, otherwise the message will be lost when we Launch() in the background
if(isatty(STDOUT_FILENO) == 0) {
_RunApp(false);
return 0;
}

int option_index = 0;
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

Suggested change
int option_index = 0;
int optionIndex = 0;

struct option long_options[] = {
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

Suggested change
struct option long_options[] = {
auto long_options = std::to_array<option>{

If std::to_array is not available, bump -std to c++20 in makefile.

{"help", no_argument, 0, 'h'},
{"wait", no_argument, 0, 'w'},
{"launchkoderapp", no_argument, 0, 0},
{0, 0, 0, 0}};

bool waitForExit = false;
int c;
while((c = getopt_long(argc, argv, "hw", long_options, &option_index)) != -1) {
switch(c) {
case 'w': {
waitForExit = true;
} break;
case 'h': {
_PrintUsage();
return 1;
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

It's not an error when -h is used.

} break;
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

redundant break

case 0: {
if(strcmp(long_options[option_index].name, "launchkoderapp") == 0) {
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

What's the use case for this option?

Copy link
Contributor Author

@augiedoggie augiedoggie Jan 26, 2026

Choose a reason for hiding this comment

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

Yeah, I was trying to avoid the "secret switch" thing. To be consistent, Koder should always fork to the background unless --wait is used. So, we can use BRoster::Launch to do that, but we need to tell the newly launched copy of the application that it should start the BApplication process and prepare to receive one of the three message types below.

In some ways this is similar to the isatty() == 0 situation except that when running from the command line, the newly launched copy may have a stdout file descriptor.

_RunApp(true);
return 0;
}
return 1;
} break;
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

redundant break

default:
_PrintUsage();
return 1;
}
}

if(waitForExit == true && argc - optind > 1) {
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

optind?

Copy link
Contributor Author

@augiedoggie augiedoggie Jan 26, 2026

Choose a reason for hiding this comment

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

optind is the current argument index. The only reason I chose this variable name is that it's pretty common when using getopt, a github/google search for "optind" will have a huge number of results. I don't have a problem with changing it though.

Copy link
Contributor Author

@augiedoggie augiedoggie Jan 26, 2026

Choose a reason for hiding this comment

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

Actually, I was mistaken. This is a global variable that getopt uses and I don't think it can be changed. I can add a comment about it to the code.

std::cerr << "Error: Only one filename allowed when using --wait." << std::endl;
return 1;
}

if(argc > 1 && (arg1 == "-w" || arg1 == "--wait")) {
if(argc > 3) {
fprintf(stderr, "Koder accepts only one filename when launching "
"in --wait mode.\n");
BMessage windowMessage;
if(waitForExit == true) {
windowMessage.what = WINDOW_NEW_WITH_QUIT_REPLY;
} else if(waitForExit == false && optind == argc) {
// no file arguments, open a blank window
windowMessage.what = WINDOW_NEW;
} else {
windowMessage.what = B_REFS_RECEIVED;
}

while(optind < argc) {
int32 line, column;
BPath filePath(ParseFileArgument(argv[optind++], &line, &column).c_str(), nullptr, true);
if(filePath.InitCheck() != B_OK) {
std::cerr << "Error: Invalid file path specified." << std::endl;
return 1;
}
BRoster roster;
team_id team = roster.TeamFor(gAppMime);
if(team == B_ERROR) {
BMessage* suppressMessage = new BMessage(SUPPRESS_INITIAL_WINDOW);
status_t status = roster.Launch(gAppMime, suppressMessage, &team);
delete suppressMessage;
if(status != B_OK && status != B_ALREADY_RUNNING) {
fprintf(stderr, "An issue occured while trying to launch Koder.\n");
return 1;
}

entry_ref ref;
BEntry entry(filePath.Path(), true);
if(entry.InitCheck() != B_OK || (entry.Exists() == true && entry.IsFile() == false)) {
std::cerr << "Error: Specified path is not a regular file." << std::endl;
return 1;
}
BMessage windowMessage(WINDOW_NEW_WITH_QUIT_REPLY);
// parse filename if any
// TODO: support -- for piping
if(argc > 2) {
int32 line, column;
std::string filename = ParseFileArgument(argv[2], &line, &column);
if(filename.find('/') != 0) {
BPath absolute(".", filename.c_str(), true);
filename = absolute.Path();
}
entry_ref ref;
BEntry(filename.c_str()).GetRef(&ref);
windowMessage.AddRef("refs", &ref);
if(line != -1) {
windowMessage.AddInt32("be:line", line);
}
if(column != -1) {
windowMessage.AddInt32("be:column", column);
}
if(entry.GetRef(&ref) != B_OK) {
std::cerr << "Error: Unable to get entry_ref for path." << std::endl;
return 1;
}
BMessenger messenger(gAppMime, team);
BMessage reply;
messenger.SendMessage(&windowMessage, &reply);
return 0;
} else {
App* app = new App();
app->Init();
app->Run();
delete app;
// always add column and line so that the count is the same as the number of refs
windowMessage.AddRef("refs", &ref);
windowMessage.AddInt32("be:column", column);
windowMessage.AddInt32("be:line", line);
}

return 0;
entry_ref appRef;
if(get_ref_for_path(argv[0], &appRef) != B_OK) {
std::cerr << "Error: Unable to determine Koder application path." << std::endl;
return 1;
}

BRoster roster;
team_id team;
const char* args[] = { "--launchkoderapp", nullptr };
// Use the entry_ref version of Launch() to make sure B_SINGLE_LAUNCH works correctly
status_t status = roster.Launch(&appRef, 1, args, &team);
Comment on lines +128 to +130
Copy link
Owner

@KapiX KapiX Jan 26, 2026

Choose a reason for hiding this comment

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

Suggested change
const char* args[] = { "--launchkoderapp", nullptr };
// Use the entry_ref version of Launch() to make sure B_SINGLE_LAUNCH works correctly
status_t status = roster.Launch(&appRef, 1, args, &team);
auto args = std::to_array<const char*>{ "--launchkoderapp", nullptr };
// Use the entry_ref version of Launch() to make sure B_SINGLE_LAUNCH works correctly
status_t status = roster.Launch(&appRef, args.size(), args.data(), &team);

if(status != B_OK && status != B_ALREADY_RUNNING) {
std::cerr << "Error: Unable to launch Koder." << std::endl;
return 1;
}

BMessenger messenger(gAppMime, team);
BMessage reply;
messenger.SendMessage(&windowMessage, &reply);

return 0;
}