cpp-best-practices/cmake_template

Feedback about the template.

ArthurSonzogni opened this issue · 1 comments

Hello,

I just tried the template. Things are working correctly!

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

  • I installed conan and got weird errors. They were really not easy to diagnose. The fix was to make python3 the default alternative instead of python2.7.
  • clang-tidy is a "required-dependency". It might worth mentioning it in: README_dependencies.md or making it work without it.

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.

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