Merge lp:~inkscape.dev/inkscape/092+lineheight-fix into lp:inkscape/0.92.x

[Mc]

This is a branch to work on the conversion of old (pre-0.92) files.
Merge request is primarily to see and review the changes.
Please do not merge before I add a commandline flag to prevent the file update

[lp:~inkscape.dev/inkscape/092+lineheight-fix]

15327. By Mc on 2017-01-22

updates display

15328. By Mc on 2017-01-22

clang-format

[Martin Owens (doctormo)]

Should the name of sp_file_fix_run_recursive be something specific to text/flowtext since that's what it works on? Maybe sp_file_text_run_recursive would work.

Should the comment in src/file.cpp:214 be kept without a comment?

Everything else looks great.

[Martin Owens (doctormo)]

I get the error:

document.o: In function `SPDocument::createDoc(Inkscape::XML::Document*, char const*, char const*, char const*, unsigned int, SPDocument*)':
/home/doctormo/Projects/inkscape/92/build/src/../../src/document.cpp:457: undefined reference to `sp_file_fix_text(SPDocument*)'

When trying to compile. Must be a build error, is make not enough or does it need a reconfigure?

[lp:~inkscape.dev/inkscape/092+lineheight-fix]

15329. By Mc on 2017-01-22

add file in automake

[Mc (mc...)]

It's because you're using autotools build. It might need a reconfigure after checking this commit, I added a file.

[lp:~inkscape.dev/inkscape/092+lineheight-fix]

15330. By Mc on 2017-01-22

useless change

15331. By Mc on 2017-01-22

merge .92.x

[Martin Owens (doctormo)]

Merged, compiled and tested as working against pepper and carrot.

review: Approve (code)

[lp:~inkscape.dev/inkscape/092+lineheight-fix]

15332. By Mc on 2017-01-23

Actually write changes to the file (!)

15333. By Mc on 2017-01-23

merge .92.x

15334. By Mc on 2017-01-23

fix for blank lines at beginning of text

15335. By Mc on 2017-01-23

add option --do-not-fix-pre-92 to prevent the automatic fix

15336. By Mc on 2017-01-23

merge .92.x

15337. By Mc on 2017-01-24

minor fix

15338. By Mc on 2017-01-24

rename helper function

[Tavmjong Bah (tavmjong-free)]

On Sun, 2017-01-22 at 16:51 +0000, Mc wrote:
> Mc has proposed merging lp:~inkscape.dev/inkscape/092+lineheight-fix
> into lp:inkscape/0.92.x.
>
> Requested reviews:
>   Tavmjong Bah (tavmjong-free): code, behavior, correctness, css
>
> For more details, see:
> https://code.launchpad.net/~inkscape.dev/inkscape/092+lineheight-fix/
> +merge/315315
>
> This is a branch to work on the conversion of old (pre-0.92) files.
> Merge request is primarily to see and review the changes.
> Please do not merge before I add a commandline flag to prevent the
> file update

Nicely structured!

1. Documentation!

2. Code style:

    if {
      // Brackets even if one line.
    }

    for {
      // Brackets even if one line.
    }

3. gchar

   Do you really need to use gchar instead of std::string?

4. fix_blank_line:

   Hopefully won't be necessary with proper fix.

   Why are you saving font-size from non-empty lines?

   <text style="font-size:20px">
     <tspan sodipodi:role="line" style="font-size:10px">xxx</tspan>
     <tspan sodipodi:role="line">yyy<tspan>

   'yyy' should be 20px high.

5. fix_line_spacing:

   This is rather brute force but maybe that is OK.

   A value of 0.00% is the same as 0.

   Why is it necessary to set line_height to 0.01% for flowed text?

