Merge lp:~novalis/inkscape/fix-167335 into lp:~inkscape.dev/inkscape/trunk

Proposed by David Turner on 2013-10-10
Status: Work in progress
Proposed branch: lp:~novalis/inkscape/fix-167335
Merge into: lp:~inkscape.dev/inkscape/trunk
Diff against target: 329 lines (+178/-19)
9 files modified
share/attributes/svgprops (+1/-1)
src/conditions.cpp (+4/-0)
src/extension/internal/svg.cpp (+152/-14)
src/sp-filter.cpp (+1/-1)
src/sp-flowtext.h (+3/-0)
src/sp-font.cpp (+1/-1)
src/sp-item-group.h (+1/-1)
src/sp-object.h (+14/-0)
src/sp-text.cpp (+1/-1)
To merge this branch: bzr merge lp:~novalis/inkscape/fix-167335
Reviewer Review Type Date Requested Status
Martin Owens full 2013-10-10 Needs Fixing on 2014-03-28
Review via email: mp+190297@code.launchpad.net

Description of the change

This appears to fix the problem.

To post a comment you must log in.
David Turner (novalis) wrote :

Oops, hang on a sec -- I just realized I left some cruft in here while fighting with bzr.

David Turner (novalis) wrote :

Yeah, that's better.

Tavmjong Bah (tavmjong-free) wrote :

Good to see this being fixed.

Note that SVG 2 will have a new way to handle flowed text that is
compatible with CSS and thus more likely to get adopted. It also has a
natural fallback mechanism for browsers that don't support SVG 2.

On Thu, 2013-10-10 at 06:00 +0000, David Turner wrote:
> Yeah, that's better.

One small quibble: shouldn't the visit() function be in sp-object.h
rather than root.h? You don't need to call it with the root object.

Krzysztof Kosinski (tweenk) wrote :

I would prefer this to be fixed by adding the fallback elements as a branch in svg:switch, but this is more complicated, and this simple approach should work.

I have 2 questions, to verify that this is useful:
1. Does this work only when saving as Plain SVG?
2. If you 'Save a copy' as plain SVG, is the original document unmodified?

David Turner (novalis) wrote :

Hm. I think there is a problem with this patch. I'm going to fix it (and move the visit() function).

The reason I didn't use a switch element is because flowroot is not actually in any recent version of SVG standard. Textarea is in svg 1.2 tiny, but it doesn't support irregular areas. Since flowroot isn't likely to be widely supported, I didn't think it was worth putting in the switch.

Krzysztof Kosinski (tweenk) wrote :

The point of putting the flowRoot in the switch is so that documents saved as Inkscape SVG that contain flowtext will show correctly in e.g. Firefox and MediaWiki. It should not be necessary to save as Plain SVG to actually get a valid SVG.

David Turner (novalis) wrote :

Hm. OK, I can probably do that.

David Turner (novalis) wrote :

I just uncommitted and recommitted -- I'm not sure whether that is the right thing to do (I'm used to git). Anyway, I can see the new code here:

http://bazaar.launchpad.net/~novalis/inkscape/fix-167335/revision/12677

su_v (suv-lp) wrote :

On 2013-10-27 21:16 +0200, <email address hidden> wrote:
> The proposal to merge lp:~novalis/inkscape/fix-167335 into lp:inkscape has been updated.
>
> Status: Needs review => Merged

In which revision was this actually merged into inkscape trunk
(lp:inkscape)?

David Turner (novalis) wrote :

suv: "In which revision was this actually merged into inkscape trunk (lp:inkscape)?"

I have no idea why this message was generated. I never did anything merge-related (as far as I know), and I hope that launchpad or bzr or whatever didn't either. All I did was uncommit and commit.

David Turner (novalis) wrote :

Actually, I just noticed that apparently this saving procedure also screws up filters. That's because sp-filter.cpp:223 uses svg:this (as sp-text did before I changed it). What is the reasoning behind svg:this? It seems to be totally bogus. I would just fix sp-filter and sp-font too, but I would like to understand why they were like this in the first place.

Markus Engel (engelmarkus) wrote :

The reason behind "this" is that I renamed some variables and the IDE seems to have screwed up this. This change was introduced in revisions >= 12532. Please fix any of these when you find them.

Martin Owens (doctormo) wrote :

