Opened 13 months ago

Closed 5 months ago

Last modified 5 months ago

#15542 closed defect (fixed)

wxColourProperty with choice box--hitting Cancel button on color dialog causes wrong color to be used

Reported by: snowleopard2 Owned by: vadz
Priority: low Milestone:
Component: wxPropertyGrid Version: dev-latest
Keywords: wxColourProperty wxPropertyGrid wxSystemColourProperty Cc:
Blocked By: Blocking:
Patch: yes

Description

In the property grid sample, set any wxColourProperty to use as "wxPGEditor_Choice" or "wxPGEditor_ChoiceAndButton" as its editor, like so:

pg->SetPropertyEditor( wxT("ColourProperty "), wxPGEditor_Choice );

Run the sample and find the property, expand its combobox, and select "Custom" at the bottom and select something like purple from the color dialog. Now, select Custom again from the combobox, but hit the Cancel button on the Color dialog.

Instead of keeping the color as it was, it will arbitrarily set it to black.

Note that using wxPGEditor_Combo doesn't have this issue.

Attachments (1)

identification-of-custom-color.patch download (489 bytes) - added by awi 5 months ago.
Fixed identication of custom colour in wxSystemColourProperty::IntToValue().

Download all attachments as: .zip

Change History (8)

comment:1 Changed 13 months ago by snowleopard2

Meant to say, wxPGEditor_ComboBox doesn't have this issue.

comment:2 Changed 5 months ago by awi

  • Keywords wxSystemColourProperty added
  • Patch set
  • Version changed from 2.9.5 to dev-latest

Apparently, problem is caused by incorrect identification of the "Custom color" item in the selection list.
Patch fixing the issue is attached (attachment:ticket:15542:identification-of-custom-color.patch).

Patch to reproducing the issue:

  • samples/propgrid/propgrid.cpp

    a b void FormMain::PopulateWithExamples () 
    12551255#endif 
    12561256 
    12571257    pid = pg->Append( new wxColourProperty(wxT("ColourProperty"),wxPG_LABEL,*wxRED) ); 
    1258     pg->SetPropertyEditor( wxT("ColourProperty"), wxPGEditor_ComboBox ); 
     1258//    pg->SetPropertyEditor( wxT("ColourProperty"), wxPGEditor_ComboBox ); 
     1259    pg->SetPropertyEditor( wxT("ColourProperty"), wxPGEditor_Choice ); 
     1260 
    12591261    pg->GetProperty(wxT("ColourProperty"))->SetAutoUnspecified(true); 
    12601262    pg->SetPropertyHelpString( wxT("ColourProperty"), 
    12611263        wxT("wxPropertyGrid::SetPropertyEditor method has been used to change ") 

Changed 5 months ago by awi

Fixed identication of custom colour in wxSystemColourProperty::IntToValue().

comment:3 Changed 5 months ago by vadz

  • Priority changed from normal to low
  • Status changed from new to confirmed

Isn't it wrong that the value associated to the custom colour index is not wxPG_COLOUR_CUSTOM though? I don't understand this code (nor the data structures used in it) at all but it seems that the intention of this code is to ensure that all custom colours use this type. I could be totally wrong though, I really have no idea about what's going on here :-( It just seems bad to test for the label, maybe we could test for the index directly instead?

comment:4 Changed 5 months ago by awi

Right, wxPropertyGrid code is quite convoluted and the module has its own logic which is not easy to reveal.

It seems to me that in the current code wxPG_COLOUR_CUSTOM is never used to identification of "custom color" item. Maybe this was an intention but apparently it is not utilized in this manner and simply color label is used for this purposes.
E.g. in the existing implementation of wxSystemColourProperty custom color item is defined as follows:

m_choices.Insert(_("Custom"), GetCustomColourIndex(), wxPG_COLOUR_CUSTOM);

and identified in StringToValue() this way:

    wxString custColName(m_choices.GetLabel(GetCustomColourIndex()));
...
    if ( colStr != custColName )

which is practically the same as comparison of given color label with _("Custom").

Definitely, identifying custom color by label is not a perfect solution but could be acceptable in the context of the reported issue.
I think it could be hard to fix this issue in a more conceptually proper manner without revamping existing code base. It seems to me that this issue is too simple to put so much effort to fix it this way.

comment:5 Changed 5 months ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

OK, I didn't realize that the existing code already used the label like this, thanks for the explanation!

I'll apply the original patch soon then, thanks again!

comment:6 Changed 5 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

In 76624:

Fix cancelling choice of custom colour in wxPropertyGrid.

When using wxPGEditor_Choice colour property, cancelling the choice of the
custom colour reset the previously selected custom colour.

Fix this by correcting the test for the custom colour which didn't work
before.

Closes #15542.

comment:7 Changed 5 months ago by VZ

In 76627:

Fix cancelling choice of custom colour in wxPropertyGrid.

When using wxPGEditor_Choice colour property, cancelling the choice of the
custom colour reset the previously selected custom colour.

Fix this by correcting the test for the custom colour which didn't work
before.

Closes #15542.

Note: See TracTickets for help on using tickets.