Opened 7 weeks ago

Closed 7 weeks ago

Last modified 6 weeks ago

#16401 closed defect (fixed)

No checking of GetGrid() result value in wxPGProperty class

Reported by: anhsoft Owned by: AW
Priority: normal Milestone:
Component: wxPropertyGrid Version: dev-latest
Keywords: InsertChoice, DeleteChoice Cc:
Blocked By: Blocking:
Patch: yes

Description

If I created a property object (eg wxEnumProperty), but not yet appended it to grid do calling the InsertChoice() or DeleteChoice(), I get this issue, because inside these members don't checking the GetGrid() result value, which surely equal NULL.
The same issue has other functions: GetDisplayInfo, GetValueAsString, SetValueInEvent, GetCell, SetValueImage, GetY, GetItemAtY, which easy to find by 'GetGrid()' keyword.
The attached patch may fix this issue?

Attachments (1)

wxpgproperty.patch download (712 bytes) - added by anhsoft 7 weeks ago.

Download all attachments as: .zip

Change History (18)

Changed 7 weeks ago by anhsoft

comment:1 follow-up: Changed 7 weeks ago by vadz

Do things work correctly with this fix, i.e. are the choices inserted/deleted before attaching to the grid work correctly once the grid is shown? If so, I'd say we should apply it.

For the other methods, it probably doesn't make sense to call them before attaching, so we should add wxCHECK_RET(GetGrid()) there instead.

comment:2 follow-up: Changed 7 weeks ago by anhsoft

This work correctly.

PS: However using AddChoice/InsertChoice methods produce unpleasant effect unlike wxPGChoices is used. This is - appearance empty ComboBox text view when it gets a focus on mouse click. And it is not associated with the topic problem.

comment:3 in reply to: ↑ 1 Changed 7 weeks ago by awi

Replying to vadz:

Do things work correctly with this fix, i.e. are the choices inserted/deleted before attaching to the grid work correctly once the grid is shown? If so, I'd say we should apply it.

For the other methods, it probably doesn't make sense to call them before attaching, so we should add wxCHECK_RET(GetGrid()) there instead.

It seems that inserting/deleting choices to/from detached property object has no side effects and propsed fixes can be applied. Part of the code executed if PG exists is responsible for updating associated editor only and doesn't modify the state of the property itself.

Assertions can be implemented in the other functions but most (if not all) of these functions are used for internal purposes only (mainly by renderer) and generally PG is validated on the higher level. But the truth is that all these functions are exposed via the public interface and theoretically can be accessed from the user code so maybe inserting assertions makes sense.

comment:4 in reply to: ↑ 2 Changed 7 weeks ago by awi

Replying to anhsoft:

This work correctly.

PS: However using AddChoice/InsertChoice methods produce unpleasant effect unlike wxPGChoices is used. This is - appearance empty ComboBox text view when it gets a focus on mouse click. And it is not associated with the topic problem.

