wxGTK: wxFont::SetFaceName() resets the font size

[golly]

This doesn't happen in wxGTK 2.6.3.2.

1. Compile XRC sample.
2. Start the sample and click on the "controls example" button (3rd from left at time of writing).
3. Select the "wx*PickerCtrl" tab (you will have to move right to find it).
4. Look at "Font Picker" at the bottom of the dialog - notice it is nice and large.
5. Exit demo.

6. Open rc/control.xrc in text editor.
7. Search for "sysfont". This should take you to the XRC for the font picker control.
8. Add <face>Arial</face> inside the <font> tags.
9. Start demo and follow steps 1-4 again. Font Picker control is not the same size as before.

[vadz]

This is not XRC-specific, in fact, and can be seen with the following patch to the minimal sample:

Index: samples/minimal/minimal.cpp
===================================================================
--- samples/minimal/minimal.cpp (revision 60362)
+++ samples/minimal/minimal.cpp (working copy)
@@ -68,6 +68,16 @@
     void OnQuit(wxCommandEvent& event);
     void OnAbout(wxCommandEvent& event);

+    void OnPaint(wxPaintEvent&)
+    {
+        wxPaintDC dc(this);
+        wxFont font(*wxNORMAL_FONT);
+        font.SetPointSize(20);
+        font.SetFaceName("Arial");
+        dc.SetFont(font);
+        dc.DrawText("Hello, bug!", 10, 10);
+    }
+
 private:
     // any class wishing to process wxWidgets events must use this macro
     DECLARE_EVENT_TABLE()
@@ -99,6 +109,7 @@
 BEGIN_EVENT_TABLE(MyFrame, wxFrame)
     EVT_MENU(Minimal_Quit,  MyFrame::OnQuit)
     EVT_MENU(Minimal_About, MyFrame::OnAbout)
+    EVT_PAINT(MyFrame::OnPaint)
 END_EVENT_TABLE()

 // Create a new application object: this macro will allow wxWidgets to create

The font is normal-sized instead of 20pt because calling SetFaceName() apparently resets all other font attributes.

Francesco, as you've just looked at this code could you please check if this is easy to fix? TIA!

[FM]

(In [60392]) mention that calling SetFaceName() with an invalid facename will invalidate the font object (see #10475)

[frm]

There is no "real" bug IMO.

In the minimal sample modification posted above, what happens is that on wxGTK, where "Arial" is typically an invalid facename, SetFaceName() will call wxFontBase::SetFaceName() (this happens on both wxGTK and wxMSW btw) and that function will invalidate the font object (calling UnRef()).

wxPaintDC::SetFont seems to accept both valid and invalid fonts on both wxMSW and wxGTK (at least we are coherent in the different implementations here :)) and the result is that the code doesn't trigger any assert/debug message and that dcPaintDC::DrawText() simply draws the text with the default font instead.

Now, we could:
a) make wxFontBase::SetFaceName() not invalidate the font object when an invalid facename is passed; however this could silently break code relying on it;
b) make wxDC::SetFont() assert if an invalid font object is passed

IMHO b) is better because the fact that wxDC::SetFont(anInvalidFont) silently resets the dc font doesn't seem right to me.
IIRC almost all functions that use GDI objects assert if the passed object is invalid...

[vadz]

Oops, sorry for the wrong analysis, I didn't know that this face name was invalid. I wonder what happens with the right name though, is the font size preserved (IMO it should be)?

Anyhow, I think there is nothing to do concerning SetFaceName() other than documenting this behaviour as you already did (thanks!), asserting is probably too much. I still wonder why did this work in 2.6 and doesn't work now though.

[FM]

(In [60412]) correct SetFont() documentation: at least wxMSW and wxGTK do allow the user to pass wxNullFont (or another invalid font instance) (see #10475)

[frm]

Replying to vadz:

Oops, sorry for the wrong analysis, I didn't know that this face name was invalid. I wonder what happens with the right name though, is the font size preserved (IMO it should be)?

yes, it is (I've tested this with the "Sans" facename which is installed by default on Ubuntu).

Anyhow, I think there is nothing to do concerning SetFaceName() other than documenting this behaviour as you already did (thanks!), asserting is probably too much.

ok, btw I verified that at least also wxDC::SetBrush and wxDC::SetPen are documented to allow wxNullBrush/wxNullPen as arguments, so I've updated the docs also for wxDC::SetFont.

[golly]

Sorry, just saw your message thread. I hope you don't mind me updating a closed ticket.

Although I understand why you do not want to assert if SetFontFace() is passed an invalid font face, I would suggest at least a trace message on invalid fonts != wxNullFont as it took me a good deal of time to find this (climbing through XRC and all). A nice little debug message along the lines of "you passed an invalid font that was not wxNullFont" would have alerted me very quickly to the fact that my XRC was bogus.

Just my 2 cents worth.

[frm]

Replying to golly:

Sorry, just saw your message thread. I hope you don't mind me updating a closed ticket.

no problem

Although I understand why you do not want to assert if SetFontFace() is passed an invalid font face, I would suggest at least a trace message on invalid fonts != wxNullFont as it took me a good deal of time to find this (climbing through XRC and all).

Yes, I know this kind of things are difficult to trace/debug if no warnings/errors are shown. However since invalid fonts are all equals to wxNullFont (they have NULL pointers for their m_refData pointers and thus there's nothing to compare for them besides the pointer itself...), we cannot make SetFontFace() output a msg when an invalid font != wxNullFont is passed :/

A nice little debug message along the lines of "you passed an invalid font that was not wxNullFont" would have alerted me very quickly to the fact that my XRC was bogus.

In fact I think that it should be XRC (in particular wxXmlResourceHandler::GetFont in this case) to warn if an invalid/unsupported parameter was present in the XRC.

I think something like:

Index: src/xrc/xmlres.cpp
===================================================================
--- src/xrc/xmlres.cpp  (revisione 60456)
+++ src/xrc/xmlres.cpp  (copia locale)
@@ -1590,6 +1590,8 @@
             iweight = wxBOLD;
         else if (weight == wxT("light"))
             iweight = wxLIGHT;
+        else
+            wxLogTrace("XRC", "Invalid font weight");
     }

     // underline

.... lots of other wxLogTraces to be added for all other font parameters....

@@ -1633,6 +1635,9 @@
         if (tk.HasMoreTokens())
             facename = tk.GetNextToken();
 #endif // wxUSE_FONTENUM/!wxUSE_FONTENUM
+
+        if (facename.IsEmpty())
+            wxLogTrace("XRC", "Invalid font facename");
     }

     // encoding

would be fine (provided the "XRC" string is #defined in xmlres.h and used in all wxLogTrace calls of xmlres.cpp). Of course it would need to be done also in others wxXmlResourceHandler::GetXXXX() helpers whenever something invalid is passed...

Vaclav, do you think this would be a good idea?
If so, I think a patch doing the above would be very welcome :)