Merge lp:~spud/spud/get-set-manager into lp:spud

[Patrick Farrell]

Changes to support accessing spud data inside python diagnostic fields in Fluidity.

This commit adds two new semi-public functions. I don't want to make these fully public (i.e. documented in the manual etc.) because they are a bit of a hack.

The two functions are spud_get_manager and spud_set_manager. These get and set the core internal data structure of libspud, the manager.options of the OptionManager class.

Why is this necessary? Here's what happens chronologically when we have python code that imports libspud:

* Before the fluidity main, the spud constructor is called on the OptionManager object. It allocates options.manager.
* Fluidity calls load_options and that populates the options.manager.
* We use it happily.
* When we import libspud, it calls the spud constructor *on the same OptionManager object*. It allocates options.manager AGAIN, losing the pointer to where we had our data stored.
* That options.manager has no data, and fluidity soon dies because of a want of options.
* At exit, the destructor gets called TWICE on the same fecking OptionManager.

Now, we modify this sequence of events to:

* Before the fluidity main, the spud constructor is called on the OptionManager object. It allocates options.manager.
* Fluidity calls load_options and that populates the options.manager.
* We use it happily.
* Before we load the Python interpreter, we take a pointer to the current good options.manager via spud_get_manager. We store that pointer in a capsule as fluidity_api._spud_manager.
* When we import libspud, it calls the spud constructor *on the same OptionManager object*. It allocates options.manager AGAIN.
* In the libspud python binding constructor, we fetch the old pointer to the good options manager from the capsule, and set it using spud_set_manager.
* Now we can happily access options from Python, and we haven't lost them from Fortran either.
* At exit, the destructor is called twice, but Patrick has been clever and added a "deallocated" flag so that we only deallocate things once.

Once this is accepted, I will propose the libspud C python bindings for addition, to replace/compliment the ctypes python binding. Once that's done, I will propose the merging of the editions on the fluidity side.

[Stephan Kramer (s-kramer)]

If I understand the story above correctly, and I'm sure that I don't :), "import libspud" will still create a new options manager, except that we use this work around in the libspud python binding to swap it out with the manager we actually want to use. What will then happen with the new manager we're not using? Does it ever get deallocated? Will we not end up with a new unused options manager for every "import spud" that gets evaluated?

[lp:~spud/spud/get-set-manager]

429. By Patrick Farrell on 2011-07-28

Clean up the previously leaked options.manager.

[Patrick Farrell (pefarrell)]

That old manager is leaked. If desired, we could make the spud_set_manager free the old one. However, I wasn't worried about it because it is very small, and more importantly does not grow; even if
multiple scripts call import libspud many times, it only actually gets imported by python once (and so the Spud object constructor only gets called twice, no matter how much the python binding is used).

However, I added the call to delete options.manager, so even that one instance is no longer leaked.

[Stephan Kramer (s-kramer)]

Very good - just thought about it because, afaik we trash the python environment for every python evaluation. Looks good to me

review: Approve

[lp:~spud/spud/get-set-manager]

430. By Patrick Farrell on 2011-07-29

Merge from the trunk

=== modified file 'diamond/diamond/interface.py'
--- diamond/diamond/interface.py	2011-07-28 14:52:55 +0000
+++ diamond/diamond/interface.py	2011-07-29 09:21:43 +0000
@@ -283,6 +283,9 @@
     # 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:
@@ -635,19 +638,12 @@
     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 self._handle_clipboard(widget, "copy"):
-      return
+    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 isinstance(self.selected_node, mixedtree.MixedTree):
       node = self.selected_node.parent
@@ -666,39 +662,28 @@
     return
 
   def on_paste(self, widget=None):
-    if self._handle_clipboard(widget, "paste"):
-      return
+    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
 
     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, 0)
+      node = self.treestore.get_value(self.selected_iter, 1)
 
     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 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()
+      if not node.active:
+        self.expand_tree(self.selected_iter)
 
       # Extract and display validation errors
       lost_eles, added_eles, lost_attrs, added_attrs = self.s.read_errors()
@@ -725,6 +710,12 @@
         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
 
@@ -918,7 +909,7 @@
     if replace:
       replacediter = iter
       iter = self.treestore.iter_parent(replacediter)
-    else: 
+    else:
       self.remove_children(iter)
   
     for t in new_tree:
@@ -1130,7 +1121,7 @@
 
     return
   
-  def collapse_tree(self, iter, confirm = True):
+  def collapse_tree(self, iter):
     """
     Collapses part of the tree.
     """
