Typing Tag in EditBox Crashing

[zhaosili]

Hi,
When Typing [window=''] or [font=''] in a EditBox, it may crash, since the window or the font does not exist.
If it doesn't occur, try to delete some of the string and retype.
Is this a bug? Or we should try-catch in our client code?
The demos shipped with 0.7.0 have the same problem.
Here is the callstack information taken from the VS debugger.

Code: Select all

>   CEGUIBase_d.dll!CEGUI::WindowManager::getWindow(const CEGUI::String & name={...})  Line 204 + 0xa6 bytes   C++
    CEGUIBase_d.dll!CEGUI::RenderedStringWidgetComponent::RenderedStringWidgetComponent(const CEGUI::String & widget_name={...})  Line 46 + 0x6a bytes   C++
    CEGUIBase_d.dll!CEGUI::BasicRenderedStringParser::processControlString(CEGUI::RenderedString & rs={...}, const CEGUI::String & ctrl_str={...})  Line 204 + 0x12 bytes   C++
    CEGUIBase_d.dll!CEGUI::BasicRenderedStringParser::parse(const CEGUI::String & input_string={...}, CEGUI::Font * initial_font=0x01ff6668, const CEGUI::ColourRect * initial_colours=0x00000000)  Line 116 + 0x1a bytes   C++
    CEGUIBase_d.dll!CEGUI::Window::getRenderedString()  Line 3478 + 0x4d bytes   C++
    CEGUIBase_d.dll!CEGUI::Window::bufferGeometry(const CEGUI::RenderingContext & __formal={...})  Line 1176   C++
    CEGUIBase_d.dll!CEGUI::Window::drawSelf(const CEGUI::RenderingContext & ctx={...})  Line 1157   C++
    CEGUIBase_d.dll!CEGUI::Window::render()  Line 1140 + 0x16 bytes   C++
    CEGUIBase_d.dll!CEGUI::Window::render()  Line 1145 + 0x19 bytes   C++
    CEGUIBase_d.dll!CEGUI::Window::render()  Line 1145 + 0x19 bytes   C++
    CEGUIBase_d.dll!CEGUI::System::renderGUI()  Line 417   C++



Code: Select all

/*************************************************************************
   Return a pointer to the named window
*************************************************************************/
Window* WindowManager::getWindow(const String& name) const
{
   WindowRegistry::const_iterator pos = d_windowRegistry.find(name);

   if (pos == d_windowRegistry.end())
   {
      throw UnknownObjectException("WindowManager::getWindow - A Window object with the name '" + name +"' does not exist within the system");
   }

   return pos->second;
}


[CrazyEddie]

Hi,

Thanks for (another!) report

This is confirmed as a bug; while the parsed content is never used (currently) in editbox widgets, we should not be parsing it in the first place. I'll have to do some work in this area because the current mechanisms are not good enough to allow a simple disabling of the parsing (you can force no parsing my assigning a custom DefaultRenderedStringParser, though this should not be forced on users) - a fix for this should go in tomorrow or Monday.

CE.

[CrazyEddie]

It's not tomorrow or Monday, but this is now fixed

I have added new functions, a property and an event related to setting the text parsing functionality to enabled or disabled. Editbox type widgets now have parsing disabled by default.

CE.

[zhaosili]

That's so cool, I'll try the version newly committed to SVN.

zhaosili

[zhaosili]

Hi,

I have tested the new version, it may still crash
if the text with tags in the EditBox is submitted to change other widget's Text Property.

E.g. I change the Sample_FalagardDemo1's layout with the following first:

Code: Select all

