Merge lp:~spud/spud/copy-paste-fix into lp:spud

[Fraser Waters]

Fix for bug #811200 and #812348. on_paste was not handling MixedTree's in the same way as on_copy. on_paste was not handling choices correctly.

[Cian Wilson (cwilson)]

With that branch I'm still getting the same error:

  File "/usr/lib/python2.7/dist-packages/diamond/interface.py", line 695, in on_paste
    newnode = self.s.read(ios, node)
  File "/usr/lib/python2.7/dist-packages/diamond/schema.py", line 532, in read
    datatree = self.valid_node(root)
  File "/usr/lib/python2.7/dist-packages/diamond/schema.py", line 167, in valid_node
    eidtree.parent.children.remove(eidtree)
ValueError: list.remove(x): x not in list

and behaviour as reported in bug #811200 (i.e. saving the file immediately afterwards saves a file that's different from the one you're seeing and you also don't get a copied branch of the tree).

Can anyone else reproduce this or have I mangled my paths somewhere? (I packaged the branch and installed it so shouldn't be mixing versions of diamond.)

review: Needs Information

[Patrick Farrell (pefarrell)]

I attempted to replicate #811200 with this branch.

I don't get Cian's terminal error; I think that's been fixed, and I suspect it's something to do with running the wrong version of diamond. Cian: when you check out a branch, just do

cd branch
./configure
python diamond/bin/diamond [args]

and it will automatically use the right code.

But I get a different bug. It doesn't give any exception, but when I save the file I get

<?xml version='1.0' encoding='utf-8'?>
<dummy_options>
  <system name="Dummy">
    <nonlinear_solver name="Simple">
      <type name="SNES">
        <python name="Jacobian" rank="1">
          <string_value lines="20" type="python">some python code here</string_value>
        </python>
      </type>
    </nonlinear_solver>
    <nonlinear_solver>
      <type name="SNES">
        <python name="Jacobian" rank="1">
          <string_value type="python" lines="20"/>
        </python>
      </type>
    </nonlinear_solver>
  </system>
</dummy_options>

Note that the second nonlinear solver has a) lost its name, and b) lost its python code. It *looks* fine in the live diamond window, but if you save it it gets lost. (If you open it up again, you'll get all sorts of lost element errors.)

[Cian Wilson (cwilson)]

> cd branch
> ./configure
> python diamond/bin/diamond [args]

I get the same error (terminal output and all) running the command this way:

cwilson@uisce:copy-paste-fix$ python diamond/bin/diamond -s ~/temp/schema/dummy.rng ~/temp/schema/dummy.dml

Just to confirm it's the branch I'm running:
cwilson@uisce:copy-paste-fix$ bzr info
Repository tree (format: 2a)
Location:
  shared repository: /home/cwilson/spud/bzr-repo
  repository branch: .

Related branches:
  parent branch: bzr+ssh://bazaar.launchpad.net/~spud/spud/copy-paste-fix/

> <?xml version='1.0' encoding='utf-8'?>
> <dummy_options>
> <system name="Dummy">
> <nonlinear_solver name="Simple">
> <type name="SNES">
> <python name="Jacobian" rank="1">
> <string_value lines="20" type="python">some python code
> here</string_value>
> </python>
> </type>
> </nonlinear_solver>
> <nonlinear_solver>
> <type name="SNES">
> <python name="Jacobian" rank="1">
> <string_value type="python" lines="20"/>
> </python>
> </type>
> </nonlinear_solver>
> </system>
> </dummy_options>
>
> Note that the second nonlinear solver has a) lost its name, and b) lost its
> python code. It *looks* fine in the live diamond window, but if you save it it
> gets lost. (If you open it up again, you'll get all sorts of lost element
> errors.)

With the exception of looking fine in the diamond window before I close it (i.e. it doesn't look like it's copied to me and diamond shows elements as blue still) that sounds like the same behaviour I see on reopening the file.

[Patrick Farrell (pefarrell)]

