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

Proposed by Cian Wilson
Status: Merged
Merged at revision: 520
Proposed branch: lp:~spud/spud/syntaxhighlighting
Merge into: lp:spud
Diff against target: 156 lines (+56/-17)
5 files modified
diamond/diamond/ (+8/-4)
diamond/diamond/ (+24/-7)
diamond/diamond/ (+19/-5)
schema/spud_base.rnc (+2/-1)
schema/spud_base.rng (+3/-0)
To merge this branch: bzr merge lp:~spud/spud/syntaxhighlighting
Reviewer Review Type Date Requested Status
Patrick Farrell Approve
Review via email:

Description of the change

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" },
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, and 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 and have been modified to is_code() and just checks for the existence of the type {"code"} attribute.

IMPORTANT QUESTION: what did the lines:
       lang = self.get_attr("language")
       if lang == "python":
         return True
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 checks whether the current node is_code() it then retrieves the language from get_code_language() in (adding this to 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.


To post a comment you must log in.
Revision history for this message
Patrick Farrell (pefarrell) wrote :

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?

Revision history for this message
Stephan Kramer (s-kramer) wrote :

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 updated
505. By Cian Wilson

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

Revision history for this message
Cian Wilson (cwilson) wrote :

Thanks for the reviews Patrick & Stephan.

diamond handles this change cleanly with a warning:

Warning: added xml attributes:

For this reason I don't think a specific script is necessary as tools/ 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" },
      element variables {
         ## A test variable
         element variable {
            attribute name { xsd:string },
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.

Revision history for this message
Cian Wilson (cwilson) wrote :

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

Revision history for this message
Florian Rathgeber (florian-rathgeber) wrote :

Any update on this?

Revision history for this message
Cian Wilson (cwilson) wrote :

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 updated
506. By Cian Wilson

Merging in changes from lp:spud

507. By Cian Wilson

Merging in changes from lp:spud.

Revision history for this message
Cian Wilson (cwilson) wrote :

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?

Revision history for this message
Patrick Farrell (pefarrell) wrote :

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?


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?

Revision history for this message
Cian Wilson (cwilson) wrote :

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.

Revision history for this message
Patrick Farrell (pefarrell) wrote :

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

Revision history for this message
Patrick Farrell (pefarrell) :
review: Approve
Revision history for this message
Cian Wilson (cwilson) wrote :

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

Revision history for this message
Patrick Farrell (pefarrell) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'diamond/diamond/'
2--- diamond/diamond/ 2011-08-01 14:13:10 +0000
3+++ diamond/diamond/ 2012-03-01 15:49:17 +0000
4@@ -124,15 +124,19 @@
5 buf = gtksourceview2.Buffer()
6 lang_manager = gtksourceview2.LanguageManager()
7 buf.set_highlight_matching_brackets(True)
8- if self.node is not None and self.node.is_python_code():
9- python = lang_manager.get_language("python")
10- buf.set_language(python)
11+ if self.node is not None and self.node.is_code():
12+ codelanguage = self.node.get_code_language();
13+ if codelanguage in lang_manager.get_language_ids():
14+ language = lang_manager.get_language(codelanguage)
15+ else:
16+ language = lang_manager.get_language("python")
17+ buf.set_language(language)
18 buf.set_highlight_syntax(True)
19 textview = gtksourceview2.View(buffer=buf)
20 textview.set_auto_indent(True)
21 textview.set_insert_spaces_instead_of_tabs(True)
22 textview.set_tab_width(2)
23- if self.node is not None and self.node.is_python_code():
24+ if self.node is not None and self.node.is_code():
25 textview.set_show_line_numbers(True)
26 font_desc = pango.FontDescription("monospace")
27 if font_desc:
29=== modified file 'diamond/diamond/'
30--- diamond/diamond/ 2012-02-29 11:44:14 +0000
31+++ diamond/diamond/ 2012-03-01 15:49:17 +0000
32@@ -184,16 +184,15 @@
34 return dim1 == dim2 and "symmetric" in self.child.attrs.keys() and self.child.attrs["symmetric"][1] == "true"
36- def is_python_code(self):
37+ def is_code(self):
38 """
39 Perform a series of tests on the current MixedTree, to determine if
40- it is intended to be used to store python code data.
41+ it is intended to be used to store code data.
42 """
44 try:
45- lang = self.get_attr("language")
46- if lang == "python":
47- return True
48+ lang = self.get_attr("language")
49+ if lang == "python":
50+ return True
51 except:
52 pass
54@@ -201,10 +200,28 @@
55 return False
57 if "type" in self.child.attrs.keys():
58- return self.child.attrs["type"][1] == "python"
59+ return self.child.attrs["type"][1] == "code"
61 return False
63+ def get_code_language(self):
64+ """
65+ Assuming this is a code snippet return the language.
66+ """
68+ if not self.is_code():
69+ return "python"
71+ try:
72+ lang = self.child.get_attr("language")
73+ return lang
74+ except:
75+ try:
76+ lang = self.get_attr("language")
77+ return lang
78+ except:
79+ return "python"
81 def get_name_path(self, leaf = True):
82 return self.parent.get_name_path(leaf)
85=== modified file 'diamond/diamond/'
86--- diamond/diamond/ 2011-11-30 15:25:44 +0000
87+++ diamond/diamond/ 2012-03-01 15:49:17 +0000
88@@ -443,21 +443,35 @@
89 def is_tensor(self, geometry_dim_tree):
90 return False
92- def is_python_code(self):
93+ def is_code(self):
94 """
95 Perform a series of tests on the current Tree, to determine if
96- it is intended to be used to store python code data.
97+ it is intended to be used to store code data.
98 """
100 try:
101- lang = self.selected_node.get_attr("language")
102- if lang == "python":
103- return True
104+ lang = self.get_attr("language")
105+ if lang == "python":
106+ return True
107 except:
108 pass
110 return False
112+ def get_code_language(self):
113+ """
114+ Assuming this is a code snippet return the language.
115+ """
117+ if not self.is_code():
118+ return "python"
120+ try:
121+ lang = self.get_attr("language")
122+ return lang
123+ except:
124+ return "python"
126 def get_display_name(self):
127 """
128 This is a fluidity hack, allowing the name displayed in the treeview on the
130=== modified file 'schema/spud_base.rnc'
131--- schema/spud_base.rnc 2008-05-23 15:38:50 +0000
132+++ schema/spud_base.rnc 2012-03-01 15:49:17 +0000
133@@ -34,7 +34,8 @@
134 python_code =
135 (
136 element string_value{
137- attribute type { "python" },
138+ attribute type { "code" },
139+ attribute language { "python" },
140 # Lines is a hint to the gui about the size of the text box.
141 # It is not an enforced limit on string length.
142 attribute lines { "20" },
144=== modified file 'schema/spud_base.rng'
145--- schema/spud_base.rng 2008-05-23 15:38:50 +0000
146+++ schema/spud_base.rng 2012-03-01 15:49:17 +0000
147@@ -46,6 +46,9 @@
148 <define name="python_code">
149 <element name="string_value">
150 <attribute name="type">
151+ <value>code</value>
152+ </attribute>
153+ <attribute name="language">
154 <value>python</value>
155 </attribute>
156 <!--


People subscribed via source and target branches