<?xml version="1.0" ?>
<GUILayout>
    <Window Type="Vanilla/FrameWindow" Name="Vanilla/Console">
        <Property Name="AlwaysOnTop" Value="True" />
        <Property Name="UnifiedMinSize" Value="{{0.2,0},{0.2,0}}" />
        <Property Name="UnifiedMaxSize" Value="{{0.8,0},{0.8,0}}" />
        <Property Name="UnifiedPosition" Value="{{0.5,0},{0.5,0}}" />
        <Property Name="UnifiedSize" Value="{{0.5,0},{0.45,0}}" />
        <Property Name="Text" Value="Console" />
        <Property Name="CloseButtonEnabled" Value="False" />

        <Window Type="Vanilla/Button" Name="Vanilla/Console/Submit">
            <Property Name="ID" Value="1" />
            <Property Name="VerticalAlignment" Value="Bottom" />
            <Property Name="HorizontalAlignment" Value="Right" />
            <Property Name="UnifiedMaxSize" Value="{{1,0},{1,0}}" />
            <Property Name="UnifiedPosition" Value="{{0,-7},{0,-7}}" />
            <Property Name="UnifiedSize" Value="{{0.25,0},{0,30}}" />
            <Property Name="Text" Value="Submit" />
        </Window>

        <Window Type="Vanilla/Editbox" Name="Vanilla/Console/Editbox">
            <Property Name="ID" Value="2" />
            <Property Name="VerticalAlignment" Value="Bottom" />
            <Property Name="TextParsingEnabled" Value="False" />
            <Property Name="UnifiedMaxSize" Value="{{1,0},{1,0}}" />
            <Property Name="UnifiedPosition" Value="{{0,7},{0,-7}}" />
            <Property Name="UnifiedSize" Value="{{0.75,-21},{0,30}}" />
            <Property Name="Text" Value="" />
        </Window>

        <!--Window Type="Vanilla/MultiLineEditbox" Name="Vanilla/Console/History"-->
   <Window Type="Vanilla/StaticText" Name="Vanilla/Console/History">
            <Property Name="ID" Value="3" />
            <Property Name="HorzFormatting" Value="WordWrapLeftAligned" />
            <Property Name="VertFormatting" Value="BottomAligned" />
            <Property Name="VertScrollbar" Value="True" />
            <Property Name="ReadOnly" Value="True" />
            <Property Name="UnifiedMaxSize" Value="{{1,0},{1,0}}" />
            <Property Name="UnifiedPosition" Value="{{0,7},{0,7}}" />
            <Property Name="UnifiedSize" Value="{{1,-14},{1,-47}}" />
            <Property Name="Text">CEGUI Demo Console - F12 toggles this window

It's now easy to add multiline text property.
Line 1
<![CDATA[
You can now put <xml>stuff</xml> in your property easly " " "
]]>
Line 2
Line 3
[image='set:BackgroundImage image:full_image']
</Property>
        </Window>
    </Window>
</GUILayout>



Then I type [font='']a or [window='']a and click the submit button, it crashes.

I think the solution is patching the constructor of RenderedStringTextComponent

Code: Select all

RenderedStringTextComponent::RenderedStringTextComponent(
        const String& text, const String& font_name) :
    d_text(text),
    d_font(font_name.empty() ? 0 : &FontManager::getSingleton().get(font_name)),
    d_colours(0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF)
{
}



and RenderedStringWidgetComponent with try-catch.

Code: Select all

RenderedStringWidgetComponent::RenderedStringWidgetComponent(
        const String& widget_name) :
    d_window(WindowManager::getSingleton().getWindow(widget_name))
{
}


Perhaps patches are also needed for the following:

Code: Select all

void RenderedStringWidgetComponent::setWindow(const String& widget_name)
{
    d_window = WindowManager::getSingleton().getWindow(widget_name);
}


Code: Select all

void RenderedStringTextComponent::setFont(const String& font_name)
{
    d_font =
        font_name.empty() ? 0 : &FontManager::getSingleton().get(font_name);
}



[CrazyEddie]

Hi,

