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

[Fraser Waters]

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

[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.

[lp:~spud/spud/copy-paste-fix]

419. By Fraser Waters on 2011-07-19

Small change to tree/choice write_core.

420. By Fraser Waters on 2011-07-19

Fix ups

421. By Fraser Waters on 2011-07-19

Merge

422. By Fraser Waters on 2011-07-19

Can reopen current file.

423. By Fraser Waters on 2011-07-19

Merge from trunk

424. By Fraser Waters on 2011-07-19

Paste needs active node

425. By Fraser Waters on 2011-07-27

Factor out handling clipboard for other widgets.

426. By Fraser Waters on 2011-07-27

Missed a :

427. By Fraser Waters on 2011-07-27

Renamed Choice.l to Choice.choices

428. By Fraser Waters on 2011-07-27

Fix for paste handling choices.

429. By Fraser Waters on 2011-07-27

non xml paste will not ask to delete trees.

430. By Fraser Waters on 2011-07-27

Start on changing schema.read

431. By Fraser Waters on 2011-07-28

Merge from trunk.

432. By Fraser Waters on 2011-07-28

Schema.read handles starting on a Choice

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-27 13:28:56 +0000
@@ -31,21 +31,21 @@
   __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, 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.cardinality = cardinality
@@ -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):
     """

=== modified file 'diamond/diamond/interface.py'
--- diamond/diamond/interface.py	2011-07-26 16:12:33 +0000
+++ diamond/diamond/interface.py	2011-07-27 13:28:56 +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,47 @@
     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)
+      oldtree = self.treestore.get_value(self.selected_iter, 1)
 
     if node != None:
-
-      newnode = self.s.read(ios, node)
-
-      if newnode is None:
+  
+      expand = not node.active
+      if expand:
+        self.expand_tree(self.selected_iter)
+
+      newtree = self.s.read(ios, oldtree)
+
+      if newtree 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 oldtree.parent is not None:
+        newtree.set_parent(oldtree.parent)
+        if isinstance(node, tree.Tree):
+          children = node.parent.get_children()
+          children.insert(children.index(oldtree), newtree)
+          children.remove(oldtree)
+ 
+      if isinstance(node, choice.Choice):
+        choices = node.get_choices()
+        choices.insert(choices.index(oldtree), newtree)
+        choices.remove(oldtree)
+        self.set_treestore(self.selected_iter, [node], True, True)
+      else:
+        self.set_treestore(self.selected_iter, [newtree], True, True)
+
+      newtree.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 +733,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 +924,7 @@
     if replace:
       replacediter = iter
       iter = self.treestore.iter_parent(replacediter)
-    else:
+    else: 
       self.remove_children(iter)
   
     for t in new_tree:
@@ -1119,7 +1136,7 @@
 
     return
   
-  def collapse_tree(self, iter):
+  def collapse_tree(self, iter, confirm = True):
     """
     Collapses part of the tree.
     """
@@ -1147,7 +1164,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 +1172,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-27 13:28:56 +0000
@@ -162,12 +162,9 @@
     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
 
@@ -395,14 +392,11 @@
       # 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 = {}
@@ -564,7 +558,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 +692,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 +719,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 +797,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 +805,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-27 13:28:56 +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)