Merge lp:~spud/spud/syntaxhighlighting into lp:spud

[Cian Wilson]

I'd like to be able to add code snippets to my options file and use syntax highlighting other than python. This is my proposal to do this but as I'm messing around in files I've never looked at before I'd appreciate feedback.

This merge would change the current python_code in spud_base.rnc from:
python_code =
   (
      element string_value{
         attribute type { "python" },
         ...
   )
to:
python_code =
   (
      element string_value{
         attribute type { "code" },
         attribute language { "python" },
         ...
   )
i.e. type is now "code" and a new attribute language {"python"} has been added.

In combination with changes to mixedtree.py, tree.py and datawidget.py this would allow developers to add various other code types into their schemas (so long as they are available in gtksourceview2). For example, I've added:
cpp_code =
  (
    element string_value{
      attribute type { "code" },
      attribute language { "cpp" },
      ...
  )
to my schema.

To achieve this is_python_code() in mixedtree.py and tree.py have been modified to is_code() and just checks for the existence of the type {"code"} attribute.

IMPORTANT QUESTION: what did the lines:
    try:
       lang = self.get_attr("language")
       if lang == "python":
         return True
    except:
      pass
in is_python_code() do? I couldn't see how they did anything, as there's no "language" attribute in the spud_base schema so I've removed them here but I'd like to double check as I'm probably missing something.

After add_text_view() in datawidget.py checks whether the current node is_code() it then retrieves the language from get_code_language() in mixedtree.py (adding this to tree.py seemed superfluous as is_code() now always returns false here). In get_code_language() there's a lot of defaulting to "python" to try to smooth the changes in the schema with old files. add_text_view() also defaults to "python" if the language type selected isn't available.

The major disadvantage of this merge is the change to the schema and hence modification of most options files. Hopefully the defaulting will smooth this for users, though it would also be good to remove the defaults with time. I'd appreciate any input on a method of achieving this without such difficulties.

Cheers,
Cian

[Patrick Farrell (pefarrell)]

I think syntax highlighting of other languages is desirable and important, so thanks for the contribution.

I agree with you: I think the self.get_attr("language") did nothing. It is probably a leftover from some previous way of denoting python code in the schema; I think you are correct in deleting it.

I think that if you're going to change the schema in such a way that a currently valid flml becomes invalid, then you should write a script that converts from type == "python" to (type, language) == ("code", "python"). I'm not sure how to manage the transition, and I'm not sure we'd want this to go in before release. David, Stephan, any thoughts?

[Stephan Kramer (s-kramer)]

I think the "language" attribute is used for the .xml test_options.rng schema. Not in the spud base language indeed, so more a schema specific hack. So do we want to change the .xml specification as well? I guess it wasn't really written with spud in mind. It might be nice to keep the current check as a transitional thing (if we do decide to change .xml)

Is it the case that diamond will be able to handle this smoothly when handling old flmls? As in, would it simply change the attributes when opening up, without loosing any elements? In that case, yes a simple transition script should be fine. I presume spud itself doesn't really care about the attributes, and just treats it as a string?

I agree this is probably not something we want to push through before the release. If we do it after, we also have to make sure the updated diamond still works correctly when using the release schema (with the old python_code definition).

But in general: a useful addition.

[lp:~spud/spud/syntaxhighlighting]

505. By Cian Wilson on 2011-10-31

Adding back in test_options schema specific hack for comment and thought.

[Cian Wilson (cwilson)]

Thanks for the reviews Patrick & Stephan.

diamond handles this change cleanly with a warning:

Warning: added xml attributes:
string_value/language

For this reason I don't think a specific script is necessary as tools/update_options.py handles it automatically.

Stephan, you're right. 'language="python"' is used in test_options.rnc and removing it has stopped all syntax highlighting on test xmls. Luckily I can add it back in without much conflict (as one is an attribute of the current node and the other is an attribute of the child of the node, thanks to string_value - see updated diff). Obviously however this is still a schema specific hack. I guess the right answer is to change the test_options.rnc from:
      element variables {
         ## A test variable
         element variable {
            attribute name { xsd:string },
            attribute language { "python" },
            xsd:string,
            comment
         }*
to:
      element variables {
         ## A test variable
         element variable {
            attribute name { xsd:string },
            python_code
         }*
with a similar change for the test element. This change however would definitely require a script as naively opening it in diamond deletes all the variables and tests! Thoughts...?

In terms of release, I wasn't really suggesting that this go in. In fact, I thought the final deadline for changes had passed. Still, it's a good point about how diamond will cope pre and post release in terms of back compatibility. It's further complicated by the dependency on where people are getting their diamond from. Not sure what to suggest here other than that people may have to put up with warnings about adding or deleting attributes depending on whether their diamond is pre or post release compatible.

[Cian Wilson (cwilson)]

Of course the bit I was missing about modifying test_options above is that regressiontest.py would also have to change. Definitely not an easy change to make back/forward/sideways compatible.

[Florian Rathgeber (florian-rathgeber)]

Any update on this?

[Cian Wilson (cwilson)]

Well, I'd still like it in and, assuming people don't mind the test specific hack staying in as it's really a separate issue, I don't think there's much stopping it from going into the spud trunk. The complications will arise when fluidity and the packages are updated to the new version (fluidity options files will have to be updated). So, when this happens we'll just have to remember to run update_options as well but then as diamond in the trunk is currently broken such a release/merge into fluidity shouldn't happen until this is also fixed.

[lp:~spud/spud/syntaxhighlighting]

506. By Cian Wilson on 2012-02-28

Merging in changes from lp:spud

507. By Cian Wilson on 2012-03-01

Merging in changes from lp:spud.

[Cian Wilson (cwilson)]

Having looked at the test_options issue I think it's easier to merge this branch first then worry about fixing those issues later (otherwise the changes to testharness are going to have to be implemented twice).

Anyone willing to rereview this?

[Patrick Farrell (pefarrell)]

I have to confess to being a bit confused. I like this commit, but before approving can you just list what steps we have to take to get everything working, including the test_options issue?

i.e.

a) Merge this commit to the spud trunk
b) Merge the spud trunk into fluidity trunk
c) update_options
d) Change the test_options schema?
e) update_options?
f) Change the test harness?