6. fix_font_size:

   Also rather brute force.

   Why is this recursive? A tspan with role 'line' should not contain
   another tspan with role 'line'. Ditto for flowPara. (You don't
   call fix_line_spacing recursively, why here?)

   Why are you testing if fontsize is set inside the loop? It must be
   or you would have returned at the beginning of the function.

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2016-11-03 21:51:32 +0000
+++ src/CMakeLists.txt	2017-01-24 00:26:50 +0000
@@ -202,6 +202,7 @@
   event-log.cpp
   extract-uri.cpp
   file.cpp
+  file-update.cpp
   filter-chemistry.cpp
   filter-enums.cpp
   gc-anchored.cpp

=== modified file 'src/Makefile_insert'
--- src/Makefile_insert	2016-09-27 10:14:53 +0000
+++ src/Makefile_insert	2017-01-24 00:26:50 +0000
@@ -40,7 +40,7 @@
 	enums.h								\
 	event-log.cpp event-log.h event.h				\
 	extract-uri.cpp extract-uri.h					\
-	file.cpp file.h							\
+	file.cpp file-update.cpp file.h					\
 	fill-or-stroke.h						\
 	filter-chemistry.cpp filter-chemistry.h				\
 	filter-enums.cpp filter-enums.h					\

=== modified file 'src/document.cpp'
--- src/document.cpp	2016-11-28 22:20:00 +0000
+++ src/document.cpp	2017-01-24 00:26:50 +0000
@@ -50,6 +50,7 @@
 #include "display/drawing-item.h"
 #include "document-private.h"
 #include "document-undo.h"
+#include "file.h"
 #include "id-clash.h"
 #include "inkscape.h"
 #include "inkscape-version.h"
@@ -78,7 +79,7 @@
 // since we want it to happen when there are no more updates.
 #define SP_DOCUMENT_REROUTING_PRIORITY (G_PRIORITY_HIGH_IDLE - 1)
 
-
+bool sp_do_not_fix_pre_92 = false;
 static gint sp_document_idle_handler(gpointer data);
 static gint sp_document_rerouting_handler(gpointer data);
 
@@ -452,7 +453,9 @@
                 sigc::ptr_fun(&DocumentUndo::resetKey), document)
     ));
     document->oldSignalsConnected = true;
-
+    if ( (!sp_do_not_fix_pre_92) && sp_version_inside_range( document->root->version.inkscape, 0, 1, 0, 92 ) ) {
+        sp_file_fix_text(document);
+   }
     return document;
 }
 

=== modified file 'src/document.h'
--- src/document.h	2016-06-14 10:26:58 +0000
+++ src/document.h	2017-01-24 00:26:50 +0000
@@ -30,6 +30,15 @@
 #include <set>
 #include <deque>
 
+// This variable is introduced with 0.92.1
+// with the introduction of automatic fix 
+// for files detected to have been created 
+// with previous versions to have a similar
+// look in 0.92+.
+extern bool sp_do_not_fix_pre_92;
+
+
+
 namespace Avoid {
 class Router;
 }

