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
=== modified file 'share/attributes/svgprops'
--- share/attributes/svgprops 2012-10-08 10:58:24 +0000
+++ share/attributes/svgprops 2013-10-28 20:46:17 +0000
@@ -278,7 +278,7 @@
278278
279"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"279"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"
280280
281"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"281"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"
282282
283"restart" - "animate","animateColor","animateMotion","animateTransform","set"283"restart" - "animate","animateColor","animateMotion","animateTransform","set"
284284
285285
=== modified file 'src/conditions.cpp'
--- src/conditions.cpp 2012-02-15 20:50:04 +0000
+++ src/conditions.cpp 2013-10-28 20:46:17 +0000
@@ -140,6 +140,7 @@
140 return parts;140 return parts;
141}141}
142142
143#define INKSCAPEFEATURE "http://www.inkscape.org/feature#"
143#define SVG11FEATURE "http://www.w3.org/TR/SVG11/feature#"144#define SVG11FEATURE "http://www.w3.org/TR/SVG11/feature#"
144#define SVG10FEATURE "org.w3c."145#define SVG10FEATURE "org.w3c."
145146
@@ -250,6 +251,9 @@
250 found = strstr(value, SVG10FEATURE);251 found = strstr(value, SVG10FEATURE);
251 if ( value == found )252 if ( value == found )
252 return evaluateSVG10Feature(found + strlen(SVG10FEATURE));253 return evaluateSVG10Feature(found + strlen(SVG10FEATURE));
254 found = strstr(value, INKSCAPEFEATURE);
255 if ( value == found )
256 return true;
253 return false;257 return false;
254}258}
255259
256260
=== modified file 'src/extension/internal/svg.cpp'
--- src/extension/internal/svg.cpp 2012-10-04 01:47:32 +0000
+++ src/extension/internal/svg.cpp 2013-10-28 20:46:17 +0000
@@ -24,8 +24,14 @@
24#include "extension/output.h"24#include "extension/output.h"
25#include <vector>25#include <vector>
26#include "xml/attribute-record.h"26#include "xml/attribute-record.h"
27#include "xml/event-fns.h"
28#include "document.h"
29#include "factory.h"
30#include "sp-factory.h"
27#include "sp-root.h"31#include "sp-root.h"
28#include "document.h"32#include "sp-switch.h"
33#include "sp-text.h"
34#include "sp-flowtext.h"
2935
30#ifdef WITH_GNOME_VFS36#ifdef WITH_GNOME_VFS
31# include <libgnomevfs/gnome-vfs.h>37# include <libgnomevfs/gnome-vfs.h>
@@ -42,7 +48,7 @@
42using Inkscape::XML::AttributeRecord;48using Inkscape::XML::AttributeRecord;
43using Inkscape::XML::Node;49using Inkscape::XML::Node;
4450
4551void flowText(SPDocument* orig, SPDocument* doc);
4652
47static void pruneExtendedAttributes( Inkscape::XML::Node *repr )53static void pruneExtendedAttributes( Inkscape::XML::Node *repr )
48{54{
@@ -234,30 +240,162 @@
234 || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVG_INKSCAPE)240 || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVG_INKSCAPE)
235 || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVGZ_INKSCAPE));241 || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVGZ_INKSCAPE));
236242
237 Inkscape::XML::Document *rdoc = NULL;
238 Inkscape::XML::Node *repr = NULL;243 Inkscape::XML::Node *repr = NULL;
239 if (exportExtensions) {244 SPDocument* newDoc = NULL;
240 repr = doc->getReprRoot();245
241 } else {246 newDoc = SPDocument::createNewDoc(0, 0, true);
242 rdoc = sp_repr_document_new ("svg:svg");247 repr = doc->getRoot()->updateRepr(newDoc->getReprDoc(),
243 repr = rdoc->root();248 newDoc->getReprRoot(), SP_OBJECT_WRITE_BUILD);
244 repr = doc->getRoot()->updateRepr(rdoc, repr, SP_OBJECT_WRITE_BUILD);249
245250 if (!exportExtensions) {
246 pruneExtendedAttributes(repr);251 pruneExtendedAttributes(repr);
247 }252 }
248253
254 flowText(doc, newDoc);
255
249 if (!sp_repr_save_rebased_file(repr->document(), filename, SP_SVG_NS_URI,256 if (!sp_repr_save_rebased_file(repr->document(), filename, SP_SVG_NS_URI,
250 doc->getBase(), filename)) {257 doc->getBase(), filename)) {
251 throw Inkscape::Extension::Output::save_failed();258 throw Inkscape::Extension::Output::save_failed();
252 }259 }
253260
254 if (!exportExtensions) {261 delete newDoc;
255 Inkscape::GC::release(rdoc);
256 }
257
258 return;262 return;
259}263}
260264
265class FlowTextReplacer {
266
267private:
268 SPDocument* document;
269
270 //we need this nonsense so that we can get a correctly-laid-out flowtext
271 SPDocument* oldDocument;
272 Inkscape::XML::Document* xml_document;
273
274 Inkscape::XML::Node* getBasicText(SPFlowtext* flowtext) const {
275 //This is probably an insane way to get a SPFlowtext with the correct layout, but I
276 //can't figure out another way.
277 SPObject* original_object = oldDocument->getObjectById(flowtext->getId());
278 SPFlowtext* original_flowtext = SP_FLOWTEXT(original_object);
279 flowtext->layout = original_flowtext->layout;
280 Inkscape::XML::Node* basic_text = flowtext->getAsText();
281 return basic_text;
282 }
283
284 SPItem* makeBasicTextItem(Inkscape::XML::Node* basic_text,
285 SPItem* item) const {
286 SPItem* new_item =
287 reinterpret_cast<SPItem*>(SPFactory::instance().createObject(
288 NodeTraits::get_type_string(*basic_text)));
289 new_item->invoke_build(document, basic_text, TRUE);
290 new_item->doWriteTransform(basic_text, item->transform);
291 new_item->updateRepr();
292 return new_item;
293 }
294
295public:
296
297 FlowTextReplacer (SPDocument* oldDocument, SPDocument* document) :
298 document (document),
299 oldDocument (oldDocument) {
300 xml_document = document->getReprDoc();
301 }
302
303 bool operator () (SPObject* obj) const {
304 SPItem* item = SP_ITEM(obj);
305 if (!item)
306 return true;
307
308 /* This might be a switch or a flowtext. If it's a flowtext, we need to wrap it
309 * in a switch. If it's a switch with <flowtext, text>, we need to update the
310 * text.
311 */
312
313 if (SP_IS_FLOWTEXT(item)) {
314 SPFlowtext* flowtext = SP_FLOWTEXT(item);
315
316 Inkscape::XML::Node* basic_text = getBasicText(flowtext);
317 if (!basic_text) {
318 //this shouldn't happen, but when it does, there's
319 //nothing we can do about it.
320 g_warning("Could not get basic text from flowed text for %s", item->getId());
321 return false;
322 }
323
324 Inkscape::XML::Node* flowtext_repr = flowtext->getRepr();
325 Inkscape::XML::Node* parent = flowtext_repr->parent();
326
327 Inkscape::XML::Node *switch_repr = xml_document->createElement("svg:switch");
328 parent->addChild(switch_repr, flowtext_repr);
329
330 //duplicate the flowtext so that we can place it into the switch
331 Inkscape::XML::Node* flowtext_dup = flowtext_repr->duplicate(xml_document);
332
333 flowtext_dup->setAttribute("requiredFeatures", FLOWROOT_FEATURE);
334
335 SPObject* switch_obj = document->getObjectByRepr(switch_repr);
336 switch_repr->addChild(flowtext_dup, NULL);
337 switch_repr->addChild(basic_text, flowtext_dup);
338 makeBasicTextItem(basic_text, item);
339
340 flowtext_dup->release();
341
342 //remove the old flowtext
343 parent->removeChild(flowtext_repr);
344
345 flowtext->deleteObject();
346 } else {
347 if (!SP_IS_SWITCH(item)) {
348 return true;
349 }
350
351 Inkscape::XML::Node* switch_repr = item->getRepr();
352 SPObject* text = NULL;
353 SPFlowtext* flowtext = NULL;
354
355 for (SPObject* child = item->firstChild(); child; child = child->getNext()) {
356 if (SP_IS_FLOWTEXT(child)) {
357 flowtext = SP_FLOWTEXT(child);
358 } else if (SP_IS_TEXT(child)) {
359 text = child;
360 }
361 }
362 if (!flowtext) {
363 //this switch doesn't contain a flowtext directly, so we don't need to think
364 //about it.
365 return true;
366 }
367
368 Inkscape::XML::Node* basic_text = getBasicText(flowtext);
369 if (!basic_text) {
370 //this shouldn't happen, but when it does, there's
371 //nothing we can do about it.
372 g_warning("Could not get basic text from flowed text for %s", item->getId());
373 return false;
374 }
375
376 //remove the text, because we're going to reconstruct it and add it back
377 if (text) {
378 switch_repr->removeChild(text->getRepr());
379 text->deleteObject();
380 }
381
382 flowtext->setAttribute("requiredFeatures", FLOWROOT_FEATURE);
383
384 switch_repr->addChild(basic_text, flowtext->getRepr());
385 makeBasicTextItem(basic_text, flowtext);
386 }
387
388 return false;
389 }
390};
391
392void flowText(SPDocument *oldDoc, SPDocument* doc) {
393
394 visit(doc->getRoot(), FlowTextReplacer(oldDoc, doc));
395
396}
397
398
261} } } /* namespace inkscape, module, implementation */399} } } /* namespace inkscape, module, implementation */
262400
263/*401/*
264402
=== modified file 'src/sp-filter.cpp'
--- src/sp-filter.cpp 2013-10-07 19:25:41 +0000
+++ src/sp-filter.cpp 2013-10-28 20:46:17 +0000
@@ -220,7 +220,7 @@
220 // Original from sp-item-group.cpp220 // Original from sp-item-group.cpp
221 if (flags & SP_OBJECT_WRITE_BUILD) {221 if (flags & SP_OBJECT_WRITE_BUILD) {
222 if (!repr) {222 if (!repr) {
223 repr = doc->createElement("svg:this");223 repr = doc->createElement("svg:filter");
224 }224 }
225225
226 GSList *l = NULL;226 GSList *l = NULL;
227227
=== modified file 'src/sp-flowtext.h'
--- src/sp-flowtext.h 2013-10-26 22:00:51 +0000
+++ src/sp-flowtext.h 2013-10-28 20:46:17 +0000
@@ -12,6 +12,9 @@
12#define SP_FLOWTEXT(obj) (dynamic_cast<SPFlowtext*>((SPObject*)obj))12#define SP_FLOWTEXT(obj) (dynamic_cast<SPFlowtext*>((SPObject*)obj))
13#define SP_IS_FLOWTEXT(obj) (dynamic_cast<const SPFlowtext*>((SPObject*)obj) != NULL)13#define SP_IS_FLOWTEXT(obj) (dynamic_cast<const SPFlowtext*>((SPObject*)obj) != NULL)
1414
15//This feature URL is totally bogus, because flowRoot isn't part of any current or
16//proposed SVG standard.
17#define FLOWROOT_FEATURE "http://www.inkscape.org/feature#FlowRoot"
1518
16namespace Inkscape {19namespace Inkscape {
1720
1821
=== modified file 'src/sp-font.cpp'
--- src/sp-font.cpp 2013-07-31 23:06:31 +0000
+++ src/sp-font.cpp 2013-10-28 20:46:17 +0000
@@ -179,7 +179,7 @@
179179
180Inkscape::XML::Node* SPFont::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {180Inkscape::XML::Node* SPFont::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {
181 if ((flags & SP_OBJECT_WRITE_BUILD) && !repr) {181 if ((flags & SP_OBJECT_WRITE_BUILD) && !repr) {
182 repr = xml_doc->createElement("svg:this");182 repr = xml_doc->createElement("svg:font");
183 }183 }
184184
185 sp_repr_set_svg_double(repr, "horiz-origin-x", this->horiz_origin_x);185 sp_repr_set_svg_double(repr, "horiz-origin-x", this->horiz_origin_x);
186186
=== modified file 'src/sp-item-group.h'
--- src/sp-item-group.h 2013-10-26 22:00:51 +0000
+++ src/sp-item-group.h 2013-10-28 20:46:17 +0000
@@ -55,7 +55,7 @@
55 void scaleChildItemsRec(Geom::Scale const &sc, Geom::Point const &p);55 void scaleChildItemsRec(Geom::Scale const &sc, Geom::Point const &p);
5656
57 gint getItemCount() const;57 gint getItemCount() const;
58 void _showChildren (Inkscape::Drawing &drawing, Inkscape::DrawingItem *ai, unsigned int key, unsigned int flags);58 virtual void _showChildren (Inkscape::Drawing &drawing, Inkscape::DrawingItem *ai, unsigned int key, unsigned int flags);
5959
60private:60private:
61 void _updateLayerMode(unsigned int display_key=0);61 void _updateLayerMode(unsigned int display_key=0);
6262
=== modified file 'src/sp-object.h'
--- src/sp-object.h 2013-10-09 19:09:49 +0000
+++ src/sp-object.h 2013-10-28 20:46:17 +0000
@@ -844,6 +844,20 @@
844 virtual void read_content();844 virtual void read_content();
845};845};
846846
847/**
848 * @brief Run a function on an SPObject and all of its descendents (parent-first)
849 * @param parent The object to visit
850 * @param fun The function to run. If the function returns false, the
851 * traversal will not visit this object's children
852 */
853template <typename T> void visit (SPObject* parent, T fun) {
854 if (fun(parent)) {
855 for (SPObject *child = parent->children ; child && child != parent ;
856 child = child->getNext() ) {
857 visit(child, fun);
858 }
859 }
860}
847861
848/**862/**
849 * Compares height of objects in tree.863 * Compares height of objects in tree.
850864
=== modified file 'src/sp-text.cpp'
--- src/sp-text.cpp 2013-10-28 09:31:07 +0000
+++ src/sp-text.cpp 2013-10-28 20:46:17 +0000
@@ -235,7 +235,7 @@
235Inkscape::XML::Node *SPText::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {235Inkscape::XML::Node *SPText::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {
236 if (flags & SP_OBJECT_WRITE_BUILD) {236 if (flags & SP_OBJECT_WRITE_BUILD) {
237 if (!repr) {237 if (!repr) {
238 repr = xml_doc->createElement("svg:this");238 repr = xml_doc->createElement("svg:text");
239 }239 }
240240
241 GSList *l = NULL;241 GSList *l = NULL;