[Cian Wilson (cwilson)]

The test_options issue is completely orthogonal to this commit. The hack that makes test xmls work will still work after this merge. So, (a), (b) and (c) are correct but (d), (e) and (f) are separate.

Only question is whether the appropriate moment to do (c) is after (b) or when a new spud package is released (kind of depends on where buildbot gets its spud_base.rng from), though these are likely to be coincident anyway.

[Patrick Farrell (pefarrell)]

I'm happy for this to be merged. Can you coordinate with Tim what needs to be done for the other bits?

[Patrick Farrell (pefarrell)]

review: Approve

[Cian Wilson (cwilson)]

Sure, does Tim take care of merging the spud branch with the fluidity version as well as packaging?

[Patrick Farrell (pefarrell)]

Yep, I think it's best if he coordinates it all. I wonder does he get these emails?

=== modified file 'diamond/diamond/datawidget.py'
--- diamond/diamond/datawidget.py	2011-08-01 14:13:10 +0000
+++ diamond/diamond/datawidget.py	2012-03-01 15:49:17 +0000
@@ -124,15 +124,19 @@
       buf = gtksourceview2.Buffer()
       lang_manager = gtksourceview2.LanguageManager()
       buf.set_highlight_matching_brackets(True)
-      if self.node is not None and self.node.is_python_code():
-        python = lang_manager.get_language("python")
-        buf.set_language(python)
+      if self.node is not None and self.node.is_code():
+        codelanguage = self.node.get_code_language();
+        if codelanguage in lang_manager.get_language_ids():
+          language = lang_manager.get_language(codelanguage)
+        else:
+          language = lang_manager.get_language("python")
+        buf.set_language(language)
         buf.set_highlight_syntax(True)
       textview = gtksourceview2.View(buffer=buf)
       textview.set_auto_indent(True)
       textview.set_insert_spaces_instead_of_tabs(True)
       textview.set_tab_width(2)