Also: I seem to get different behaviour depending on whether I CTRL+C/CTRL+V, or right-click copy, right-click paste. When I right-click copy, I get the following error in the terminal:

Traceback (most recent call last):
  File "/data/pfarrell/src/spud/copy-paste-fix/diamond/bin/../diamond/interface.py", line 658, in on_copy
    if widget is not self.treeview and gobject.signal_lookup("copy-clipboard", widget):
TypeError: type must be instantiable or an interface

The XML appears correct regardless of which approach is taken.

When I paste onto the blank nonlinear_solver, it doesn't have the right cardinality: it doesn't have the '-' on the right-hand side of the treeview to deactivate it.

[Cian Wilson (cwilson)]

The above was done by first adding a new nonlinear_solver element then running CTRL+C on the old copy nonlinear_solver::Simple then moving to the new element and pressing CTRL+V.

If instead I use the right-click feature I get:

Traceback (most recent call last):
  File "/home/cwilson/spud/bzr-repo/copy-paste-fix/diamond/bin/../diamond/interface.py", line 658, in on_copy
    if widget is not self.treeview and gobject.signal_lookup("copy-clipboard", widget):
TypeError: type must be instantiable or an interface

on copy and:

Traceback (most recent call last):
  File "/home/cwilson/spud/bzr-repo/copy-paste-fix/diamond/bin/../diamond/interface.py", line 681, in on_paste
    if widget is not self.treeview and gobject.signal_lookup("paste-clipboard", widget):
TypeError: type must be instantiable or an interface

on paste.

[Fraser Waters (fraser-waters08)]

"TypeError: type must be instantiable or an interface"

I found the cause of that, it's fixed in my local version.

The rest I'm not sure, I'll look into it more it's probably some confusion between Choices, Trees and MixedTrees. Really need to standardize them at some point.

[Cian Wilson (cwilson)]

Repeating the steps (and using the schema and options file) in bug #811200 (i.e. activating a new nonlinear_solver element, copying nonlinear_solver::Simple and trying to paste it into the new active element - using Ctrl+C and Ctrl+V) I get the error:

  File "/usr/lib/python2.7/dist-packages/diamond/interface.py", line 684, in on_paste
    newnode = self.s.read(ios, node)
  File "/usr/lib/python2.7/dist-packages/diamond/schema.py", line 526, in read
    datatree = self.valid_node(root)
  File "/usr/lib/python2.7/dist-packages/diamond/schema.py", line 156, in valid_node
    xpath = self.tree.xpath(eid)
  File "lxml.etree.pyx", line 2029, in lxml.etree._ElementTree.xpath (src/lxml/lxml.etree.c:45934)
  File "xpath.pxi", line 363, in lxml.etree.XPathDocumentEvaluator.__call__ (src/lxml/lxml.etree.c:114234)
  File "apihelpers.pxi", line 1364, in lxml.etree._utf8 (src/lxml/lxml.etree.c:22190)
TypeError: Argument must be bytes or unicode, got 'Choice'

Nothing appears in the pasted to element.

If I then delete the node that I just failed to copy information to and save the file, close it and reopen it, it reappears as I last saw (and saved) it so that part of the bug appears fixed.

I get the same error using right-click copy and paste and if the element being pasted to is active or not.

Can anyone reproduce this with this branch? I've built packages from this branch and overwritten my previously installed spud so am reasonably confident I'm not getting paths muddled.

review: Needs Information

[Patrick Farrell (pefarrell)]

Cian: yes, I can reproduce your behaviour. Thanks again for your thorough testing.

We know that the way choices are handled needs to be changed (there's another bug I didn't bother writing up), so Fraser will start on that soon.

[Fraser Waters (fraser-waters08)]

Yes just to confirm. Choice nodes aren't being handled correctly at the moment. I'm working on it. Trying to paste on a choice should always fail at the moment. If your getting bugs with normal nodes keep reporting them, because they SHOULD work.

review: Needs Fixing

[Fraser Waters (fraser-waters08)]

Choice nodes should be handled correctly now.

review: Needs Resubmitting