=== added file 'src/file-update.cpp'
--- src/file-update.cpp	1970-01-01 00:00:00 +0000
+++ src/file-update.cpp	2017-01-24 00:26:50 +0000
@@ -0,0 +1,174 @@
+/**
+ * @file-update
+ * Operations to bump files from the pre-0.92 era into the 0.92+ era
+ * (90dpi vs 96dpi, line height problems, and related bugs)
+ */
+/* Authors:
+ * Tavmjong Bah
+ * Marc Jeanmougin
+ * su_v
+ */
+#include "file.h"
+#include "sp-root.h"
+#include "sp-text.h"
+#include "sp-tspan.h"
+#include "sp-flowdiv.h"
+#include "sp-flowtext.h"
+#include "sp-object.h"
+#include "sp-item.h"
+#include "style.h"
+#include "document.h"
+#include <string>
+#include "text-editing.h"
+
+using namespace std;
+
+bool is_line(SPObject *i)
+{
+    if (!(i->getAttribute("sodipodi:role")))
+        return false;
+    return !strcmp(i->getAttribute("sodipodi:role"), "line");
+}
+
+
+void fix_blank_line(SPObject *o)
+{
+    if (SP_IS_TEXT(o))
+        ((SPText *)o)->rebuildLayout();
+    else if (SP_IS_FLOWTEXT(o))
+        ((SPFlowtext *)o)->rebuildLayout();
+
+    SPIFontSize fontsize = o->style->font_size;
+    SPILengthOrNormal lineheight = o->style->line_height;
+    vector<SPObject *> cl = o->childList(false);
+    Inkscape::Text::Layout::iterator pos = te_get_layout((SPItem *)(o))->begin();
+    bool beginning = true;
+    for (vector<SPObject *>::const_iterator ci = cl.begin(); ci != cl.end(); ++ci) {
+        SPObject *i = *ci;
+        if ((SP_IS_TSPAN(i) && is_line(i)) || SP_IS_FLOWPARA(i)) {
+            if (sp_text_get_length((SPItem *)i) <= 1) { // empty line
+                // Inkscape::Text::Layout::iterator pos = te_get_layout((SPItem*)(o))->charIndexToIterator(1 +
+                // sp_text_get_length_upto(o,i));
+                sp_te_insert((SPItem *)o, pos, "\u00a0"); //"\u00a0"
+                gchar *l = g_strdup_printf("%f", lineheight.value);
+                gchar *f = g_strdup_printf("%f", fontsize.value);
+                i->style->line_height.readIfUnset(l);
+                if (!beginning)
+                    i->style->font_size.read(f);
+                else
+                    i->style->font_size.readIfUnset(f);
+                g_free(l);
+                g_free(f);
+            } else {
+                beginning = false;
+                fontsize = i->style->font_size;
+                lineheight = o->style->line_height;
+            }
+            pos.nextLineCursor();
+            // pos.cursorDownWithControl();
+        }
+    }
+}
+
+void fix_line_spacing(SPObject *o)
+{
+    SPILengthOrNormal lineheight = o->style->line_height;
+    bool inner = false;
+    vector<SPObject *> cl = o->childList(false);
+    for (vector<SPObject *>::const_iterator ci = cl.begin(); ci != cl.end(); ++ci) {
+        SPObject *i = *ci;
+        if ((SP_IS_TSPAN(i) && is_line(i)) || SP_IS_FLOWPARA(i)) {
+            // if no line-height attribute, set it
+            gchar *l = g_strdup_printf("%f", lineheight.value);
+            i->style->line_height.readIfUnset(l);
+            g_free(l);
+        }
+        inner = true;
+    }
+    if (inner) {
+        if (SP_IS_TEXT(o)) {
+            o->style->line_height.read("0.00%");
+        } else {
+            o->style->line_height.readIfUnset("0.01%");
+        }
+    }
+}
+
+void fix_font_name(SPObject *o)
+{
+    vector<SPObject *> cl = o->childList(false);
+    for (vector<SPObject *>::const_iterator ci = cl.begin(); ci != cl.end(); ++ci)
+        fix_font_name(*ci);
+    string prev = o->style->font_family.value ? o->style->font_family.value : o->style->font_family.value_default;
+    if (prev == "Sans")
+        o->style->font_family.read("sans-serif");
+    else if (prev == "Serif")
+        o->style->font_family.read("serif");
+    else if (prev == "Monospace")
+        o->style->font_family.read("monospace");
+}
+
+
+void fix_font_size(SPObject *o)
+{
+    SPIFontSize fontsize = o->style->font_size;
+    if (!fontsize.set)
+        return;
+    bool inner = false;
+    vector<SPObject *> cl = o->childList(false);
+    for (vector<SPObject *>::const_iterator ci = cl.begin(); ci != cl.end(); ++ci) {
+        SPObject *i = *ci;
+        fix_font_size(i);
+        if ((SP_IS_TSPAN(i) && is_line(i)) || SP_IS_FLOWPARA(i)) {
+            inner = true;
+            gchar *s = g_strdup_printf("%f", fontsize.value);
+            if (fontsize.set)
+                i->style->font_size.readIfUnset(s);
+            g_free(s);
+        }
+    }
+    if (inner && (SP_IS_TEXT(o) || SP_IS_FLOWTEXT(o)))
+        o->style->font_size.clear();
+}
+
+
+
+// helper function
+void sp_file_text_run_recursive(void (*f)(SPObject *), SPObject *o)
+{
+    if (SP_IS_TEXT(o) || SP_IS_FLOWTEXT(o))
+        f(o);
+    else {
+        vector<SPObject *> cl = o->childList(false);
+        for (vector<SPObject *>::const_iterator ci = cl.begin(); ci != cl.end(); ++ci)
+            sp_file_text_run_recursive(f, *ci);
+    }
+}
+
+// does not work :(
+void fix_update(SPObject *o) { 
+    o->style->write();
+    o->updateRepr();
+}
+
+void sp_file_fix_text(SPDocument *doc)
+{
+    sp_file_text_run_recursive(fix_blank_line, doc->getRoot());
+    sp_file_text_run_recursive(fix_line_spacing, doc->getRoot());
+    sp_file_text_run_recursive(fix_font_name, doc->getRoot());
+    sp_file_text_run_recursive(fix_font_size, doc->getRoot());
+    sp_file_text_run_recursive(fix_update, doc->getRoot());
+}
+
+
+
+/*
+  Local Variables:
+  mode:c++
+  c-file-style:"stroustrup"
+  c-file-offsets:((innamespace . 0)(inline-open . 0)(case-label . +))
+  indent-tabs-mode:nil
+  fill-column:99
+  End:
+*/
+// vim: filetype=cpp:expandtab:shiftwidth=4:tabstop=8:softtabstop=4 :

