Opened 17 months ago

Closed 3 months ago

Last modified 3 months ago

#15251 closed defect (fixed)

Search through categories in wxPropertyGrid::GetPropertyByLabel()

Reported by: alexandrub Owned by: AW
Priority: low Milestone:
Component: wxPropertyGrid Version: dev-latest
Keywords: GetPropertyByLabel wxPropertyGridInterface Cc:
Blocked By: Blocking:
Patch: yes

Description

As oposed to current stable release (2.8.12), as far as I notice. This breaks functionality of existing code without (at least for me) an apparent reason. This part of code has been committed along with the bulk of progrid 2.9 code (in r55576, Ticket #9934) so I have no additional clues on this.

The solution is simple, when obtaining the iterator in wxPropertyGridInterface::GetPropertyByLabel() specify wxPG_ITERATE_CATEGORIES as well (as seen in the patch).

Attachments (1)

GetPropertyByLabel_cycle_categories.patch download (550 bytes) - added by alexandrub 17 months ago.

Download all attachments as: .zip

Change History (7)

Changed 17 months ago by alexandrub

comment:1 Changed 17 months ago by vadz

  • Priority changed from high to low
  • Summary changed from GetPropertyByLabel() does not search through categories in 2.9 to Search through categories in wxPropertyGrid::GetPropertyByLabel()
  • Type changed from defect to enhancement

I didn't realize it immediately but actually I have a big problem with the premise of this bug report: wxPG has only been included in wx since 2.9. So what do you compare it with, exactly? It simply wasn't there in 2.8.12.

Now I don't know if the proposed behaviour would be better or worse than the current one, I really don't know this component at all. Could you please explain the effect of this change, preferably in the propgrid sample?

But in any case, this is not a regression, knowing that there was nothing to regress in the first place...

comment:2 Changed 3 months ago by awi

  • Keywords GetPropertyByLabel wxPropertyGridInterface added
  • Type changed from enhancement to defect
  • Version set to dev-latest

There are no records regarding wxPropertyGridInterface::GetPropertyByLabel function in the svn log so indeed this is not a regression but it seems that the real issue was reported.
There are few functions to get wxPG property by name/label:

  1. wxPropertyGridInterface::GetPropertyByName
  2. wxPropertyGridInterface::GetPropertyByLabel
  3. wxPropertyGridPageState::GetPropertyByLabel

They all return the same results if asked for regular properties but if asked for 'category property' (wxPropertyCategory) then function 1 and 3 return correct pointer but function 2 (this reported as failed) returns NULL. Test case (modified propgrid sample) below.

Conclusions:

  1. For the sake of consistency all these functions should return the same results for the same arguments so including 'category properties' to the iteration in wxPGInterface::GetPropertyByLabel seems to be reasonable.
  2. For unknown reasons there exposed in wxPGPage both above mentioned GetPropertyByLabel functions (inherited from base wxPGInterface and wxPGState classes). They have the same signature (with default arguments) what leads to ambiguity and hence they must be called by qualified names (as in below sample) what is really inconvenient and ugly. Hiding in a some way one of these functions would break ABI so this question cannot be fixed now.

@Vadz,
When is earliest opportunity to do some changes in ABI? At wx 3.1 or only 4.0?

  • samples/propgrid/propgrid.cpp

    a b FormMain::FormMain(const wxString& title, const wxPoint& pos, const wxSize& size 
    21412141                                  GetPosition().y); 
    21422142    m_logWindow->Show(); 
    21432143#endif 
     2144 
     2145    wxPropertyGridManager* pgman = m_pPropGridManager; 
     2146    wxPropertyGridPage* pg = pgman->GetPage(wxT("Standard Items")); 
     2147    // Find category property 
     2148    wxPGProperty *p1 = pg->GetPropertyByName(wxT("Appearance")); 
     2149    wxPGProperty *p2 = pg->wxPropertyGridInterface::GetPropertyByLabel(wxT("Appearance")); 
     2150    wxPGProperty *p3 = pg->wxPropertyGridPageState::GetPropertyByLabel(wxT("Appearance")); 
     2151    wxASSERT(p1 == p2); 
     2152    wxASSERT(p2 == p3); 
     2153    // Find regular property 
     2154    wxPGProperty *p4 = pg->GetPropertyByName(wxT("Font")); 
     2155    wxPGProperty *p5 = pg->wxPropertyGridInterface::GetPropertyByLabel(wxT("Font")); 
     2156    wxPGProperty *p6 = pg->wxPropertyGridPageState::GetPropertyByLabel(wxT("Font")); 
     2157    wxASSERT(p4 == p5); 
     2158    wxASSERT(p5 == p6); 
    21442159} 
    21452160 
    21462161void FormMain::FinalizeFramePosition() 

comment:3 Changed 3 months ago by vadz

ABI is only fixed in 3.0 branch, i.e. you don't need to care about it in the trunk (that will become 3.1.0 eventually).

Even the API breakage is permitted in the trunk, although it is, of course, always better to avoid it if possible.

comment:4 Changed 3 months ago by AW

In 76925:

Resolve ambiguity in multiple inheritance of function GetPropertyByLabel in wxPGPage.

In wxPropertyGridPage class derived from wxPropertyGridPageState and wxPropertyGridInterface, function GetPropertyByLabel is inherited from both base classes.
To resolve this ambiguity, function defined in wxPGInterface is introduced in wxPGPage through 'using-declaration'.
(This is a temporary fix and finally GetPropertyByLabel function should be removed from wxPropertyGridPageState.)

See #15251.

comment:5 Changed 3 months ago by AW

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

In 76926:

Search through all properties in wxPG::GetPropertyByLabel.

Function should search for given label through all properties. This is consistent with searching by name (in GetPropertyByName) which is not limited to the subset of properties.

Closes #15251.

comment:6 Changed 3 months ago by alexandrub

  1. For the sake of consistency all these functions should return the same results for the same arguments so including 'category properties' to the iteration in wxPGInterface::GetPropertyByLabel seems to be reasonable.

I was not entirely wrong when I stated this is a regression. Even before wx 2.9, we were using wxPG 1.4 as an extension and I assume other users did as well. Hence there actually was a functionality regression between wxPG code included in wx 2.9 and wxPG v1.4. In v1.4, GetPropertyByLabel() returned the same value as GetPropertyByName() for category properties as well.

Note: See TracTickets for help on using tickets.