Unmerged revisions

=== modified file 'diamond/diamond/choice.py'
--- diamond/diamond/choice.py	2011-07-26 14:04:07 +0000
+++ diamond/diamond/choice.py	2011-07-28 13:56:32 +0000
@@ -31,23 +31,23 @@
   __gsignals__ = { "on-set-data" : (gobject.SIGNAL_RUN_LAST, gobject.TYPE_NONE, (str,)),
                    "on-set-attr"  : (gobject.SIGNAL_RUN_LAST, gobject.TYPE_NONE, (str, str))}
 
-  def __init__(self, l, cardinality=''):
+  def __init__(self, choices, schemaname="", cardinality=''):
     gobject.GObject.__init__(self)
 
-    self.l = l
-    if l == []:
+    self.choices = choices
+    if choices == []:
       raise Exception
     self.index = 0
     name = ""
-    for choice in l:
+    for choice in choices:
       assert choice.__class__ is tree.Tree
-      name = name + choice.name + ":"
       choice.connect("on-set-data", self._on_set_data)
       choice.connect("on-set-attr", self._on_set_attr)
 
-    name = name[:-1]
+    name = ":".join(choice.name for choice in choices)
+
     self.name = name
-    self.schemaname = name
+    self.schemaname = schemaname
     self.cardinality = cardinality
     self.parent = None
     self.set_default_active()
@@ -67,21 +67,21 @@
     self.index = i
 
   def find_tree(self, name):
-    for t in self.l:
+    for t in self.choices:
       if t.name == name:
         return t
 
     debug.deprint("self.name == %s" % self.name, 0)
-    for choice in self.l:
+    for choice in self.choices:
       debug.deprint("choice.name == %s" % choice.name, 0)
     raise Exception, "No such choice name: %s" % name
 
   def set_active_choice_by_name(self, name):
     matched = False
-    for t in self.l:
+    for t in self.choices:
       if t.name == name.strip():
         matched = True
-        self.index = self.l.index(t)
+        self.index = self.choices.index(t)
 
     if not matched:
       raise Exception, "no such name %s found" % name
@@ -89,11 +89,11 @@
     self.recompute_validity()
 
   def set_active_choice_by_ref(self, ref):
-    self.index = self.l.index(ref)
+    self.index = self.choices.index(ref)
     self.recompute_validity()
 
   def get_current_tree(self):
-    return self.l[self.index]
+    return self.choices[self.index]
 
   def add_children(self, schema):
     return self.get_current_tree().add_children(schema)
@@ -106,7 +106,7 @@
 
   def copy(self):
     new_choices = []
-    for choice in self.l:
+    for choice in self.choices:
       new_choices.append(choice.copy())
 
     new_choice = Choice(new_choices)
@@ -114,37 +114,23 @@
       setattr(new_choice, attr, copy.copy(getattr(self, attr)))
 
     new_choice.set_parent(self.parent)
-    for choice in new_choice.l:
+    for choice in new_choice.choices:
       choice.children = copy.copy([])
 
     return new_choice
 
   def get_possible_names(self):
-    return [x.name for x in self.l]
+    return [x.name for x in self.choices]
 
   def set_parent(self, parent):
     self.parent = parent
-    for choice in self.l:
+    for choice in self.choices:
       choice.parent = parent
 
   def write_core(self, parent):
-    l = self.l
-    for i in range(len(l)):
-      if self.index == i:
-        l[i].write_core(parent)
-#      else:
-#        root=etree.Element(parent.tag)
-#        l[i].write_core(root)
-#        comment_buffer = StringIO.StringIO(etree.tostring(root))
-#        comment_text = ("DIAMOND MAGIC COMMENT (neglected choice subtree %s):\n" % l[i].schemaname)
-#        comment_text = comment_text + base64.b64encode(bz2.compress(comment_buffer.getvalue()))
-#        parent.append(etree.Comment(unicode(comment_text)))
-
+    self.choices[self.index].write_core(parent)
     return parent
 