=== modified file 'src/file.h'
--- src/file.h	2014-10-08 02:22:03 +0000
+++ src/file.h	2017-01-24 00:26:50 +0000
@@ -203,7 +203,7 @@
  * clean unused defs out of file
  */
 void sp_file_vacuum (SPDocument *doc);
-
+void sp_file_fix_text(SPDocument *doc);
 
 #endif // SEEN_SP_FILE_H
 

=== modified file 'src/main.cpp'
--- src/main.cpp	2017-01-18 20:32:49 +0000
+++ src/main.cpp	2017-01-24 00:26:50 +0000
@@ -177,6 +177,7 @@
     SP_ARG_SHELL,
     SP_ARG_VERSION,
     SP_ARG_VACUUM_DEFS,
+    SP_ARG_DO_NOT_FIX_PRE_92,
 #ifdef WITH_DBUS
     SP_ARG_DBUS_LISTEN,
     SP_ARG_DBUS_NAME,
@@ -241,7 +242,6 @@
 static gchar *sp_export_svg_utf8 = NULL;
 static gchar *sp_global_printer_utf8 = NULL;
 
-
 /**
  *  Reset variables to default values.
  */
@@ -280,6 +280,7 @@
         sp_query_all = FALSE;
         sp_query_id = NULL;
         sp_vacuum_defs = FALSE;
+        sp_do_not_fix_pre_92 = FALSE;
 #ifdef WITH_DBUS
         sp_dbus_listen = FALSE;
         sp_dbus_name = NULL;
@@ -526,6 +527,11 @@
      N_("Start Inkscape in interactive shell mode."),
      NULL},
 
+    {"do-not-fix-pre-92", 0,
+    POPT_ARG_NONE, &sp_do_not_fix_pre_92, SP_ARG_DO_NOT_FIX_PRE_92,
+    N_("Prevents automatic fix of pre-92 files on opening them."),
+    NULL},
+
     POPT_AUTOHELP POPT_TABLEEND
 };