@@ -1158,7 +1149,7 @@
         self.set_saved(False)
         self.remove_children(iter)
       else:
-        self.delete_tree(iter, confirm)
+        self.delete_tree(iter)
 
     elif choice_or_tree.cardinality == "+":
       count = parent_tree.count_children_by_schemaname(choice_or_tree.schemaname)
@@ -1166,23 +1157,20 @@
         # do nothing
         return
       else: # count > 2
-        self.delete_tree(iter, confirm)
+        self.delete_tree(iter)
   
     parent_tree.recompute_validity()
     self.treeview.queue_draw()
     return
 
-  def delete_tree(self, iter, confirm):
+  def delete_tree(self, iter):
     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)
 
-    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:
+    confirm = dialogs.prompt(self.main_window, "Are you sure you want to delete this node?")
+    if confirm == gtk.RESPONSE_YES:
       parent_tree.delete_child_by_ref(choice_or_tree)
       self.remove_children(iter)
       self.treestore.remove(iter)

=== modified file 'diamond/diamond/tree.py'
--- diamond/diamond/tree.py	2011-07-27 13:53:35 +0000
+++ diamond/diamond/tree.py	2011-07-29 09:21:43 +0000
@@ -266,7 +266,7 @@
 
     sub_tree=etree.Element(self.name)
     
-    for key in self.attrs:
+    for key in self.attrs.keys():
       val = self.attrs[key]
       output_val = val[1]
       if output_val is not None:
@@ -275,9 +275,17 @@
     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)
@@ -533,9 +541,6 @@
 
   def __str__(self):
     return self.get_display_name()
-  
-  def __repr__(self):
-    return self.get_name_path()
- 
+   
 gobject.type_register(Tree)
 

=== modified file 'include/spud'
--- include/spud	2011-07-11 15:25:35 +0000
+++ include/spud	2011-07-29 09:21:43 +0000
@@ -52,6 +52,8 @@
     public:
     
       static void clear_options();
+      static void* get_manager();
+      static void set_manager(void* m);
 
       static OptionError load_options(const std::string& filename);
       static OptionError write_options(const std::string& filename);
@@ -404,6 +406,7 @@
           
       };
       
+      bool deallocated;
       Option* options;
       
   };
@@ -412,6 +415,15 @@
     OptionManager::clear_options();
   }
 
+  inline void* get_manager(){
+    return OptionManager::get_manager();
+  }
+
+  inline void set_manager(void* m){
+    OptionManager::set_manager(m);
+    return;
+  }
+
   inline OptionError load_options(const std::string& filename){
     return OptionManager::load_options(filename);
   }

=== modified file 'include/spud.h'
--- include/spud.h	2011-07-11 15:25:35 +0000
+++ include/spud.h	2011-07-29 09:21:43 +0000
@@ -36,6 +36,8 @@
 #endif
 
   void spud_clear_options();
+  void* spud_get_manager();
+  void spud_set_manager(void* m);
   
   int spud_load_options(const char* filename, const int filename_len);
   int spud_write_options(const char* filename, const int filename_len);

=== modified file 'src/spud.cpp'
--- src/spud.cpp	2011-07-11 15:25:35 +0000
+++ src/spud.cpp	2011-07-29 09:21:43 +0000
@@ -41,6 +41,16 @@
     return;
   }
 
+  void* OptionManager::get_manager() {
+    return (void*) manager.options;
+  }
+
+  void OptionManager::set_manager(void* m) {
+    delete manager.options;
+    manager.options = (Spud::OptionManager::Option*) m;
+    return;
+  }
+
   OptionError OptionManager::load_options(const string& filename){
     return manager.options->load_options(filename);
   }
@@ -543,6 +553,7 @@
 
   OptionManager::OptionManager(){
     options = new Option();
+    deallocated = false;
 
     return;
   }
@@ -553,7 +564,11 @@
   }
 
   OptionManager::~OptionManager(){
-    delete options;
+    if (!deallocated)
+    {
+      delete options;
+      deallocated = true;
+    }
 
     return;
   }

=== modified file 'src/spud_interfaces.cpp'
--- src/spud_interfaces.cpp	2011-07-11 15:25:35 +0000
+++ src/spud_interfaces.cpp	2011-07-29 09:21:43 +0000
@@ -40,6 +40,15 @@
     return;
   }
 
+  void* spud_get_manager(){
+    return get_manager();
+  }
+
+  void spud_set_manager(void* m){
+    set_manager(m);
+    return;
+  }
+
   int spud_load_options(const char* filename, const int filename_len)
   {
     return load_options(string(filename, filename_len));