-  def choices(self):
-    return self.l
-
   def is_comment(self):
     return False
 
@@ -167,7 +153,7 @@
     return [self.get_current_tree()]
 
   def get_choices(self):
-    return self.l
+    return self.choices
 
   def is_hidden(self):
     """
@@ -176,20 +162,7 @@
     return False
 
   def get_name_path(self, leaf = True):
-    name = self.get_display_name() if leaf else self.get_name()
-
-    if self.parent is None:
-      return name
-    else:
-
-      pname = self.parent.get_name_path(False)
-
-      if name is None:
-        return pname
-      elif pname is None:
-        return name
-      else:
-        return pname + "/" + name
+    return self.get_current_tree().get_name_path(leaf)
 
   def get_mixed_data(self):
     return self
@@ -203,4 +176,7 @@
     """
     return self.get_display_name()
 
+  def __repr__(self):
+    return self.get_name_path() + "[" + self.name + "]"
+ 
 gobject.type_register(Choice)
 
=== modified file 'diamond/diamond/interface.py'
--- diamond/diamond/interface.py	2011-07-28 12:03:24 +0000
+++ diamond/diamond/interface.py	2011-07-28 13:56:32 +0000
@@ -283,9 +283,6 @@
     # if we have a relative path, make it absolute
     filename = os.path.abspath(filename)
   
-    if filename == self.filename:
-      return
-  
     try:
       os.stat(filename)
     except OSError:
@@ -638,12 +635,19 @@
     else:
       return self._get_focus_widget(focus)
 
+  def _handle_clipboard(self, widget, signal):
+    if isinstance(widget, gtk.MenuItem):
+      return False
+    else:
+      widget = self._get_focus_widget(self.main_window)
+      if widget is not self.treeview and gobject.signal_lookup(signal + "-clipboard", widget):
+        widget.emit(signal + "-clipboard")
+        return True
+      return False
+
   def on_copy(self, widget=None):
-    if not isinstance(widget, gtk.MenuItem):
-      widget = self._get_focus_widget(self.main_window)
-      if widget is not self.treeview and gobject.signal_lookup("copy-clipboard", widget):
-        widget.emit("copy-clipboard")
-        return
+    if self._handle_clipboard(widget, "copy"):
+      return
 
     if isinstance(self.selected_node, mixedtree.MixedTree):
       node = self.selected_node.parent
@@ -662,28 +666,39 @@
     return
 
   def on_paste(self, widget=None):
-    if not isinstance(widget, gtk.MenuItem):
-      widget = self._get_focus_widget(self.main_window)
-      if widget is not self.treeview and gobject.signal_lookup("paste-clipboard", widget):
-        widget.emit("paste-clipboard")
-        return
+    if self._handle_clipboard(widget, "paste"):
+      return
 
     clipboard = gtk.clipboard_get()
     ios = StringIO.StringIO(clipboard.wait_for_text())
     
     if self.selected_iter is not None:    
-      node = self.treestore.get_value(self.selected_iter, 1)
+      node = self.treestore.get_value(self.selected_iter, 0)
 
     if node != None:
+  
+      expand = not node.active
+      if expand:
+        self.expand_tree(self.selected_iter)
 
       newnode = self.s.read(ios, node)
 
       if newnode is None:
+        if expand:
+          self.collapse_tree(self.selected_iter, False)
         self.statusbar.set_statusbar("Trying to paste invalid XML.")
         return
 
-      if not node.active:
-        self.expand_tree(self.selected_iter)
+      if node.parent is not None:
+        newnode.set_parent(node.parent)
+        children = node.parent.get_children()
+        children.insert(children.index(node), newnode)
+        children.remove(node)
+ 
+      self.set_treestore(self.selected_iter, [newnode], True, True)
+
+      newnode.recompute_validity()
+      self.treeview.queue_draw()
 
       # Extract and display validation errors
       lost_eles, added_eles, lost_attrs, added_attrs = self.s.read_errors()
@@ -710,12 +725,6 @@
         dialogs.long_message(self.main_window, msg)
  
       self.set_saved(False)     
