cpp-best-practices / cmake_template

CMake for C++ Best Practices
The Unlicense
1.07k stars 116 forks source link

Feedback about the template. #5

Closed ArthurSonzogni closed 2 years ago

ArthurSonzogni commented 2 years ago

Hello,

I just tried the template. Things are working correctly!

I only had two issues build. It blocked me 10min.

Some comments:

  1. Bitmap::Render writes into the [0, width-1] x [0, height-1] box. However you might get an area smaller than what was asked in ComputeRequirement(). This would normally result in out of bounds errors. Fortunately, FTXUI has the concept of a "stencil", inhibiting writing outside of the box assigned to you: https://github.com/ArthurSonzogni/FTXUI/blob/548fa51b7115551b42c0f12235de9eb79e7e33e3/src/ftxui/screen/screen.cpp#L408-L413 So this works, despite this. Not sure if you knew it.

  2. I see Bitmap::SetBox overrides Node::SetBox, but provide the same implementation has the default one. You might want to remove this line if this wasn't added purposefully to help people to learn it.

  3. spdlog is included, but I don't see any usage. I am not sure how this will interact with FTXUI, if used.

  4. You can replace:

    auto container = ftxui::Container::Horizontal(all_buttons);
    
    auto renderer = ftxui::Renderer(container, make_layout);

    by

    auto renderer = Renderer(make_layout);

    The Renderer function "decorates" a component, by replacing it's "Render()" function implementation, by the one provided. This is a alternative to using c++ composition/inheritance, that is sometimes quicker to write. In your case, the decorated component doesn't do much. So the second version taking only the function rendering the interface can be used.

lefticus commented 2 years ago

Thank you for the insight @ArthurSonzogni !

  1. I guess I'll leave it how it is, since it's safe at least for now, and I'm not sure how I would deal with it otherwise. (scaling?)
  2. Fixed
  3. Very good point, which I wondered about. I'll leave it for now, because developers can still sink to files if they choose to, or sink to a window inside of FTXUI
  4. Fixed - I need a better understanding of how buttons and button order interacts with layout.

See: #8