-      if self.node is not None and self.node.is_python_code():
+      if self.node is not None and self.node.is_code():
         textview.set_show_line_numbers(True)
         font_desc = pango.FontDescription("monospace")
         if font_desc:

=== modified file 'diamond/diamond/mixedtree.py'
--- diamond/diamond/mixedtree.py	2012-02-29 11:44:14 +0000
+++ diamond/diamond/mixedtree.py	2012-03-01 15:49:17 +0000
@@ -184,16 +184,15 @@
 
     return dim1 == dim2 and "symmetric" in self.child.attrs.keys() and self.child.attrs["symmetric"][1] == "true"
 
-  def is_python_code(self):
+  def is_code(self):
     """
     Perform a series of tests on the current MixedTree, to determine if
-    it is intended to be used to store python code data.
+    it is intended to be used to store code data.
     """
-
     try:
-       lang = self.get_attr("language")
-       if lang == "python":
-         return True
+      lang = self.get_attr("language")
+      if lang == "python":
+        return True
     except:
       pass
 
@@ -201,10 +200,28 @@
       return False
 
     if "type" in self.child.attrs.keys():
-      return self.child.attrs["type"][1] == "python"
+      return self.child.attrs["type"][1] == "code"
     
     return False
 
+  def get_code_language(self):
+    """
+    Assuming this is a code snippet return the language.
+    """
+
+    if not self.is_code():
+      return "python"
+
+    try:
+      lang = self.child.get_attr("language")
+      return lang
+    except:
+      try:
+        lang = self.get_attr("language")
+        return lang
+      except:   
+        return "python"
+
   def get_name_path(self, leaf = True):
     return self.parent.get_name_path(leaf)
 

=== modified file 'diamond/diamond/tree.py'
--- diamond/diamond/tree.py	2011-11-30 15:25:44 +0000
+++ diamond/diamond/tree.py	2012-03-01 15:49:17 +0000
@@ -443,21 +443,35 @@
   def is_tensor(self, geometry_dim_tree):
     return False
 
-  def is_python_code(self):
+  def is_code(self):
     """
     Perform a series of tests on the current Tree, to determine if
-    it is intended to be used to store python code data.
+    it is intended to be used to store code data.
     """
 
     try:
-       lang = self.selected_node.get_attr("language")
-       if lang == "python":
-         return True
+      lang = self.get_attr("language")
+      if lang == "python":
+        return True
     except:
       pass
 
     return False
 
+  def get_code_language(self):
+    """
+    Assuming this is a code snippet return the language.
+    """
+
+    if not self.is_code():
+      return "python"
+
+    try:
+      lang = self.get_attr("language")
+      return lang
+    except:   
+      return "python"
+
   def get_display_name(self):
     """
     This is a fluidity hack, allowing the name displayed in the treeview on the

=== modified file 'schema/spud_base.rnc'
--- schema/spud_base.rnc	2008-05-23 15:38:50 +0000
+++ schema/spud_base.rnc	2012-03-01 15:49:17 +0000
@@ -34,7 +34,8 @@
 python_code =
    (
       element string_value{
-         attribute type { "python" },
+         attribute type { "code" },
+         attribute language { "python" },
          # Lines is a hint to the gui about the size of the text box.
          # It is not an enforced limit on string length.
          attribute lines { "20" },

=== modified file 'schema/spud_base.rng'
--- schema/spud_base.rng	2008-05-23 15:38:50 +0000
+++ schema/spud_base.rng	2012-03-01 15:49:17 +0000
@@ -46,6 +46,9 @@
   <define name="python_code">
     <element name="string_value">
       <attribute name="type">
+        <value>code</value>
+      </attribute>
+      <attribute name="language">
         <value>python</value>
       </attribute>
       <!--