-   
-      self.treeview.freeze_child_notify()
-      iter = self.set_treestore(self.selected_iter, [newnode], True, True)
-      self.treeview.thaw_child_notify()
-      
-      self.treeview.get_selection().select_iter(iter)
 
     return
 
@@ -907,7 +916,7 @@
     if replace:
       replacediter = iter
       iter = self.treestore.iter_parent(replacediter)
-    else:
+    else: 
       self.remove_children(iter)
   
     for t in new_tree:
@@ -1119,7 +1128,7 @@
 
     return
   
-  def collapse_tree(self, iter):
+  def collapse_tree(self, iter, confirm = True):
     """
     Collapses part of the tree.
     """
@@ -1147,7 +1156,7 @@
         self.set_saved(False)
         self.remove_children(iter)
       else:
-        self.delete_tree(iter)
+        self.delete_tree(iter, confirm)
 
     elif choice_or_tree.cardinality == "+":
       count = parent_tree.count_children_by_schemaname(choice_or_tree.schemaname)
@@ -1155,20 +1164,23 @@
         # do nothing
         return
       else: # count > 2
-        self.delete_tree(iter)
+        self.delete_tree(iter, confirm)
   
     parent_tree.recompute_validity()
     self.treeview.queue_draw()
     return
 
-  def delete_tree(self, iter):
+  def delete_tree(self, iter, confirm):
     choice_or_tree, = self.treestore.get(iter, 0)
     parent_tree = choice_or_tree.parent
     isSelected = self.treeview.get_selection().iter_is_selected(iter)
     sibling = self.treestore.iter_next(iter)
 
-    confirm = dialogs.prompt(self.main_window, "Are you sure you want to delete this node?")
-    if confirm == gtk.RESPONSE_YES:
+    if confirm:
+      response = dialogs.prompt(self.main_window, "Are you sure you want to delete this node?")
+
+    # not A or B == A implies B
+    if not confirm or response == gtk.RESPONSE_YES:
       parent_tree.delete_child_by_ref(choice_or_tree)
       self.remove_children(iter)
       self.treestore.remove(iter)
 
=== modified file 'diamond/diamond/schema.py'
--- diamond/diamond/schema.py	2011-07-18 11:49:27 +0000
+++ diamond/diamond/schema.py	2011-07-28 13:56:32 +0000
@@ -142,10 +142,10 @@
     return results
 
   def valid_node(self, eid):
-    if isinstance(eid, tree.Tree):
+    if isinstance(eid, tree.Tree) or isinstance(eid, choice.Choice):
       eidtree = eid
       eid = eid.schemaname
-
+    
     if eid == ":start":
       try:
         node = self.tree.xpath('/t:grammar/t:start', namespaces={'t': 'http://relaxng.org/ns/structure/1.0'})[0]
@@ -162,12 +162,8 @@
     node = self.to_tree(node)
   
     if eidtree is not None:
-      if eidtree.parent is not None:
-        eidtree.parent.children.append(node)
-        eidtree.parent.children.remove(eidtree) 
-        node.set_parent(eidtree.parent)
-      node.attrs = eidtree.attrs
       node.cardinality = eidtree.cardinality
+      node.parent = eidtree.parent
 
     return node
 
@@ -176,7 +172,7 @@
     f = self.callbacks[tag]
     facts = {}
     x = f(element, facts)
-    return x
+    return x    
 
   #############################################
   # Beginning of schema processing functions. #
@@ -389,20 +385,19 @@
       if "schemaname" in facts:
         return
 
+      facts['schemaname'] = self.tree.getpath(element)
+
       r = []
       children = self.choice_children(self.element_children(element))
       
       # bloody simplified RNG
       if len(children) == 2:
         empty = [x for x in children if self.tag(x) == "empty"]
-        nonempty = [x for x in children if self.tag(x) != "empty"]
-        if len(empty) > 0:
+        if empty:
+          nonempty = [x for x in children if self.tag(x) != "empty"]
           tag = self.tag(nonempty[0])