David Turner: you might be interested in this documentation: http://staging.inkscape.org/en/develop/using-git-repositories/

David Turner (novalis) wrote :

I guess I just fixed the remaining two instances of svg:this.

su_v (suv-lp) wrote :

I have no idea what has happened in your branch and why launchpad apparently does not create a preview diff anymore - trying to apply the diff from r12677 of your branch to local trunk checkout fails with a conflict:

> $ patch -p0 --dry-run < ../12677_12676.diff

> patching file src/sp-item-group.h
> Hunk #1 FAILED at 55.
> 1 out of 1 hunk FAILED -- saving rejects to file src/sp-item-group.h.rej

Trying to merge the branch into a local checkout of trunk fails with the same text conflict:

> $ bzr merge --preview lp:~novalis/inkscape/fix-167335
> Text conflict in src/sp-item-group.h

lp:~novalis/inkscape/fix-167335 updated on 2013-10-28
12738. By David Turner on 2013-10-28

when saving, convert flowed text to plain text with a switch

Martin Owens (doctormo) wrote :

I've done a full review of the code and it's features:

The code looks good, using the right syntax as much as I can fathom and doesn't add anything too bad.

The method to fix the bug is interesting and I wanted to see if the fix worked:

 1. Open an existing flowText svg: OK
 2. Save svg back to a new file: FAILED (SEGFAULT)
  a. Fixed by commenting out line 180 is the diff below.
  b. This then caused a SEGFAULT after the svg had been saved
 3. The new svg from the old file produces better and more compatible results: NO
  a. SVG file in text editor shows switch statement: YES
  b. New svg with switch statement improved in librsvg: NO, text does not appear
  c. New svg with switch statement improved in firefox: NO, text does not appear
  d. New svg with switch statement improved in chrome: NO, text does not appear
 4. Create new svg file and add a flowText to the document: OK
 5. Save document and test in browsers: FAILED (no text shown again)
 6. Opening svg file with switch statement for editing: OK
 7. flowText object appears as flowText object: NO (shows as conditional group)

Thus this branch does not yet provide a fix for the issue of flowedText and can not be recommended for Merge.

review: Needs Fixing (full)
David Turner (novalis) wrote :

Unsurprisingly, I haven't looked at this code in a while. I wanted to test it out, so I grabbed an example from the original bug report: https://bugs.launchpad.net/inkscape/+bug/167335/+attachment/173581/+files/sample.svg

First, I tested with an unmodified version of lp:inkscape (without my patch), and I found that a save/load cycle produces visible changes. Then I tested with my patch, and found the same changes, but no segfault.

Can you (a) upload the file which is producing a segfault and (b) tell me what branch I should actually be working off of?

Martin Owens (doctormo) wrote :

The linked file doesn't contain either a flowRoot or a switch statement. Just text objects and tspans.

Was I testing the wrong thing to make an svg with flowRoots which then saved out to an svg with a switch statement?

David Turner (novalis) wrote :

The linked file does contain a flowRoot.

Martin Owens (doctormo) wrote :

Ah, another firefox hatchet job. Sorry, I should learn to wget svg files instead of letting firefox butcher them first.

Can you make a working svg with switches from the flowRoot example you linked?

Unmerged revisions

12738. By David Turner on 2013-10-28