I can confitm this issue. Here is the test case:

  • samples/propgrid/propgrid.cpp

    a b void FormMain::PopulateWithExamples () 
    13191319                                   soc, 
    13201320                                  240) ); 
    13211321    pg->GetProperty(wxT("EnumProperty 2"))->AddChoice(wxT("Testing Extra"), 360); 
     1322    pg->GetProperty(wxT("EnumProperty 2"))->DeleteChoice(1); 
    13221323 
    13231324    // Here we only display the original 'soc' choices 
    13241325    pg->Append( new wxEnumProperty(wxT("EnumProperty 3"),wxPG_LABEL, 

It can be observed whether property is attached or detached. The cause of the problem is that when added/removed choice item modifies the index of currently selected item then wrong new choice value is selected and empty field is displayed in the associated combobox.

comment:5 Changed 7 weeks ago by awi

See r76969

comment:6 Changed 7 weeks ago by AW

In 76970:

Additional assertions in several wxPGProperty methods.

Check if property is attached to the property grid before wxPG methods are invoked.

See #16401.

comment:7 Changed 7 weeks ago by AW

  • Owner set to AW
  • Resolution set to fixed
  • Status changed from new to closed

In 76971:

Fix wxPGProperty::SetChoiceSelection

When selected choice is changed then set as selected value the value corresponding to the new index, not the index itself.

Closes #16401.

comment:8 Changed 7 weeks ago by anhsoft

In sample code (dev-latest with fixed wxPGProperty::SetChoiceSelection):

wxPGProperty* pProp = new wxEnumProperty(wxSTR("label"), wxSTR("name"));
Append(pProp);
pProp->AddChoice(wxSTR("item 3"));
pProp->AddChoice(wxSTR("item 4"));
// Append(pProp);

causes new assertion:

./include/wx/propgrid/property.h(803): assert "i < GetCount()" failed in Item(): invalid index

Looking at the nested calls is clear that wxPGProperty::InsertChoice() received the index value (newSel) and increments it before send to wxPGProperty::SetChoiceSelection(). But initially our choices is empty, that is newSel will be referenced to the element after the last.

Last edited 7 weeks ago by anhsoft (previous) (diff)

comment:9 Changed 7 weeks ago by AW

In 76974:

Fix inserting first choice item to wxEnumProperty

Situation when when first choice item is inserted (in wxPGProperty::InsertChoice) to the empty collection must be handled in a special way. In order to do so:

  1. The state when there are no choice items in wxEnumProperty object and hence no item is selected must be explicitly indicated (by special index value wxNOT_FOUND).
  2. This initial state must be handled in a special way when there is determined new selection index after insertion.

See #16401.

comment:10 follow-up: Changed 7 weeks ago by anhsoft

Due to changeset #76977 needs to call SetIndex() in all ctors, otherwise we get fail in follow sample:

wxPGChoices empty_choices;
pProp = new wxEnumProperty(wxT("label"), wxT("name"), empty_choices);
pProp->AddChoice(wxT("item"));

Additional, DeleteChoice() may to unspecified the value, but wxEnumProperty::m_index not changes and returns by GetChoiceSelection() as is

Last edited 7 weeks ago by anhsoft (previous) (diff)

comment:11 Changed 7 weeks ago by AW

In 76993:

Add missing index initialization in one of wxEnumProperty ctor.

In every wxEnumProperty ctor, initial index value of selected choice must be set to zero.

See #16401.

comment:12 in reply to: ↑ 10 Changed 7 weeks ago by awi

Replying to anhsoft:

Thanks for testing!

comment:13 Changed 6 weeks ago by anhsoft

Do I understand that the line

index = value.GetLong();

must be

index = m_choices.Index(value.GetLong());

instead?
In property.cpp

int wxPGProperty::GetChoiceSelection() const
{
//skip
    if ( valueType == wxPG_VARIANT_TYPE_LONG )
    {
        index = value.GetLong();
    }
//skip

comment:14 Changed 6 weeks ago by anhsoft

Note: behaviour of InsertChoice(), InsertChoice(), GetChoiceSelection() and SetChoiceSelection() isn't consistent.
When I create a new property of native wxEnumProperty class then it works ok.
When I create any wxPGProperty-derived object with the same editor type (i.e. wxPGEditor_Choice), then the assertion is occurs in wxPGProperty::InsertChoice(), because wxPGProperty::SetChoiceSelection(newSel) is called with newSel = -1 (in my test this occurs when the property object don't assigned to grid and isn't initialized with any choices before).

comment:15 Changed 6 weeks ago by AW

In 77011:

Fix wxPGProperty::GetChoiceSelection

Return the index of the integer choice item on the list instead of item value itself.

See #16401.

comment:16 Changed 6 weeks ago by AW

In 77012:

Fix wxPGProperty::InsertChoice

Take into account that wxPGProperty::GetChoiceSelection can return either 0 (for wxEnumProperty) or -1 (for other properties) if choice value list is empty and initialize new selection index properly.

See #16401.

comment:17 Changed 6 weeks ago by anhsoft

Changeset #77011 did what we can't add a new choices without getting assert!

Note: See TracTickets for help on using tickets.