-          if tag == "oneOrMore":
-            return self.cb_oneormore(element, facts)
-          else:
-            f = self.callbacks[tag]
-            return f(element, facts)
+          f = self.callbacks[tag]
+          return f(element, facts)
 
       for child in children:
         newfacts = {}
@@ -533,7 +528,7 @@
 
     xmlnode  = doc.getroot()
     self.xml_read_merge(datatree, xmlnode)
-    self.xml_read_core(datatree, xmlnode, doc)
+    self.xml_read_core(datatree.get_current_tree(), xmlnode, doc)
 
     if len(self.lost_eles) != 0:
       debug.deprint("WARNING: Lost XML elements:\n" + str(self.lost_eles))
@@ -564,7 +559,7 @@
         xmlname = xmlnode.get("name")
         have_found = False
 
-        possibles = [tree_choice for tree_choice in datatree.choices() if tree_choice.name == xmlnode.tag]
+        possibles = [tree_choice for tree_choice in datatree.get_choices() if tree_choice.name == xmlnode.tag]
         # first loop over the fixed-value names
         for tree_choice in possibles:
           if "name" not in tree_choice.attrs:
@@ -698,7 +693,7 @@
 
     for schemachild in priority_queue:
       if schemachild.cardinality in ['', '?']:
-        for curtree in schemachild.choices():
+        for curtree in schemachild.get_choices():
           name = curtree.name
 
           have_fixed_name = False
@@ -725,7 +720,7 @@
               xmls[schemachild.schemaname] = copy.deepcopy([])
       elif schemachild.cardinality in ['*', '+']:
         xmls[schemachild.schemaname] = copy.deepcopy([])
-        for curtree in schemachild.choices():
+        for curtree in schemachild.get_choices():
           name = curtree.name
 
           have_fixed_name = False
@@ -803,7 +798,7 @@
         bins[schemachild.schemaname].append(child)
 
       # search for neglected choices
-      if schemachild.__class__ is choice.Choice and schemachild.cardinality in ['', '?']:
+      if isinstance(schemachild, choice.Choice) and schemachild.cardinality in ['', '?']:
         for child in bins[schemachild.schemaname]:
 
           # Does the child have a valid XML node attached?
@@ -811,7 +806,7 @@
           if child.xmlnode is None: continue
 
           current_choice = child.get_current_tree()
-          for tree_choice in child.l:
+          for tree_choice in child.get_choices():
             if tree_choice is current_choice: continue
 
     return bins
 
=== modified file 'diamond/diamond/tree.py'
--- diamond/diamond/tree.py	2011-07-26 14:04:07 +0000
+++ diamond/diamond/tree.py	2011-07-28 13:56:32 +0000
@@ -266,7 +266,7 @@
 
     sub_tree=etree.Element(self.name)
     
-    for key in self.attrs.keys():
+    for key in self.attrs:
       val = self.attrs[key]
       output_val = val[1]
       if output_val is not None:
@@ -275,17 +275,9 @@
     for child in self.children:
       if child.active is True:
         child.write_core(sub_tree)
-#      else:
-#        if child.cardinality == '?':
-#          root=etree.Element(self.name)
-#          child.write_core(root)
-#          comment_buffer = StringIO.StringIO(etree.tostring(root))
-#          comment_text = ("DIAMOND MAGIC COMMENT (inactive optional subtree %s):\n" % child.schemaname)
-#          comment_text = comment_text + base64.b64encode(bz2.compress(comment_buffer.getvalue()))
-#          sub_tree.append(etree.Comment(unicode(comment_text)))
         
     if self.data is not None:
-      sub_tree.text=(unicode(self.data))
+      sub_tree.text = unicode(self.data)
       
     if parent is not None:
       parent.append(sub_tree)
@@ -541,6 +533,9 @@
 
   def __str__(self):
     return self.get_display_name()
-   
+  
+  def __repr__(self):
+    return self.get_name_path()
+ 
 gobject.type_register(Tree)