Adding content items to SR tree with AM_beforeCurrent fails

[zaq]

Hello,


Recently I've been playing around with DCMTK's SR module and I found what I think is a bug in DSRTree::addNode() method. It seems that not all pointers to surrounding nodes get updated which corrupts the whole structure.

I noticed the problem when trying to use DSRDocumentTree::addContentItem() in AM_beforeCurrent mode. Even though this method was returning success, the node wasn't included in the dataset produced by DSRDocumentTree::write(). After some research I came across DSRTree::addNode() routine and noticed that some associations weren't updated, which caused write() method to omit newly inserted item.

I applied a quick patch to fix this behavior, here is diff:

Code: Select all

*** /tmp/dsrtree_old.cc     2010-10-14 15:14:42.000000000 +0200
--- /tmp/dsrtree_new.cc   2012-08-14 22:46:40.264994655 +0200
***************

*** 111,122 ****
--- 111,132 ----
                      node->Prev = NodeCursor;
                      node->Next = NodeCursor->Next;
                      NodeCursor->Next = node;
+                     if ( node->Next ) {
+                         node->Next->Prev = node;
+                     }
                      ++Position;
                      break;
                  case AM_beforeCurrent:
                      node->Prev = NodeCursor->Prev;
                      node->Next = NodeCursor;
                      NodeCursor->Prev = node;
+                     if ( Position > 1 ) {
+                         assert( node->Prev );
+                         node->Prev->Next = node;
+                     }
+                     else if ( Position == 1 && ! NodeCursorStack.empty() ) {
+                         NodeCursorStack.top()->Down = node;
+                     }
                      break;
                  case AM_belowCurrent:
                      /* store old position */

I only tested it with my application and it worked, but I don't know dcmsr module internals enough to promise it doesn't break something. Anyway, I hope it helps someone if he encounters the same problem.


Paweł

[J. Riesmeier]

Good catch! Thank you for the report and suggested fix. I would propose the following (slightly different) patch:

Code: Select all

diff --git a/dcmsr/libsrc/dsrtree.cc b/dcmsr/libsrc/dsrtree.cc
index 5b666c3..4cea6b0 100644
--- a/dcmsr/libsrc/dsrtree.cc
+++ b/dcmsr/libsrc/dsrtree.cc
@@ -1,6 +1,6 @@
 /*
  *
- *  Copyright (C) 2000-2010, OFFIS e.V.
+ *  Copyright (C) 2000-2012, OFFIS e.V.
  *  All rights reserved.  See COPYRIGHT file for details.
  *
  *  This software and supporting documentation were developed by
@@ -98,17 +98,26 @@ size_t DSRTree::addNode(DSRTreeNode *node,
     {
         if (NodeCursor != NULL)
         {
+            /* update references based on 'addMode' */
             switch (addMode)
             {
                 case AM_afterCurrent:
                     node->Prev = NodeCursor;
                     node->Next = NodeCursor->Next;
+                    /* connect to current node */
+                    if (NodeCursor->Next != NULL)
+                        (NodeCursor->Next)->Prev = node;
                     NodeCursor->Next = node;
                     ++Position;
                     break;
                 case AM_beforeCurrent:
                     node->Prev = NodeCursor->Prev;
                     node->Next = NodeCursor;
+                    /* connect to current node */
+                    if ((NodeCursor->Prev != NULL) && (Position > 1))
+                        (NodeCursor->Prev)->Next = node;
+                    else if (!NodeCursorStack.empty() && (Position == 1))
+                        NodeCursorStack.top()->Down = node;
                     NodeCursor->Prev = node;
                     break;
                 case AM_belowCurrent:

It's really interesting to see that nobody expecienced this behavior in the last 12 years

I will also write a little test program on this issue (regression test) in order to make sure that it will work "forever" ...

[J. Riesmeier]

This is the git commit: http://git.dcmtk.org/web?p=dcmtk.git;a= ... 395ccd134e
And here is the regression test: http://git.dcmtk.org/web?p=dcmtk.git;a= ... 94787b10d3

[zaq]

That's great, thank you. I'm very glad I could help this project just a little bit.