[Solved] Combo box - Mouse cursor handling - Taharez

[Bertram]

Hey there,
I hope you guys are all doing fine.

Just for the record, we have been using CEGUI for OpenDungeons and successfully ported the GUI to custom graphics and it looks shiny. Thanks for all the help you've been given us and for the GUI, really.

Back on topic, here is a small question following a observation made by Akien here:
https://github.com/OpenDungeons/OpenDungeons/issues/462

I wanted to first discuss that before either opening an issue or have a go at some merge request or something.

It seems the combo Edit Box in the TaharezLook look'n'feel type will use a MouseCursorImage parameter to know what mouse cursor to display when hovering the edit box part.
This part is working fine and well. It's just we are using a text caret mouse cursor when hovering such a widget, following the examples, but using this cursor doesn't make sense when the combo box is in read-only mode.

On our side, we will work around that by using the default mouse cursor, but shouldn't there be a ReadOnlyMouseCursorImage parameter for such a case?

Best regards,

[Ident]

How do you switch to read-only mode? Can you tell us the function you call or the state you change? We need to be precise here.

My first idea is that this can easily be solved by editing the LNF. But I need to know the above first to be certain.

[Bertram]

Hey Ident,

Here is the LNF part of the combo box:
(OD/* widgets are custom skinned widgets based their CEGUI/* counterparts.)

Code: Select all

    <WidgetLook name="OD/ComboEditbox">
        <PropertyDefinition initialValue="FFFFFFFF" name="NormalTextColour" redrawOnWrite="true" />
        <PropertyDefinition initialValue="FF000000" name="SelectedTextColour" redrawOnWrite="true" />
        <PropertyDefinition initialValue="FF607FFF" name="ActiveSelectionColour" redrawOnWrite="true" />
        <PropertyDefinition initialValue="FF808080" name="InactiveSelectionColour" redrawOnWrite="true" />
        <Property name="MouseCursorImage" value="OpenDungeonsSkin/MouseTextBar" />
        <NamedArea name="TextArea">
            <Area>
                <Dim type="LeftEdge"><AbsoluteDim value="5" /></Dim>
                <Dim type="TopEdge"><AbsoluteDim value="5" /></Dim>
                <Dim type="RightEdge"><UnifiedDim offset="-5" scale="1.0" type="RightEdge" /></Dim>
                <Dim type="BottomEdge"><UnifiedDim offset="-5" scale="1.0" type="BottomEdge" /></Dim>
            </Area>
        </NamedArea>
        <ImagerySection name="container_normal">
            <FrameComponent>
                <Area>
                    <Dim type="LeftEdge"><AbsoluteDim value="0" /></Dim>
                    <Dim type="TopEdge"><AbsoluteDim value="0" /></Dim>
                    <Dim type="Width"><UnifiedDim scale="1.0" type="Width" /></Dim>
                    <Dim type="Height"><UnifiedDim scale="1.0" type="Height" /></Dim>
                </Area>
                <Image component="LeftEdge" name="OpenDungeonsSkin/ComboboxEditLeft" />
                <Image component="Background" name="OpenDungeonsSkin/ComboboxEditMiddle" />
            </FrameComponent>
        </ImagerySection>
        <ImagerySection name="selection">
            <ImageryComponent>
                <Area>
                    <Dim type="LeftEdge"><AbsoluteDim value="0" /></Dim>
                    <Dim type="TopEdge"><AbsoluteDim value="0" /></Dim>
                    <Dim type="RightEdge"><UnifiedDim scale="1.0" type="RightEdge" /></Dim>
                    <Dim type="BottomEdge"><UnifiedDim scale="1.0" type="BottomEdge" /></Dim>
                </Area>
                <Image name="OpenDungeonsSkin/SelectionBrush" />
                <VertFormat type="Stretched" />
                <HorzFormat type="Stretched" />
            </ImageryComponent>
        </ImagerySection>
        <ImagerySection name="Caret">
            <ImageryComponent>
                <Area>
                    <Dim type="LeftEdge"><AbsoluteDim value="0" /></Dim>
                    <Dim type="TopEdge"><AbsoluteDim value="0" /></Dim>
                    <Dim type="Width"><ImageDim dimension="Width" name="OpenDungeonsSkin/EditBoxCaret" /></Dim>
                    <Dim type="BottomEdge"><UnifiedDim scale="1.0" type="BottomEdge" /></Dim>
                </Area>
                <Image name="OpenDungeonsSkin/EditBoxCaret" />
                <VertFormat type="Stretched" />
                <HorzFormat type="Stretched" />
            </ImageryComponent>
        </ImagerySection>
        <StateImagery name="Enabled">
            <Layer>
                <Section section="container_normal" />
            </Layer>
        </StateImagery>
        <StateImagery name="ReadOnly">
            <Layer>
                <Section section="container_normal" />
            </Layer>
        </StateImagery>
        <StateImagery name="Disabled">
            <Layer>
                <Section section="container_normal">
                    <Colours bottomLeft="FF7F7F7F" bottomRight="FF7F7F7F" topLeft="FF7F7F7F" topRight="FF7F7F7F" />
                </Section>
            </Layer>
        </StateImagery>
        <StateImagery name="ActiveSelection">
            <Layer>
                <Section section="selection">
                    <ColourProperty name="ActiveSelectionColour" />
                </Section>
            </Layer>
        </StateImagery>
        <StateImagery name="InactiveSelection">
            <Layer>
                <Section section="selection">
                    <ColourProperty name="InactiveSelectionColour" />
                </Section>
            </Layer>
        </StateImagery>
    </WidgetLook>

    <WidgetLook name="OD/Combobox">
        <PropertyLinkDefinition initialValue="FFFFFFFF" name="NormalEditTextColour" targetProperty="NormalTextColour" widget="__auto_editbox__" />
        <PropertyLinkDefinition initialValue="FF000000" name="SelectedEditTextColour" targetProperty="SelectedTextColour" widget="__auto_editbox__" />
        <PropertyLinkDefinition initialValue="FF6060FF" name="ActiveEditSelectionColour" targetProperty="ActiveSelectionColour" widget="__auto_editbox__" />
        <PropertyLinkDefinition initialValue="FF808080" name="InactiveEditSelectionColour" targetProperty="InactiveSelectionColour" widget="__auto_editbox__" />
        <Child nameSuffix="__auto_editbox__" type="OD/ComboEditbox">
            <Area>
                <Dim type="LeftEdge"><AbsoluteDim value="0" /></Dim>
                <Dim type="TopEdge"><AbsoluteDim value="0" /></Dim>
                <Dim type="RightEdge">
                    <OperatorDim op="Subtract">
                        <UnifiedDim scale="1" type="RightEdge" />
                        <OperatorDim op="Multiply">
                            <FontDim type="LineSpacing" />
                            <AbsoluteDim value="1.5" />
                        </OperatorDim>
                    </OperatorDim>
                </Dim>
                <Dim type="Height">
                    <OperatorDim op="Multiply">
                        <FontDim type="LineSpacing" />
                        <AbsoluteDim value="1.5" />
                    </OperatorDim>
                </Dim>
            </Area>
        </Child>
        <Child nameSuffix="__auto_droplist__" type="OD/ComboDropList">
            <Area>
                <Dim type="LeftEdge"><AbsoluteDim value="0" /></Dim>
                <Dim type="TopEdge"><WidgetDim dimension="BottomEdge" widget="__auto_editbox__" /></Dim>
                <Dim type="Width"><UnifiedDim scale="1" type="Width" /></Dim>
                <Dim type="BottomEdge"><UnifiedDim scale="1" type="BottomEdge" /></Dim>
            </Area>
        </Child>
        <Child nameSuffix="__auto_button__" type="OD/Button">
            <Area>
                <Dim type="LeftEdge"><AbsoluteDim value="0" /></Dim>
                <Dim type="TopEdge"><AbsoluteDim value="0" /></Dim>
                <Dim type="Width"><WidgetDim dimension="Height" widget="__auto_editbox__" /></Dim>
                <Dim type="Height"><WidgetDim dimension="Height" widget="__auto_editbox__" /></Dim>
            </Area>
            <HorzAlignment type="RightAligned" />
            <Property name="NormalImage" value="OpenDungeonsSkin/ComboboxListButtonNormal" />
            <Property name="HoverImage" value="OpenDungeonsSkin/ComboboxListButtonHover" />
            <Property name="PushedImage" value="OpenDungeonsSkin/ComboboxListButtonNormal" />
        </Child>
        <StateImagery name="Enabled" />
        <StateImagery name="Disabled" />
    </WidgetLook>

And here is a sample of a read-only Combobox in the concerned layout file:

Code: Select all

        <Window type="OD/Combobox" name="Combobox" >
            <Property name="Area" value="{{0,171.5},{0,122},{0,322},{0,184.5}}" />
            <Property name="Text" value="ComboBox" />
            <Property name="ReadOnly" value="True" />
        </Window>

Again, the combox is working fine, except for the cursor.

Is there something I'm missing?

Best regards,

[Ident]

Again: How do you switch to, as what you refer to as "read-only mode"?

[Bertram]

Hey,

With this parameter in the combox layout set to True:

Code: Select all

            <Property name="ReadOnly" value="True" />

Best regards,

[Ident]

I tried it out, this is indeed stupid.

The problem is not in the Combobox but in the Editbox. The Editbox is a child widget of the Combobox widget.

The Editbox defines a unique mouse cursor to be used for the widget. This cursor is never changed. I checked and from what I see there is no way to change the value of the CursorImage (or any other Property) from within the LNF upon switching the StateImagery to another one (as is the case with ReadOnly).

To achieve what you want, the Editbox class (widgets/Editbox) would have to be edited. Also an additional property would have to be added to the LNF to save the other Cursor. The editing of Editbox is simple: in void Editbox::setReadOnly(bool setting) you would have to add at the end:

Code: Select all

if(d_readOnly)
    this->setProperty("CursorImage", ""); //This changes to default (as far as i remember). If we want another cursor we need two define two PropertyDefinitions
else
    this->setProperty("CursorImage", getProperty("EditableCursorImage"));


For the Editbox LNF in Taharez you would have to add a PropertyDefinition:

Code: Select all

<PropertyDefinition name="EditableCursorImage" initialValue="WhateverSet/WhateverImage" redrawOnWrite="true" type="Image"/>

This is off the top of my head, didn't try it out. Please try it out if you want.

[Bertram]

I'll sure do!! Thanks

If I can manage to make your own example work, will you burn me alive if I create a merge request for 1.0?

Plus, there is another similar (but still small) issue I'd like to discuss, but one topic at a time.

EDIT: To stay backward compatible, I'd rather create a "NonEditableCursorImage" parameter as the default cursor is when the widget is not read only. just tell me if it's ok for you.

Best regards,

[Ident]

Please check first if my suggestion works. Maybe it doesn't. Once we know what works we can discuss details and how we can add the feature to CEGUI and also we can talk about the other issue you just mentioned

[Bertram]

Hey

I don't know whether it's a suitable commit (while I tried to mimic) but this is working:
https://bitbucket.org/Bertram25/cegui/c ... at=default

Best regards,

[Ident]

1. The changes to Editbox.cpp seem good overall, except for one thing: You change new lines all over the place as you can see in the diff. We cannot accept commit like this, maybe you should activate the EOL package of HG/Mercurial. Are you changing the new lines to the format of windows-style new lines?The correct line endings are UNIX line endings for the repository. The EOL package should take care of that for you. Convert the changed files line endings to UNIX line endings with notepad++ or similar.
2. You add a new member variable in Editbox.h. Is this necessary? If yes explain why. You already store the value in the property so why not just use that in the function where readonly is changed using getProperty?
3. You would probably want to add the Property to the Taharez LNF and see if it works

If you took the idea of adding a member from another class please notify me where you saw this. I am aware that FalagardImage does this and this is a bug that will be resolved in 1.0.



Aside from these (minor) issues it looks good. I am very glad this works! If you follow my suggestions and we discuss the rest that is in question, we can definitely accept this as a PR soon.

[Bertram]

Hey Ident,

1. The changes to Editbox.cpp seem good overall, except for one thing: You change new lines all over the place as you can see in the diff. We cannot accept commit like this, maybe you should activate the EOL package of HG/Mercurial. Are you changing the new lines to the format of windows-style new lines?The correct line endings are UNIX line endings for the repository. The EOL package should take care of that for you. Convert the changed files line endings to UNIX line endings with notepad++ or similar.

This one is simply my bad, Kate had trailing whitespace removal option on, as this is a test commit for discussion so I didn't care removing it. I'll definitely use linux line endings as I did the patch on Debian, and won't remove the trailing spaces in the final commit.

2. You add a new member variable in Editbox.h. Is this necessary? If yes explain why. You already store the value in the property so why not just use that in the function where readonly is changed using getProperty?

I first had to define a ReadOnlyMouseCursorImage property using the CEGUI_DEFINE_PROPERTY(...) macro because the getProperty("ReadOnlyMouseCursorImage") was triggering an exception at runtime in Editbox::setReadOnly().

This macro is requiring a setter and a getter or NULL/0 when none. I first set both to 0, but then, at runtime, when starting the game using the newly compiled CEGUI version, an exception was triggered because the "ReadOnlyMouseCursorImage" property in the LNF wasn't writable.
I thus added that member and made it so the value is stored and it went fine as is. I don't really have a clue of what I should do now, though.

3. You would probably want to add the Property to the Taharez LNF and see if it works

I did, and tested live in game. It is working. Or is the LNF something else than the xml file?

If you took the idea of adding a member from another class please notify me where you saw this. I am aware that FalagardImage does this and this is a bug that will be resolved in 1.0.

I took the inspiration from Editbox::get/setValidationString() as it was behaving quite the same.

Aside from these (minor) issues it looks good. I am very glad this works! If you follow my suggestions and we discuss the rest that is in question, we can definitely accept this as a PR soon.

Sure. I just wish I had done hg merge before starting my commits. But the lesson is learned. I'll recreate a proper commit once we have sorted out all the details.

Best regards,

[Ident]

Hey,

This is actually a by far tougher change than I thought it was. I am, however, entirely sure that this is worth it. The caret bothers me since you brought it up. It is like a painting that is hanging slightly rotated and once someone has pointed it out to you, you can't unsee it And in that case, what you want to do is fix it, so it doesnt bother you anymore. So lets try to accomplish that.

I am trying to find an existing example of similar usage so that we can follow it. So far what I found out is that these properties are all linked to actual values inside classes so I was wrong, you would have to add a member. Unless you used a PropertyDefinition. For that case,however, the performance is not as good and the access is indirect plus you cannot be sure inside the class that the LNF defines this, whereas the macro defines it directly from within the class- an arguably better solution. Now here comes the bummer: We can't add new members to classes in v0-8, this would break ABI compatibility. We can accept your changes for v0 though. which is the 0.9 Releases.

Here is my findings regarding the property stuff:
CEGUI_DEFINE_WINDOW_RENDERER_PROPERTY is indeed what we need
the first parameters for it are obvious, interesting are the last 3: setter, getter and default value. The default value for us can be "" afaik. The getter and setter can theoretically be left 0, it is also possibly to just have one of them be 0. In our case we want to set both though and we actually do wanna store the variable in a local variable like you did, i was wrong about what I said there, sorry for the confusion.

I thus added that member and made it so the value is stored and it went fine as is. I don't really have a clue of what I should do now, though.

It is fine, you did it right.

I did, and tested live in game. It is working. Or is the LNF something else than the xml file?

I meant as a <Property...> and see if it actually updates to the right cursor set in it, in read-only mode, and also see if it switches back. If your code works like that it is fine.

If you took the idea of adding a member from another class please notify me where you saw this. I am aware that FalagardImage does this and this is a bug that will be resolved in 1.0.

I was wrong on this. While it is possible to replace some of the variables with property-only definitions, it doesnt make sense in any of the cases. The status-quo is fine. I will have to look into the StaticImage Image property though because I know we now have pure-LNF images and they seem to do fine.

Very important: if you make a PR it has to be based off the v0 branch. Changes between v0 and v0-8 are however minimal. They are compatible overall and all your function calls are equivalent. Only default branch will break anything so updating to v0 should pose no issue whatsoever.

[Ident]

I also commented on your commit. I will copy-paste it here for future reference:

So if it is read-only you will use the default mouse cursor of the CEGUI GUIContext but what if somebody wants a custom editbox cursor AND a custom editbox-readonly-cursor. Right now you can only have a readonly-cursor or the default cursor for a CEGUI GUIContext will be used.

Since we have only one effective value out of the box (CursorImage) and we need two switchable values (specific ReadOnly CursorImage and specific Non-ReadOnly CursorImage), you will have to add a second member variable for the Editbox. This means that CursorImage will not be a variable that should be set, it would only be effective until a user switches the readonly mode. This would have to be documented very very clearly otherwise there will be a forum outrage. I remember it is also possible to make the property readonly but i am not sure this is possible in this case, i dont think this is the case for which that was intended.

[Bertram]

Hey Ident,

I just wanted to say I'm not forgetting you.

I'm currently finishing a few other stuff and I'll then be back on the version 2.0 of the patch.

Best regards,

[Ident]

Thanks for the update. I am looking forward to your patch