Yes, it may still crash, but the key fact is that the issue is now that the string from the editbox is being passed to another component unchecked - this is the responsibility of the user of the library to do this. The demos should be updated in this regard in order to show good practice - whether that be by cleaning the entered strings or by simply adding a try/catch block somewhere (there are ways of forcing the immediate parse of the assigned text, so the user code is always able to do it's own validation via some means or other - thus preventing the throw from occurring later on in the renderGUI call).

I certainly don't agree that we should be protecting against this in the library itself - what happens in some theoretical scenario where the users code needs to hear about those exceptions for some reason? If we catch those internally we deny the user certain powers - there are lots of other places where we allow the user to potentially shoot themselves in the foot also

CE.

[zhaosili]

Hi,

Yeah, if the library itself protects the scenario, the user'll lose the power to check whether the text is valid.
But for users that are porting from 0.6.x, there maybe tens of dozens of setText or similar things in the code without try-catch for this.
E.g. In a listbox, typically it's normal for the users using previous verisons' CEGUI like this:

Code: Select all

try
{
    listbox->resetList();
    for(xxx;xxx;xxx)
    {
       listbox->addItem(xxxx)
     }
}
catch
{
}


If We get exception with the first item, the listbox'll be blank. Since for the new version ,we need to deal with the situation like this:

Code: Select all

    listbox->resetList();
    for(xxx;xxx;xxx)
    {
      try
      {
         listbox->addItem(xxxx)
      }
      catch()
      {
      }
    }


This change will cost a lot in Porting for a large project.

I wonder if the library could provide some interface to set GLOBAL string parser e.g. something like d_basicStringParser (currently, the d_customStringParser is just for one window instance), and this'll give user power to control whether the exceptions can be all ignored.

[CrazyEddie]

I wonder if the library could provide some interface to set GLOBAL string parser e.g. something like d_basicStringParser (currently, the d_customStringParser is just for one window instance), and this'll give user power to control whether the exceptions can be all ignored.

Yes, it should be possible to add this. The priorities used when choosing the parser to use will be as follows:

1) Window has parsing disabled - use built-in DefaultRenderedStringParser to render string verbatim (i.e no parsing).
2) Window has parsing enabled and custom parser set on Window - use the Window instance's custom parser.
3) Window has parsing enabled, no custom parser set on Window - use the globally set custom parser.
4) Window has parsing enabled, no custom parser set on Window, no global parser set - use the built-in BasicRenderedStringParser.

CE.

[zhaosili]

This feature is so cool that we can port code from 0.6.x to 0.7.x easily

Thanks CE.

zhaosili

[CrazyEddie]

Ok, this has now been added. You can set a system-wide custom parser via the a RSI inducing function named: System::setDefaultCustomRenderedStringParser and passing in a pointer to the desired parser.

HTH

CE.

[zhaosili]

Thank you very much, CE.
I'll try it.
BTW: I found the code of the ListboxTextItem and TreeItem also includes basicStringParser, does the global parser affect this?

[CrazyEddie]

The situation with ListboxTextItem and TreeItem is a bit sticky. Currently these are not affected by the global parser. What I really would like to do is have those item classes use a settable custom parser, but otherwise defer to the owning window (and so to the global parser where appropriate). The issue now is whether we consider this quite a major change in behaviour, and will 0.7.0 users be mad should the behaviour change when making an upgrade to 0.7.1? I'm very tempted to say f*** it and make the change (since it's the Right Thing™ to do for the lib).

CE.

[zhaosili]

Yeah, the problem is sticky. Currently I hack CEGUI with this:

Code: Select all

    //void parseTextString() const;
    //Hack
    virtual void parseTextString() const;


Then I use 'MyListBoxTextItem:public ListboxTextItem' and 'MyTreeItem:public TreeItem',
override the 'parseTextString()' method to work around.

[CrazyEddie]

I think it might be a good idea for us to add a new version of these item classes that behave the way we would now prefer, and mark the existing versions deprecated (or in the case of TreeItem, double-deprecated ). This way we can get the better, more consistent behaviour in for 0.7.1 while maintaining the previous implementation for those that might have started using it already.

CE.

[zhaosili]

I agree with you, that's a good idea for users who want to porting from 0.7.0 to 0.7.1.

zhaosili