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

[David Turner]

This appears to fix the problem.

[David Turner (novalis)]

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

[David Turner (novalis)]

Yeah, that's better.

[Tavmjong Bah (tavmjong-free)]

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)]

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)]

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)]

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)]

Hm. OK, I can probably do that.

[David Turner (novalis)]

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)]

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)]

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)]

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)]

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)]

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

[David Turner (novalis)]

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

[su_v (suv-lp)]

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]

12738. By David Turner on 2013-10-28

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

[Martin Owens (doctormo)]

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)]

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)]

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)]

The linked file does contain a flowRoot.

[Martin Owens (doctormo)]

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

=== 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 @@
 
 "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"
 
-"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"
+"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"
 
 "restart" - "animate","animateColor","animateMotion","animateTransform","set"
 
 
=== 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 @@
     return parts;
 }
 
+#define INKSCAPEFEATURE    "http://www.inkscape.org/feature#"
 #define SVG11FEATURE    "http://www.w3.org/TR/SVG11/feature#"
 #define SVG10FEATURE    "org.w3c."
 
@@ -250,6 +251,9 @@
     found = strstr(value, SVG10FEATURE);
     if ( value == found )
         return evaluateSVG10Feature(found + strlen(SVG10FEATURE));
+    found = strstr(value, INKSCAPEFEATURE);
+    if ( value == found )
+        return true;
     return false;
 }
 
 
=== 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 @@
 #include "extension/output.h"
 #include <vector>
 #include "xml/attribute-record.h"
+#include "xml/event-fns.h"
+#include "document.h"
+#include "factory.h"
+#include "sp-factory.h"
 #include "sp-root.h"
-#include "document.h"
+#include "sp-switch.h"
+#include "sp-text.h"
+#include "sp-flowtext.h"
 
 #ifdef WITH_GNOME_VFS
 # include <libgnomevfs/gnome-vfs.h>
@@ -42,7 +48,7 @@
 using Inkscape::XML::AttributeRecord;
 using Inkscape::XML::Node;
 
-
+void flowText(SPDocument* orig, SPDocument* doc);
 
 static void pruneExtendedAttributes( Inkscape::XML::Node *repr )
 {
@@ -234,30 +240,162 @@
       || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVG_INKSCAPE)
       || !strcmp (mod->get_id(), SP_MODULE_KEY_OUTPUT_SVGZ_INKSCAPE));
 
-    Inkscape::XML::Document *rdoc = NULL;
     Inkscape::XML::Node *repr = NULL;
-    if (exportExtensions) {
-        repr = doc->getReprRoot();
-    } else {
-        rdoc = sp_repr_document_new ("svg:svg");
-        repr = rdoc->root();
-        repr = doc->getRoot()->updateRepr(rdoc, repr, SP_OBJECT_WRITE_BUILD);
-
+    SPDocument* newDoc = NULL;
+
+    newDoc = SPDocument::createNewDoc(0, 0, true);
+    repr = doc->getRoot()->updateRepr(newDoc->getReprDoc(),
+            newDoc->getReprRoot(), SP_OBJECT_WRITE_BUILD);
+
+    if (!exportExtensions) {
         pruneExtendedAttributes(repr);
     }
 
+    flowText(doc, newDoc);
+
     if (!sp_repr_save_rebased_file(repr->document(), filename, SP_SVG_NS_URI,
                                    doc->getBase(), filename)) {
         throw Inkscape::Extension::Output::save_failed();
     }
 
-    if (!exportExtensions) {
-        Inkscape::GC::release(rdoc);
-    }
-
+    delete newDoc;
     return;
 }
 