when saving, convert flowed text to plain text with a switch

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'share/attributes/svgprops'
2--- share/attributes/svgprops 2012-10-08 10:58:24 +0000
3+++ share/attributes/svgprops 2013-10-28 20:46:17 +0000
4@@ -278,7 +278,7 @@
5
6 "requiredExtensions" - "a","altGlyph","animate","animateColor","animateMotion","animateTransform","circle","clipPath","cursor","defs","ellipse","foreignObject","g","image","line","mask","path","pattern","polygon","polyline","rect","set","svg","switch","text","textPath","tref","tspan","use"
7
8-"requiredFeatures" - "a","altGlyph","animate","animateColor","animateMotion","animateTransform","circle","clipPath","cursor","defs","ellipse","foreignObject","g","image","line","mask","path","pattern","polygon","polyline","rect","set","svg","switch","text","textPath","tref","tspan","use"
9+"requiredFeatures" - "a","altGlyph","animate","animateColor","animateMotion","animateTransform","circle","clipPath","cursor","defs","ellipse","flowRoot","foreignObject","g","image","line","mask","path","pattern","polygon","polyline","rect","set","svg","switch","text","textPath","tref","tspan","use"
10
11 "restart" - "animate","animateColor","animateMotion","animateTransform","set"
12
13
14=== modified file 'src/conditions.cpp'
15--- src/conditions.cpp 2012-02-15 20:50:04 +0000
16+++ src/conditions.cpp 2013-10-28 20:46:17 +0000
17@@ -140,6 +140,7 @@
18 return parts;
19 }
20
21+#define INKSCAPEFEATURE "http://www.inkscape.org/feature#"
22 #define SVG11FEATURE "http://www.w3.org/TR/SVG11/feature#"
23 #define SVG10FEATURE "org.w3c."
24
25@@ -250,6 +251,9 @@
26 found = strstr(value, SVG10FEATURE);
27 if ( value == found )
28 return evaluateSVG10Feature(found + strlen(SVG10FEATURE));
29+ found = strstr(value, INKSCAPEFEATURE);
30+ if ( value == found )
31+ return true;
32 return false;
33 }
34
35
36=== modified file 'src/extension/internal/svg.cpp'
37--- src/extension/internal/svg.cpp 2012-10-04 01:47:32 +0000
38+++ src/extension/internal/svg.cpp 2013-10-28 20:46:17 +0000
39@@ -24,8 +24,14 @@
40 #include "extension/output.h"
41 #include <vector>
42 #include "xml/attribute-record.h"
43+#include "xml/event-fns.h"
44+#include "document.h"
45+#include "factory.h"
46+#include "sp-factory.h"
47 #include "sp-root.h"
48-#include "document.h"
49+#include "sp-switch.h"
50+#include "sp-text.h"
51+#include "sp-flowtext.h"
52
53 #ifdef WITH_GNOME_VFS
54 # include <libgnomevfs/gnome-vfs.h>
55@@ -42,7 +48,7 @@
56 using Inkscape::XML::AttributeRecord;
57 using Inkscape::XML::Node;
58
59-
60+void flowText(SPDocument* orig, SPDocument* doc);
61
62 static void pruneExtendedAttributes( Inkscape::XML::Node *repr )
63 {
64@@ -234,30 +240,162 @@
65 || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVG_INKSCAPE)
66 || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVGZ_INKSCAPE));
67
68- Inkscape::XML::Document *rdoc = NULL;
69 Inkscape::XML::Node *repr = NULL;
70- if (exportExtensions) {
71- repr = doc->getReprRoot();
72- } else {
73- rdoc = sp_repr_document_new ("svg:svg");
74- repr = rdoc->root();
75- repr = doc->getRoot()->updateRepr(rdoc, repr, SP_OBJECT_WRITE_BUILD);
76-
77+ SPDocument* newDoc = NULL;
78+
79+ newDoc = SPDocument::createNewDoc(0, 0, true);
80+ repr = doc->getRoot()->updateRepr(newDoc->getReprDoc(),
81+ newDoc->getReprRoot(), SP_OBJECT_WRITE_BUILD);
82+
83+ if (!exportExtensions) {
84 pruneExtendedAttributes(repr);
85 }
86
87+ flowText(doc, newDoc);
88+
89 if (!sp_repr_save_rebased_file(repr->document(), filename, SP_SVG_NS_URI,
90 doc->getBase(), filename)) {
91 throw Inkscape::Extension::Output::save_failed();
92 }
93
94- if (!exportExtensions) {
95- Inkscape::GC::release(rdoc);
96- }
97-
98+ delete newDoc;
99 return;
100 }
101
102+class FlowTextReplacer {
103+
104+private:
105+ SPDocument* document;
106+
107+ //we need this nonsense so that we can get a correctly-laid-out flowtext
108+ SPDocument* oldDocument;
109+ Inkscape::XML::Document* xml_document;
110+
111+ Inkscape::XML::Node* getBasicText(SPFlowtext* flowtext) const {
112+ //This is probably an insane way to get a SPFlowtext with the correct layout, but I
113+ //can't figure out another way.
114+ SPObject* original_object = oldDocument->getObjectById(flowtext->getId());
115+ SPFlowtext* original_flowtext = SP_FLOWTEXT(original_object);
116+ flowtext->layout = original_flowtext->layout;
117+ Inkscape::XML::Node* basic_text = flowtext->getAsText();
118+ return basic_text;
119+ }
120+
121+ SPItem* makeBasicTextItem(Inkscape::XML::Node* basic_text,
122+ SPItem* item) const {
123+ SPItem* new_item =
124+ reinterpret_cast<SPItem*>(SPFactory::instance().createObject(
125+ NodeTraits::get_type_string(*basic_text)));
126+ new_item->invoke_build(document, basic_text, TRUE);
127+ new_item->doWriteTransform(basic_text, item->transform);
128+ new_item->updateRepr();
129+ return new_item;
130+ }
131+
132+public:
133+
134+ FlowTextReplacer (SPDocument* oldDocument, SPDocument* document) :
135+ document (document),
136+ oldDocument (oldDocument) {
137+ xml_document = document->getReprDoc();
138+ }
139+
140+ bool operator () (SPObject* obj) const {
141+ SPItem* item = SP_ITEM(obj);
142+ if (!item)
143+ return true;
144+
145+ /* This might be a switch or a flowtext. If it's a flowtext, we need to wrap it
146+ * in a switch. If it's a switch with <flowtext, text>, we need to update the
147+ * text.
148+ */
149+
150+ if (SP_IS_FLOWTEXT(item)) {
151+ SPFlowtext* flowtext = SP_FLOWTEXT(item);
152+
153+ Inkscape::XML::Node* basic_text = getBasicText(flowtext);
154+ if (!basic_text) {
155+ //this shouldn't happen, but when it does, there's
156+ //nothing we can do about it.
157+ g_warning("Could not get basic text from flowed text for %s", item->getId());
158+ return false;
159+ }
160+
161+ Inkscape::XML::Node* flowtext_repr = flowtext->getRepr();
162+ Inkscape::XML::Node* parent = flowtext_repr->parent();
163+
164+ Inkscape::XML::Node *switch_repr = xml_document->createElement("svg:switch");
165+ parent->addChild(switch_repr, flowtext_repr);
166+
167+ //duplicate the flowtext so that we can place it into the switch
168+ Inkscape::XML::Node* flowtext_dup = flowtext_repr->duplicate(xml_document);
169+
170+ flowtext_dup->setAttribute("requiredFeatures", FLOWROOT_FEATURE);
171+
172+ SPObject* switch_obj = document->getObjectByRepr(switch_repr);
173+ switch_repr->addChild(flowtext_dup, NULL);
174+ switch_repr->addChild(basic_text, flowtext_dup);
175+ makeBasicTextItem(basic_text, item);
176+
177+ flowtext_dup->release();
178+
179+ //remove the old flowtext
180+ parent->removeChild(flowtext_repr);
181+
182+ flowtext->deleteObject();
183+ } else {
184+ if (!SP_IS_SWITCH(item)) {
185+ return true;
186+ }
187+
188+ Inkscape::XML::Node* switch_repr = item->getRepr();
189+ SPObject* text = NULL;
190+ SPFlowtext* flowtext = NULL;
191+
192+ for (SPObject* child = item->firstChild(); child; child = child->getNext()) {
193+ if (SP_IS_FLOWTEXT(child)) {
194+ flowtext = SP_FLOWTEXT(child);
195+ } else if (SP_IS_TEXT(child)) {
196+ text = child;
197+ }
198+ }
199+ if (!flowtext) {
200+ //this switch doesn't contain a flowtext directly, so we don't need to think
201+ //about it.
202+ return true;
203+ }
204+
205+ Inkscape::XML::Node* basic_text = getBasicText(flowtext);
206+ if (!basic_text) {
207+ //this shouldn't happen, but when it does, there's
208+ //nothing we can do about it.
209+ g_warning("Could not get basic text from flowed text for %s", item->getId());
210+ return false;
211+ }
212+
213+ //remove the text, because we're going to reconstruct it and add it back
214+ if (text) {
215+ switch_repr->removeChild(text->getRepr());
216+ text->deleteObject();
217+ }
218+
219+ flowtext->setAttribute("requiredFeatures", FLOWROOT_FEATURE);
220+
221+ switch_repr->addChild(basic_text, flowtext->getRepr());
222+ makeBasicTextItem(basic_text, flowtext);
223+ }
224+
225+ return false;
226+ }
227+};
228+
229+void flowText(SPDocument *oldDoc, SPDocument* doc) {
230+
231+ visit(doc->getRoot(), FlowTextReplacer(oldDoc, doc));
232+
233+}
234+
235+
236 } } } /* namespace inkscape, module, implementation */
237
238 /*
239
240=== modified file 'src/sp-filter.cpp'
241--- src/sp-filter.cpp 2013-10-07 19:25:41 +0000
242+++ src/sp-filter.cpp 2013-10-28 20:46:17 +0000
243@@ -220,7 +220,7 @@
244 // Original from sp-item-group.cpp
245 if (flags & SP_OBJECT_WRITE_BUILD) {
246 if (!repr) {
247- repr = doc->createElement("svg:this");
248+ repr = doc->createElement("svg:filter");
249 }
250
251 GSList *l = NULL;
252
253=== modified file 'src/sp-flowtext.h'
254--- src/sp-flowtext.h 2013-10-26 22:00:51 +0000
255+++ src/sp-flowtext.h 2013-10-28 20:46:17 +0000
256@@ -12,6 +12,9 @@
257 #define SP_FLOWTEXT(obj) (dynamic_cast<SPFlowtext*>((SPObject*)obj))
258 #define SP_IS_FLOWTEXT(obj) (dynamic_cast<const SPFlowtext*>((SPObject*)obj) != NULL)
259
260+//This feature URL is totally bogus, because flowRoot isn't part of any current or
261+//proposed SVG standard.
262+#define FLOWROOT_FEATURE "http://www.inkscape.org/feature#FlowRoot"
263
264 namespace Inkscape {
265
266
267=== modified file 'src/sp-font.cpp'
268--- src/sp-font.cpp 2013-07-31 23:06:31 +0000
269+++ src/sp-font.cpp 2013-10-28 20:46:17 +0000
270@@ -179,7 +179,7 @@
271
272 Inkscape::XML::Node* SPFont::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {
273 if ((flags & SP_OBJECT_WRITE_BUILD) && !repr) {
274- repr = xml_doc->createElement("svg:this");
275+ repr = xml_doc->createElement("svg:font");
276 }
277
278 sp_repr_set_svg_double(repr, "horiz-origin-x", this->horiz_origin_x);
279
280=== modified file 'src/sp-item-group.h'
281--- src/sp-item-group.h 2013-10-26 22:00:51 +0000
282+++ src/sp-item-group.h 2013-10-28 20:46:17 +0000
283@@ -55,7 +55,7 @@
284 void scaleChildItemsRec(Geom::Scale const &sc, Geom::Point const &p);
285
286 gint getItemCount() const;
287- void _showChildren (Inkscape::Drawing &drawing, Inkscape::DrawingItem *ai, unsigned int key, unsigned int flags);
288+ virtual void _showChildren (Inkscape::Drawing &drawing, Inkscape::DrawingItem *ai, unsigned int key, unsigned int flags);
289
290 private:
291 void _updateLayerMode(unsigned int display_key=0);
292
293=== modified file 'src/sp-object.h'
294--- src/sp-object.h 2013-10-09 19:09:49 +0000
295+++ src/sp-object.h 2013-10-28 20:46:17 +0000
296@@ -844,6 +844,20 @@
297 virtual void read_content();
298 };
299
300+/**
301+ * @brief Run a function on an SPObject and all of its descendents (parent-first)
302+ * @param parent The object to visit
303+ * @param fun The function to run. If the function returns false, the
304+ * traversal will not visit this object's children
305+ */
306+template <typename T> void visit (SPObject* parent, T fun) {
307+ if (fun(parent)) {
308+ for (SPObject *child = parent->children ; child && child != parent ;
309+ child = child->getNext() ) {
310+ visit(child, fun);
311+ }
312+ }
313+}
314
315 /**
316 * Compares height of objects in tree.
317
318=== modified file 'src/sp-text.cpp'
319--- src/sp-text.cpp 2013-10-28 09:31:07 +0000
320+++ src/sp-text.cpp 2013-10-28 20:46:17 +0000
321@@ -235,7 +235,7 @@
322 Inkscape::XML::Node *SPText::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {
323 if (flags & SP_OBJECT_WRITE_BUILD) {
324 if (!repr) {
325- repr = xml_doc->createElement("svg:this");
326+ repr = xml_doc->createElement("svg:text");
327 }
328
329 GSList *l = NULL;