Opened 17 months ago

Last modified 35 hours ago

#18405 accepted defect

wxDataViewMainWindow::ItemAdded() adds duplicate child nodes to parents that haven't been opened yet

Reported by: Honey Owned by: vadz
Priority: normal Milestone: 3.1.5
Component: GUI-generic Version: 3.1.3
Keywords: wxDataViewCtrl Cc: greebo@…
Blocked By: Blocking:
Patch: yes

Description (last modified by vadz)

As the title suggests.

Basically, when I'm opening a file with my software, its code builds a BlockTree which derives from wxDataViewModel.
Then, it executes this snippet:

const auto l_parentChildPairs{ m_pTree->InsertBlockPack(wxDataViewItem(nullptr), l_dat.BlockPack()) };
for (const auto& i : l_parentChildPairs)
{
    m_pTree->ItemAdded(i.first, i.second);
}

m_pTree is a pointer to the built BlockTree object.
InsertBlockPack(...) returns a vector of parent-child pairs.

Then it just goes over the vector calling the ItemAdded(parent, child) function to notify the model that these items have been added.

This worked like a charm since forever, but lately I updated wxWidgets to the latest 3.1.2 release and now that code in said circumstances generates, for each node except the root' child nodes, a duplicate of all nodes except for its first child node.

Example tree:

root_child
..foo
..bar
....node0
....node1
....node1
..bar

I've already "fixed" my code. Although, I see the lazy-construction logic comment in ItemAdded() and it does this:

if ( !parentNode->IsOpen() && parentNode->GetChildNodes().empty() )

but if fails, because child node array is not empty at that point. And then it goes haywire.

Change History (11)

comment:1 Changed 17 months ago by vadz

  • Description modified (diff)
  • Keywords wxDataViewCtrl added
  • Status changed from new to infoneeded_new

Sorry, we'd need to have some (simple) way of reproducing the problem because I just don't see why are the items added twice from the code provided here and I can't do much about it without either reproducing it myself or at least getting a more detailed explanation from you (the current one is not sufficient because I don't see anything wrong with the logic of the test you're showing...)

I'd like to fix any regressions since 3.0 before 3.2, of course, and this looks like a rather bad one but, again, there is not much I can do without a reproducible test case (ideally as a patch to the dataview sample) or more details.

Could you please provide it/them?

TIA!

comment:2 Changed 7 days ago by codereader

  • Cc greebo@… added
  • Status changed from infoneeded_new to new

Revisiting this entry, I can confirm the behaviour. I have quite a few wxDataViewCtrls with custom tree models in my application, but only one of them shows this behaviour. Still it's reproducible.

I'm currently working with wxWidgets 3.1.3, and I can confirm that this problem didn't exist in 3.0.x.

The reason for this bug can be found in wxDataViewMainWindow::ItemAdded:

  • If the parent node is either expanded or has child tree nodes, ItemAdded will construct a new wxDataViewTreeNode for the given wxDataViewItem
  • Earlier in ItemAdded, there's a call to FindNode(parent), which tries to locate the tree node for the parent item.

The problem is that FindNode will implicitly instantiate a subtree under certain conditions, creating new wxDataViewTreeNodes on its own - once control is back in ItemAdded, the construction of the wxDataViewTreeNode will create duplicates.

The reproduction likely requires a tree model that is already populated with a hierarchy of more than one depth level and more than one sibling node. The BuildTreeHelper will invoke model->GetChildren() to find out about any uninstantiated nodes and will create them automatically - making the ItemAdded call obsolete.

Since there's no way for the client code to know when it's safe to call ItemAdded, I suppose a check in ItemAdded whether the corresponding tree node has already been constructed would fix the problem. Or a different lazy-evaluation logic, I'm not too familiar with that code.

I don't have a working build environment for wxWidgets itself (it would be my first-time contribution anyway), so I can't provide a patch as of yet. But maybe the information here is useful.

comment:3 Changed 7 days ago by codereader

  • Version changed from 3.1.2 to 3.1.3

comment:4 Changed 7 days ago by vadz

Yes, thank you, it's useful and I'll try to look at this later, but without having a test case, I can't even check whether some fix for the problem really works.

FWIW building wx should just work out of the box, it doesn't have any weird build systems requiring elaborate setup or anything like this.

comment:5 Changed 6 days ago by codereader

I managed to check out and compile wxWidgets sources (forgot to include the submodules first... duh).

I'll see if I can come up with a unit test case for the above problem. Looking at the test_gui project, it looks like there currently are only a few small tests covering the wxDataViewCtrl.

comment:6 Changed 4 days ago by codereader

  • Cc greebo@… removed

I've set up the control and the test data in the dataviewctrltest.cpp file in the test_gui project.

It's in commit 9f6e3b38 of my fork. The unit test is called "wxDataViewModel::ItemAdded", if you want to run it directly.

There's no assertion in the unit test, since we don't have access to the wxDataViewMainWindow where the actual problem is happening. Instead the unit test will display a dialog, where you can see the problematic behaviour: all the items below the first level are duplicated.

Is this something you can work with?

Last edited 4 days ago by codereader (previous) (diff)

comment:7 Changed 4 days ago by codereader

  • Cc greebo@… added

comment:8 Changed 3 days ago by vadz

  • Milestone set to 3.1.5
  • Owner set to vadz
  • Status changed from new to accepted

I'll try to have a look at this, thanks.

comment:9 Changed 2 days ago by vadz

  • Patch set

So I understand the problem now, thanks to your example, but I'm still not sure about the solution. I've created PR 2095 which does solve it but it does it by reverting 4dc78a33e0 (Fix adding items to a never opened branch of generic wxDataViewCtrl, 2018-01-17) which was added to fix a related problem. Unfortunately that commit just can't work with the scenario in this ticket, i.e. depth-first model traversal, for the reasons described in the commit message. Even more unfortunately, I couldn't actually reproduce the problem that commit was meant to solve. Because of this I'm not really sure that reverting it is the right thing to do, but OTOH it does fix a bug I can reproduce, so for now let's try doing it like this...

Ideal would be to postpone building the internal tree until as late as possible, but this requires more changes to FindNode() and I'm not sure this is not going to break something else.

Anyhow, please test the PR above with your actual application. I'm reasonably confident it should fix the problem in it too, but it would be nice to confirm this, of course. TIA!

comment:10 Changed 42 hours ago by codereader

Thanks for looking into it, this is appreciated! I'll try to have a look as soon as I can. It will be a busy week, so I can't do it immediately.

One thought I had while debugging the problem: wouldn't it be possible to check the parent node whether it already contains a tree node representing the currently added item? Such that it prevents a duplicate being added after FindNode(parent)?

(Such a fix will affect performance though, re-populating really flat trees with huge numbers of items might not be as fast as it used to, if every call to ItemAdded iterates over the parent node to find a possible duplicate.)

comment:11 Changed 35 hours ago by vadz

I've thought about doing this, but it doesn't seem right to compensate for adding the items twice, when normally this shouldn't happen in the first place.

I believe postponing construction of the internal node tree until even later, i.e. not creating the subtree in FindNode() when it's not really needed, is a more promising idea, but I couldn't make it work quickly yesterday.

Note: See TracTickets for help on using tickets.