Opened 7 years ago

Closed 7 years ago

#13682 closed defect (invalid)

wxDataViewCtrl ItemAdded check

Reported by: Allonii Owned by:
Priority: normal Milestone:
Component: GUI-generic Version: stable-latest
Keywords: wxDataViewCtrl, items not added Cc:
Blocked By: Blocking:
Patch: yes

Description

I have just built an application using the source from svn and noticed a problem in wxDataViewCtrl check in ItemAdded. As can be seen in the patch that if the item was not found then ItemAdded returns false and nothing happens. So one should also check if the model is empty.

Attachments (2)

datavgen.patch download (663 bytes) - added by Allonii 7 years ago.
newer version
testcase.patch download (3.6 KB) - added by Allonii 7 years ago.
to show the problem

Download all attachments as: .zip

Change History (9)

comment:1 follow-up: Changed 7 years ago by vaclavslavik

  • Status changed from new to infoneeded_new

Sorry, I don't understand -- this notification is called when an item was added to the model, so this list cannot be empty.

How do I reproduce the problem in samples/dataview?

comment:2 in reply to: ↑ 1 ; follow-up: Changed 7 years ago by Allonii

  • Status changed from infoneeded_new to new

Replying to vaclavslavik:

Sorry, I don't understand -- this notification is called when an item was added to the model, so this list cannot be empty.

How do I reproduce the problem in samples/dataview?

Sure, but let me explain first. If you check the "Music Model" in the sample then you will see that it has a root with children. Now if you don't want a root you would call ItemAdded(NULL,branch), so ItemAdded will do FindNode(NULL) and will return m_root instead. The code in ItemAdded function then calls GetModel()->GetChildren(parent, modelSiblings) but we want to use m_root as parent and that's why modelSiblingsSize will be zero.

Hope this makes it easier to understand what I'm trying to say.

comment:3 Changed 7 years ago by vaclavslavik

  • Status changed from new to infoneeded_new

Again, how do I reproduce the problem in the sample? If I can't, can you please attach a patch necessary to see it?

comment:4 Changed 7 years ago by Allonii

  • Status changed from infoneeded_new to new

Ok, I just did not have time to create a patch showing the problem. I hoped I could just explain it =).

Anyway apply the patch "testcase" to the dataview sample and you will see the problem. I have made a lot of comments in the code so you should understand from there. I have updated the patch too which should make more sense now, so look at it also.

Changed 7 years ago by Allonii

newer version

Changed 7 years ago by Allonii

to show the problem

comment:5 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by vadz

  • Milestone 2.9.3 deleted

Replying to Allonii:

Sure, but let me explain first. If you check the "Music Model" in the sample then you will see that it has a root with children. Now if you don't want a root

Sorry, what do you mean by "don't want a root"? You always have some items at the root level, and with wxDVC you can have several of them without any problem (i.e. unlike with wxTreeCtrl you don't need any wxTR_HIDDEN_ROOT hacks). So are you sure you're not just overcomplicating things a lot doing something that is naturally supported by the control in an unsupported way?

In any case this doesn't seem to be 2.9.3-critical to me, even though it would be nice to at least understand whether there is something wrong in the control itself here or if the problem is in your code.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by Allonii

Replying to vadz:

Sorry, what do you mean by "don't want a root"? You always have some items at the root level, and with wxDVC you can have several of them without any problem (i.e. unlike with wxTreeCtrl you don't need any wxTR_HIDDEN_ROOT hacks). So are you sure you're not just overcomplicating things a lot doing something that is naturally supported by the control in an unsupported way?

In any case this doesn't seem to be 2.9.3-critical to me, even though it would be nice to at least understand whether there is something wrong in the control itself here or if the problem is in your code.

Did you try the testcase.patch? The items are not added if you don't specify the root for the children and in that case you must have a visible root. This worked without a problem in 2.9.2 but the changes at 69547 added that check. (My problem is only with the check and not the patch itself)

This is the music model

root

branch
branch

you should be able to have

branch
branch

without having to create a root (that is use the controls m_root).

what's unclear about this?

comment:7 in reply to: ↑ 6 Changed 7 years ago by vaclavslavik

  • Resolution set to invalid
  • Status changed from new to closed

Replying to Allonii:

what's unclear about this?

Pretty much everything.

I tried your patch and even though you never said how to reproduce it using the patch, I think I did: clicking "Add Mozart" triggers it. Turns out that the assert is valid and that it correctly caught a bug in your model implementation: Your GetChildren(NULL, ...) incorrectly returns empty list instead of the list of toplevel nodes ("roots").

The assert is correct and can only be triggered by one of the following:

(1) the model incorrectly reports what the children are; or
(2) the model calls ItemAdded() notification at a wrong time, i.e. before the model is updated

Note: See TracTickets for help on using tickets.