Hello and note about Gui::getFont()

Started by Drifty Pine, 12 May 2020, 01:22:01

Drifty Pine

Hello Bruno!

I downloaded your code for the first time a few days ago to check it out. Very nice!
It was a snap to compile it using gcc 9.3 on MSYS running on Windows 10.
Also, from what I've seen so far, the code is pretty easy to follow.
Thank you so very much for creating and maintaining this library!

I did encounter an issue when checking out the ManyDifferentWidgets example program, and I thought you might want to know about it.

The ManyDifferentWidgets.cpp example has this:
        [line 240]      sf::Text text{"SFML Canvas", *gui.getFont(), 24};

However, at this point, gui.getFont() returns a std::shared_ptr containing nullptr.

Here is a very brief breakdown of why it happens. I'm writing this out to save you time and also in case I have made a mistake in following your code:

    The function Gui::getFont() returns a std::shared_ptr<sf::Font> that points to Widget::m_inheritedFont (a tgui::Font object).
    (Gui::m_container is a GuiContainer, which is a Container, which is a Widget, so getFont() calls Widget::getInheritedFont().)

   The problem is, Widget::m_inheritedFont is initially default-constructed and therefore its shared_ptr<sf::Font> is nullptr.

   Widget::m_inheritedFont is not otherwise set unless Widget::setInheritedFont() is explicitly called, and this is only done via Gui::setFont() or Widget::rendererChanged() called for a "font" property.   Neither of these functions are called if a Gui object is used in the seemingly usual way as the master widget container/manager/controller.

Since gui.getFont() returns a shared_ptr containing nullptr, de-referencing it is undefined behavior. In case you are wondering...nothing bad happens in the example app because the sf::Text constructor takes sf::Font const& and just stores the address of the reference (that will be nullptr in this case). Later, sf::Text::draw() checks that the address of the font and only does something if the address is not nullptr.
So, the text "SFML Canvas" is never put on the canvas, but no exception or crash ever occurs.

In looking around your site, I noticed that the TGUI 0.6 intro tutorial mentioned that Gui::setGlobalFont() should be called.
It seems by version 0.7 this was changed, and in version 0.8 there is a separate tgui::getGlobalFont() that returns a tgui::Font.
[This function builds an sf:Font based on the DejaVuSans TTF from a static in-memory array if no font was set before it is called.]

So, it might be a good idea to change the example to this:
   [line 240]      sf::Text text{"SFML Canvas", *tgui::getGlobalFont().getFont(), 24};

However, if your intention is to always have some font available "by default" when Gui::getFont() is called (and I like that idea), may I suggest a very minor change to Gui::getFont() so it does this:
std::shared_ptr<sf::Font> Gui::getFont() const{
   if (m_container->getInheritedFont())
      return m_container->getInheritedFont();
   return getGlobalFont();

Here are three other obvious possible remedies, but I don't like them:

  • Initialize Widget::m_inheritedFont via getGlobalFont(), just like is done for Widget::m_fontCached.
    This is not so good because it circumvents having Widget::m_inheritedFont as nullptr used to indicate "no inherited font".

  • GuiContainer's constructor could call Widget::setInheritedFont().
    This seems problematic because that will trigger a call to Widget::rendererChanged().

  • GuiContainer's constructor could do `m_inheritedFont = getGlobalFont()`.
    This seems brittle; directly setting a parent class's protected member directly violates encapsulation.
Again, many thanks for the library!


Thanks for looking into this in such detail.
It does look like this is just a case of the example not being updated with the rest of the code. The correct way is indeed *tgui::getGlobalFont().getFont().

The behavior shouldn't be changed in 0.8 as it appears to be the "intended" behavior: https://github.com/texus/TGUI/blob/0.8/tests/Container.cpp#L46
So for 0.8 I'll just have to fix the example code.

For 0.9 I will follow your suggestion to call getGlobalFont inside Gui::getFont. It does seem to be the best option.