Factor out the routine for monitor resolution query, bring up GLFW RAII

[hujiajie]

In particular, the design takes in mind GLFW invocations even before main().

[shaoboyan]

@gyagp Not see your comments, will you add your input?

[shaoboyan]

If stick on a specific monitor will help simplify the code, then do it!(And could provide info/comments to describe this restriction)

[shaoboyan]

Can we cache them? (Assume call glfwGetPrimaryMonitor for the assumption that the user have multiple monitors?)

[hujiajie]

This was one of my initial candidate ideas, and I happened to choose the other considering it's only called during initialization. Personally I'd like to avoid introducing too many static variables in the code base, but let's see how others feel about this.

[shaoboyan]

Ditto. It's a getter, concerns for so many glfw calls

[gyagp]

@hujiajie, can you upload and point me the changes to option parser? I'd like to understand more about the motivation for this patch.

[hujiajie]

The start point is that abseil-cpp has become a hard dependency of googletest, and it provides an option parser, making cxxopts redundant.

The typical usage of the Abseil flags library looks like the following:

ABSL_FLAG(std::string, window_size, DEFAULT_VALUE, "help message");
// Approximately equivalent with:
// std::string FLAGS_window_size = DEFAULT_VALUE;

int main(int argc, char *argv[]) {
  absl::ParseCommandLine(argc, argv);
  std::string foobar = absl::GetFlag(FLAGS_window_size);
  // ...
  return 0;
}

The question is how to set DEFAULT_VALUE. One idea is to fill with an "invalid" value like "2147483647,2147483647". When Aquarium encounters this "invalid" value, a window is created using the screen size. But in my opinion Aquarium should never care the valid range of window size. It should pass the size to GLFW as is, and let GLFW / the underlying window system determine whether the value is valid. So here's my proposal to avoid the hardcoded magic number:

ABSL_FLAG(std::string,
          window_size,
          std::to_string(getMonitorResolution().width) + "," + std::to_string(getMonitorResolution().height),
          "help message");

Note getMonitorResolution() might be called during the global initialization phase before main(), so internally the function tries to initialize GLFW on demand.

[hujiajie]

Full PoC here: https://github.com/hujiajie/aquarium/tree/abseil-cpp

[gyagp]

My most concerns come from the following points:

• I don't like global things, and I hope we can avoid them in total.
• Most of time, I just hope to understand the basic usage of Aquarium, why should a Window System be instantiated? I prefer to use 0,0 or -1,-1 to represent the default screen size. It's overkill to have a complex design just for a default value in help.
• The life cycle of glfw can be similar as context. We don't need a counter (static variable) to track, and we may not even need an extra class for it. It's also weird for me that GLFW helper class is a nested class of Context.

And in your ABSEIL POC, I also don't think the option parser is a part of Context. Option parser belongs to App (Aquarium in our case), and it parses the options and passes them to Context.

[hujiajie]

My most concerns come from the following points:

• I don't like global things, and I hope we can avoid them in total.

I don't like either, but unfortunately it's the standard usage of the Abseil flags library. Putting ABSL_FLAG() in a function may still work, but there's no guarantee.

• Most of time, I just hope to understand the basic usage of Aquarium, why should a Window System be instantiated? I prefer to use 0,0 or -1,-1 to represent the default screen size. It's overkill to have a complex design just for a default value in help.

So call for voting here: a simple command-line user interface (no special case like 0,0) + a complicated implementation, or a complicated command-line user interface + a simple implementation?

• The life cycle of glfw can be similar as context. We don't need a counter (static variable) to track, and we may not even need an extra class for it. It's also weird for me that GLFW helper class is a nested class of Context.

And in your ABSEIL POC, I also don't think the option parser is a part of Context. Option parser belongs to App (Aquarium in our case), and it parses the options and passes them to Context.

This is caused by another strange design of the Abseil flags library. To illustrate the auto-generated --help output:

  Flags from FILE1:
    --foo ...
    --bar ...

  Flags from FILE2:
    --baz ...

The Flags from ... line looks meaningless if there's only one centralized file, but perhaps better for multiple files. Without a new class, Context* is the most suitable place so far.