+class FlowTextReplacer {
+
+private:
+    SPDocument* document;
+
+    //we need this nonsense so that we can get a correctly-laid-out flowtext
+    SPDocument* oldDocument;
+    Inkscape::XML::Document* xml_document;
+
+    Inkscape::XML::Node* getBasicText(SPFlowtext* flowtext) const {
+        //This is probably an insane way to get a SPFlowtext with the correct layout, but I
+        //can't figure out another way.
+        SPObject* original_object = oldDocument->getObjectById(flowtext->getId());
+        SPFlowtext* original_flowtext = SP_FLOWTEXT(original_object);
+        flowtext->layout = original_flowtext->layout;
+        Inkscape::XML::Node* basic_text = flowtext->getAsText();
+        return basic_text;
+    }
+
+    SPItem* makeBasicTextItem(Inkscape::XML::Node* basic_text,
+            SPItem* item) const {
+        SPItem* new_item =
+                reinterpret_cast<SPItem*>(SPFactory::instance().createObject(
+                        NodeTraits::get_type_string(*basic_text)));
+        new_item->invoke_build(document, basic_text, TRUE);
+        new_item->doWriteTransform(basic_text, item->transform);
+        new_item->updateRepr();
+        return new_item;
+    }
+
+public:
+
+    FlowTextReplacer (SPDocument* oldDocument, SPDocument* document) :
+        document (document),
+        oldDocument (oldDocument) {
+        xml_document = document->getReprDoc();
+    }
+
+    bool operator () (SPObject* obj) const {
+        SPItem* item = SP_ITEM(obj);
+        if (!item)
+            return true;
+
+        /* This might be a switch or a flowtext.  If it's a flowtext, we need to wrap it
+         * in a switch.  If it's a switch with <flowtext, text>, we need to update the
+         * text.
+        */
+
+        if (SP_IS_FLOWTEXT(item)) {
+            SPFlowtext* flowtext = SP_FLOWTEXT(item);
+
+            Inkscape::XML::Node* basic_text = getBasicText(flowtext);
+            if (!basic_text) {
+                //this shouldn't happen, but when it does, there's
+                //nothing we can do about it.
+                g_warning("Could not get basic text from flowed text for %s", item->getId());
+                return false;
+            }
+
+            Inkscape::XML::Node* flowtext_repr = flowtext->getRepr();
+            Inkscape::XML::Node* parent = flowtext_repr->parent();
+
+            Inkscape::XML::Node *switch_repr = xml_document->createElement("svg:switch");
+            parent->addChild(switch_repr, flowtext_repr);
+
+            //duplicate the flowtext so that we can place it into the switch
+            Inkscape::XML::Node* flowtext_dup = flowtext_repr->duplicate(xml_document);
+
+            flowtext_dup->setAttribute("requiredFeatures", FLOWROOT_FEATURE);
+
+            SPObject* switch_obj = document->getObjectByRepr(switch_repr);
+            switch_repr->addChild(flowtext_dup, NULL);
+            switch_repr->addChild(basic_text, flowtext_dup);
+            makeBasicTextItem(basic_text, item);
+
+            flowtext_dup->release();
+
+            //remove the old flowtext
+            parent->removeChild(flowtext_repr);
+
+            flowtext->deleteObject();
+        } else {
+            if (!SP_IS_SWITCH(item)) {
+                return true;
+            }
+
+            Inkscape::XML::Node* switch_repr = item->getRepr();
+            SPObject* text = NULL;
+            SPFlowtext* flowtext = NULL;
+
+            for (SPObject* child = item->firstChild(); child; child = child->getNext()) {
+                if (SP_IS_FLOWTEXT(child)) {
+                    flowtext = SP_FLOWTEXT(child);
+                } else if (SP_IS_TEXT(child)) {
+                    text = child;
+                }
+            }
+            if (!flowtext) {
+                //this switch doesn't contain a flowtext directly, so we don't need to think
+                //about it.
+                return true;
+            }
+
+            Inkscape::XML::Node* basic_text = getBasicText(flowtext);
+            if (!basic_text) {
+                //this shouldn't happen, but when it does, there's
+                //nothing we can do about it.
+                g_warning("Could not get basic text from flowed text for %s", item->getId());
+                return false;
+            }
+
+            //remove the text, because we're going to reconstruct it and add it back
+            if (text) {
+                switch_repr->removeChild(text->getRepr());
+                text->deleteObject();
+            }
+
+            flowtext->setAttribute("requiredFeatures", FLOWROOT_FEATURE);
+
+            switch_repr->addChild(basic_text, flowtext->getRepr());
+            makeBasicTextItem(basic_text, flowtext);
+        }
+
+        return false;
+    }
+};
+
+void flowText(SPDocument *oldDoc, SPDocument* doc) {
+
+    visit(doc->getRoot(), FlowTextReplacer(oldDoc, doc));
+
+}
+
+
 } } }  /* namespace inkscape, module, implementation */
 
 /*
 
=== 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 @@
     // Original from sp-item-group.cpp
     if (flags & SP_OBJECT_WRITE_BUILD) {
         if (!repr) {
-            repr = doc->createElement("svg:this");
+            repr = doc->createElement("svg:filter");
         }
 
         GSList *l = NULL;
 
=== 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 @@
 #define SP_FLOWTEXT(obj) (dynamic_cast<SPFlowtext*>((SPObject*)obj))
 #define SP_IS_FLOWTEXT(obj) (dynamic_cast<const SPFlowtext*>((SPObject*)obj) != NULL)
 
+//This feature URL is totally bogus, because flowRoot isn't part of any current or
+//proposed SVG standard.
+#define FLOWROOT_FEATURE "http://www.inkscape.org/feature#FlowRoot"
 
 namespace Inkscape {
 
 
=== 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 @@
 
 Inkscape::XML::Node* SPFont::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {
     if ((flags & SP_OBJECT_WRITE_BUILD) && !repr) {
-        repr = xml_doc->createElement("svg:this");
+        repr = xml_doc->createElement("svg:font");
     }
 
     sp_repr_set_svg_double(repr, "horiz-origin-x", this->horiz_origin_x);
 
=== 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 @@
     void scaleChildItemsRec(Geom::Scale const &sc, Geom::Point const &p);
 
     gint getItemCount() const;
-    void _showChildren (Inkscape::Drawing &drawing, Inkscape::DrawingItem *ai, unsigned int key, unsigned int flags);
+    virtual void _showChildren (Inkscape::Drawing &drawing, Inkscape::DrawingItem *ai, unsigned int key, unsigned int flags);
 
 private:
     void _updateLayerMode(unsigned int display_key=0);
 
=== 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 @@
 	virtual void read_content();
 };
 
+/**
+ * @brief Run a function on an SPObject and all of its descendents (parent-first)
+ * @param parent The object to visit
+ * @param fun The function to run.  If the function returns false, the
+ * traversal will not visit this object's children
+ */
+template <typename T> void visit (SPObject* parent, T fun) {
+    if (fun(parent)) {
+        for (SPObject *child = parent->children ; child && child != parent ;
+            child = child->getNext() ) {
+            visit(child, fun);
+        }
+    }
+}
 
 /**
  * Compares height of objects in tree.
 
=== 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 @@
 Inkscape::XML::Node *SPText::write(Inkscape::XML::Document *xml_doc, Inkscape::XML::Node *repr, guint flags) {
     if (flags & SP_OBJECT_WRITE_BUILD) {
         if (!repr) {
-            repr = xml_doc->createElement("svg:this");
+            repr = xml_doc->createElement("svg:text");
         }
 
         GSList *l = NULL;