Fix keyboard shortcuts for changing line spacing.

Many bug fixes to ensure proper behavior for the different line spacing modes.
Return focus to canvas when buttons are pushed in text tool bar.
Change step size to a reasonable value for line height when unit not percent.
Fix some issues with history for changes to line spacing in flowed text.

2 files changed, 221 insertions, 112 deletions

diff --git a/src/text-editing.cpp b/src/text-editing.cpp
index 193f4b15a..8bcdd3407 100644
--- a/src/text-editing.cpp
+++ b/src/text-editing.cpp
@@ -1214,6 +1214,7 @@ sp_te_adjust_tspan_letterspacing_screen(SPItem *text, Inkscape::Text::Layout::it
     text->requestDisplayUpdate(SP_OBJECT_MODIFIED_FLAG | SP_TEXT_LAYOUT_MODIFIED_FLAG);
 }
 
+// Only used for page-up and page-down and sp_te_adjust_linespacing_screen
 double
 sp_te_get_average_linespacing (SPItem *text)
 {
@@ -1227,6 +1228,91 @@ sp_te_get_average_linespacing (SPItem *text)
     return average_line_height;
 }
 
+/** Adjust the line height by 'amount'.
+ *  If top_level is true then objects without 'line-height' set or withwill get a set value,
+ *  otherwise objects that inherit line-height will not get onw=e.
+ */
+void
+sp_te_adjust_line_height (SPObject *object, double amount, double average, bool top_level = true) {
+
+    SPStyle *style = object->style;
+
+    // Always set if top level true.
+    // Also set if line_height is set to a non-zero value.
+    if (top_level ||
+        (style->line_height.set && !style->line_height.inherit && !style->line_height.computed == 0)){
+
+        // Scale default values
+        if (!style->line_height.set || style->line_height.inherit || style->line_height.normal) {
+            style->line_height.set = TRUE;
+            style->line_height.inherit = FALSE;
+            style->line_height.normal = FALSE;
+            style->line_height.unit = SP_CSS_UNIT_NONE;
+            style->line_height.value = style->line_height.computed = Inkscape::Text::Layout::LINE_HEIGHT_NORMAL;
+        }
+
+        switch (style->line_height.unit) {
+
+            case SP_CSS_UNIT_NONE:
+            default:
+                // Multiplier-type units, stored in computed
+                if (fabs(style->line_height.computed) < 0.001) {
+                    style->line_height.computed = amount < 0.0 ? -0.001 : 0.001;
+                    // the formula below could get stuck at zero
+                } else {
+                    style->line_height.computed *= (average + amount) / average;
+                }
+                style->line_height.value = style->line_height.computed;
+                break;
+
+
+            // Relative units, stored in value
+            case SP_CSS_UNIT_EM:
+            case SP_CSS_UNIT_EX:
+            case SP_CSS_UNIT_PERCENT:
+                if (fabs(style->line_height.value) < 0.001) {
+                    style->line_height.value = amount < 0.0 ? -0.001 : 0.001;
+                } else {
+                    style->line_height.value *= (average + amount) / average;
+                }
+                break;
+
+
+            // Absolute units
+            case SP_CSS_UNIT_PX:
+                style->line_height.computed += amount;
+                style->line_height.value = style->line_height.computed;
+                break;
+            case SP_CSS_UNIT_PT:
+                style->line_height.computed += Inkscape::Util::Quantity::convert(amount, "px", "pt");
+                style->line_height.value = style->line_height.computed;
+                break;
+            case SP_CSS_UNIT_PC:
+                style->line_height.computed += Inkscape::Util::Quantity::convert(amount, "px", "pc");
+                style->line_height.value = style->line_height.computed;
+                break;
+            case SP_CSS_UNIT_MM:
+                style->line_height.computed += Inkscape::Util::Quantity::convert(amount, "px", "mm");
+                style->line_height.value = style->line_height.computed;
+                break;
+            case SP_CSS_UNIT_CM:
+                style->line_height.computed += Inkscape::Util::Quantity::convert(amount, "px", "cm");
+                style->line_height.value = style->line_height.computed;
+                break;
+            case SP_CSS_UNIT_IN:
+                style->line_height.computed += Inkscape::Util::Quantity::convert(amount, "px", "in");
+                style->line_height.value = style->line_height.computed;
+                break;
+        }
+        object->updateRepr();
+    }
+
+    std::vector<SPObject*> children = object->childList(false);
+    for (auto child: children) {
+        sp_te_adjust_line_height (child, amount, average, false);
+    }
+}
+
 void
 sp_te_adjust_linespacing_screen (SPItem *text, Inkscape::Text::Layout::iterator const &/*start*/, Inkscape::Text::Layout::iterator const &/*end*/, SPDesktop *desktop, gdouble by)
 {
@@ -1235,71 +1321,30 @@ sp_te_adjust_linespacing_screen (SPItem *text, Inkscape::Text::Layout::iterator
     g_return_if_fail (SP_IS_TEXT(text) || SP_IS_FLOWTEXT(text));
 
     Inkscape::Text::Layout const *layout = te_get_layout(text);
-    SPStyle *style = text->style;
 
-    if (!style->line_height.set || style->line_height.inherit || style->line_height.normal) {
-        style->line_height.set = TRUE;
-        style->line_height.inherit = FALSE;
-        style->line_height.normal = FALSE;
-        style->line_height.unit = SP_CSS_UNIT_PERCENT;
-        style->line_height.value = style->line_height.computed = Inkscape::Text::Layout::LINE_HEIGHT_NORMAL;
-    }
-
-    unsigned line_count = layout->lineIndex(layout->end());
-    double all_lines_height = layout->characterAnchorPoint(layout->end())[Geom::Y] - layout->characterAnchorPoint(layout->begin())[Geom::Y];
-    double average_line_height = all_lines_height / (line_count == 0 ? 1 : line_count);
+    double average_line_height = sp_te_get_average_linespacing (text);
     if (fabs(average_line_height) < 0.001) average_line_height = 0.001;
 
     // divide increment by zoom and by the number of lines,
     // so that the entire object is expanded by by pixels
+    unsigned line_count = layout->lineIndex(layout->end());
     gdouble zby = by / (desktop->current_zoom() * (line_count == 0 ? 1 : line_count));
 
     // divide increment by matrix expansion
     Geom::Affine t(text->i2doc_affine());
     zby = zby / t.descrim();
 
-    switch (style->line_height.unit) {
-        case SP_CSS_UNIT_NONE:
-        default:
-            // multiplier-type units, stored in computed
-            if (fabs(style->line_height.computed) < 0.001) style->line_height.computed = by < 0.0 ? -0.001 : 0.001;    // the formula below could get stuck at zero
-            else style->line_height.computed *= (average_line_height + zby) / average_line_height;
-            style->line_height.value = style->line_height.computed;
-            break;
-        case SP_CSS_UNIT_EM:
-        case SP_CSS_UNIT_EX:
-        case SP_CSS_UNIT_PERCENT:
-            // multiplier-type units, stored in value
-            if (fabs(style->line_height.value) < 0.001) style->line_height.value = by < 0.0 ? -0.001 : 0.001;
-            else style->line_height.value *= (average_line_height + zby) / average_line_height;
-            break;
-            // absolute-type units
-        case SP_CSS_UNIT_PX:
-            style->line_height.computed += zby;
-            style->line_height.value = style->line_height.computed;
-            break;
-        case SP_CSS_UNIT_PT:
-            style->line_height.computed += Inkscape::Util::Quantity::convert(zby, "px", "pt");
-            style->line_height.value = style->line_height.computed;
-            break;
-        case SP_CSS_UNIT_PC:
-            style->line_height.computed += (Inkscape::Util::Quantity::convert(zby, "px", "pt") / 12);
-            style->line_height.value = style->line_height.computed;
-            break;
-        case SP_CSS_UNIT_MM:
-            style->line_height.computed += Inkscape::Util::Quantity::convert(zby, "px", "mm");
-            style->line_height.value = style->line_height.computed;
-            break;
-        case SP_CSS_UNIT_CM:
-            style->line_height.computed += Inkscape::Util::Quantity::convert(zby, "px", "cm");
-            style->line_height.value = style->line_height.computed;
-            break;
-        case SP_CSS_UNIT_IN:
-            style->line_height.computed += Inkscape::Util::Quantity::convert(zby, "px", "in");
-            style->line_height.value = style->line_height.computed;
-            break;
+    Inkscape::Preferences *prefs = Inkscape::Preferences::get();
+    gint mode = prefs->getInt("/tools/text/line_spacing_mode", 0);
+    if (mode == 0) { // Adaptive: <text> line-spacing is zero, only scale children.
+        std::vector<SPObject*> children = text->childList(false);
+        for (auto child: children) {
+            sp_te_adjust_line_height (child, zby, average_line_height, false);
+        }
+    } else {
+        sp_te_adjust_line_height (text, zby, average_line_height, true);
     }
-    text->updateRepr();
+
     text->requestDisplayUpdate(SP_OBJECT_MODIFIED_FLAG | SP_TEXT_LAYOUT_MODIFIED_FLAG);
 }
 
diff --git a/src/widgets/text-toolbar.cpp b/src/widgets/text-toolbar.cpp
index 0cd083432..8c7bec75b 100644
--- a/src/widgets/text-toolbar.cpp
+++ b/src/widgets/text-toolbar.cpp
@@ -15,11 +15,13 @@
  *   Tavmjong Bah <tavmjong@free.fr>
  *   Abhishek Sharma
  *   Kris De Gussem <Kris.DeGussem@gmail.com>
+ *   Tavmjong Bah <tavmjong@free.fr>
  *
  * Copyright (C) 2004 David Turner
  * Copyright (C) 2003 MenTaLguY
  * Copyright (C) 2001-2002 Ximian, Inc.
  * Copyright (C) 1999-2013 authors
+ * Copyright (C) 2017 Tavmjong Bah
  *
  * Released under GNU GPL, read the file 'COPYING' for more information
  */
@@ -577,7 +579,7 @@ static void sp_text_align_mode_changed( GObject *tbl, int mode )
     }
     sp_repr_css_attr_unref (css);
 
-    gtk_widget_grab_focus (GTK_WIDGET(desktop->canvas));
+    gtk_widget_grab_focus (GTK_WIDGET(SP_ACTIVE_DESKTOP->canvas));
 
     g_object_set_data( tbl, "freeze", GINT_TO_POINTER(FALSE) );
 }
@@ -586,6 +588,64 @@ static bool is_relative( Unit const *unit ) {
     return (unit->abbr == "" || unit->abbr == "em" || unit->abbr == "ex" || unit->abbr == "%");
 }
 
+// Set property for object, but unset all descendents
+// Should probably be moved to desktop_style.cpp
+static void recursively_set_properties( SPObject* object, SPCSSAttr *css ) {
+    object->changeCSS (css, "style");
+
+    SPCSSAttr *css_unset = sp_repr_css_attr_unset_all( css );
+    std::vector<SPObject *> children = object->childList(false);
+    for (auto i: children) {
+        recursively_set_properties (i, css_unset);
+    }
+    sp_repr_css_attr_unref (css_unset);
+}
+
+// Apply line height changes (line-height value changed or line-height unit changed)
+static void set_lineheight (SPCSSAttr *css) {
+
+    Inkscape::Preferences *prefs = Inkscape::Preferences::get();
+    bool outer = prefs->getInt("/tools/text/outer_style", false);
+    gint mode  = prefs->getInt("/tools/text/line_spacing_mode", 0);
+    SPDesktop *desktop = SP_ACTIVE_DESKTOP;
+
+    // Calling sp_desktop_set_style will result in a call to TextTool::_styleSet() which
+    // will set the style on selected text inside the <text> element. If we want to set
+    // the style on the outer <text> objects we need to bypass this call.
+    if ( mode == 3 && !outer ) {
+        // This will call sp_te_apply_style via signal
+        sp_desktop_set_style (desktop, css, true, true);
+    } else {
+        Inkscape::Selection *selection = desktop->getSelection();
+        auto itemlist= selection->items();
+        for (auto i: itemlist) {
+
+            if (dynamic_cast<SPText *>(i) || dynamic_cast<SPFlowtext *>(i)) {
+                SPItem *item = i;
+
+                // Scale by inverse of accumulated parent transform
+                SPCSSAttr *css_set = sp_repr_css_attr_new();
+                sp_repr_css_merge(css_set, css);
+                Geom::Affine const local(item->i2doc_affine());
+                double const ex(local.descrim());
+                if ( (ex != 0.0) && (ex != 1.0) ) {
+                    sp_css_attr_scale(css_set, 1/ex);
+                }
+
+                if ( mode == 1 || mode == 2 || mode == 3) {  // Minimum, Even, or Adjustable w/ outer.
+                    // We change only outer style
+                    item->changeCSS(css_set,"style");
+                } else {
+                    // We change only inner style (Adaptive).
+                    for (auto child: item->childList(false)) {
+                        child->changeCSS(css_set,"style");
+                    }
+                }
+            }
+        }
+    }
+}
+
 static void sp_text_lineheight_value_changed( GtkAdjustment *adj, GObject *tbl )
 {
     // quit if run by the _changed callbacks
@@ -622,56 +682,28 @@ static void sp_text_lineheight_value_changed( GtkAdjustment *adj, GObject *tbl )
     }
     sp_repr_css_set_property (css, "line-height", osfs.str().c_str());
 
-
-    // Apply line-height to selected objects. See comment in font size function.
-    bool outer = prefs->getInt("/tools/text/outer_style", false);
-    SPDesktop *desktop = SP_ACTIVE_DESKTOP;
-    if (outer) {
-        Inkscape::Selection *selection = desktop->getSelection();
-        auto itemlist= selection->items();
-        for(auto i=itemlist.begin();i!=itemlist.end(); ++i){
-            if (dynamic_cast<SPText *>(*i) || dynamic_cast<SPFlowtext *>(*i)) {
-                SPItem *item = *i;
-
-                // Scale by inverse of accumulated parent transform
-                SPCSSAttr *css_set = sp_repr_css_attr_new();
-                sp_repr_css_merge(css_set, css);
-                Geom::Affine const local(item->i2doc_affine());
-                double const ex(local.descrim());
-                if ( (ex != 0.0) && (ex != 1.0) ) {
-                    sp_css_attr_scale(css_set, 1/ex);
-                }
-
-                item->changeCSS(css_set,"style");
-
-                sp_repr_css_attr_unref(css_set);
-            }
-        }
-    } else {
-        sp_desktop_set_style (desktop, css, true, true);
-    }
-
-
+    // Internal function to set line-height which is spacing mode dependent.
+    set_lineheight (css);
 
     // Only need to save for undo if a text item has been changed.
-    Inkscape::Selection *selection = desktop->getSelection();
+    Inkscape::Selection *selection = SP_ACTIVE_DESKTOP->getSelection();
     bool modmade = false;
     auto itemlist= selection->items();
     for(auto i=itemlist.begin();i!=itemlist.end(); ++i){
-        if (SP_IS_TEXT (*i)) {
+        if (dynamic_cast<SPText *>(*i) || dynamic_cast<SPFlowtext *>(*i)) {
             modmade = true;
         }
     }
 
     // Save for undo
-    if(modmade) {
+    if (modmade) {
         // Call ensureUpToDate() causes rebuild of text layout (with all proper style
         // cascading, etc.). For multi-line text with sodipodi::role="line", we must explicitly
         // save new <tspan> 'x' and 'y' attribute values by calling updateRepr().
         // Partial fix for bug #1590141.
-        desktop->getDocument()->ensureUpToDate();
+        SP_ACTIVE_DESKTOP->getDocument()->ensureUpToDate();
         for(auto i=itemlist.begin();i!=itemlist.end(); ++i){
-            if (SP_IS_TEXT (*i)) {
+            if (dynamic_cast<SPText *>(*i) || dynamic_cast<SPFlowtext *>(*i)) {
                 (*i)->updateRepr();
             }
         }
@@ -817,13 +849,22 @@ static void sp_text_lineheight_unit_changed( gpointer /* */, GObject *tbl )
     // Update GUI with line_height value.
     gtk_adjustment_set_value(line_height_adj, line_height);
 
-    // Apply line-height to selected objects.
-    sp_desktop_set_style (desktop, css, true, false);
+    // Update "climb rate"  The custom action has a step property but no way to set it.
+    if (unit->abbr == "%") {
+        gtk_adjustment_set_step_increment (line_height_adj,  1.0);
+        gtk_adjustment_set_page_increment (line_height_adj, 10.0);
+    } else {
+        gtk_adjustment_set_step_increment (line_height_adj,  0.1);
+        gtk_adjustment_set_page_increment (line_height_adj,  1.0);
+    }
+
+    // Internal function to set line-height which is spacing mode dependent.
+    set_lineheight (css);
 
     // Only need to save for undo if a text item has been changed.
     bool modmade = false;
     for(auto i=itemlist.begin();i!=itemlist.end(); ++i){
-        if (SP_IS_TEXT (*i)) {
+        if (dynamic_cast<SPText *>(*i) || dynamic_cast<SPFlowtext *>(*i)) {
             modmade = true;
         }
     }
@@ -849,19 +890,6 @@ static void sp_text_lineheight_unit_changed( gpointer /* */, GObject *tbl )
     g_object_set_data( tbl, "freeze", GINT_TO_POINTER(FALSE) );
 }
 
-// Set property for object, but unset all descendents
-// Should probably be moved to desktop_style.cpp
-static void recursively_set_properties( SPObject* object, SPCSSAttr *css ) {
-    object->changeCSS (css, "style");
-
-    SPCSSAttr *css_unset = sp_repr_css_attr_unset_all( css );
-    std::vector<SPObject *> children = object->childList(false);
-    for (auto i: children) {
-        recursively_set_properties (i, css_unset);
-    }
-    sp_repr_css_attr_unref (css_unset);
-}
-
 //
 static void sp_text_line_spacing_mode_changed( GObject *tbl, int mode )
 {
@@ -1004,12 +1032,30 @@ static void sp_text_line_spacing_mode_changed( GObject *tbl, int mode )
             break;
     }
 
-            // Update widgets to reflect new state of Text Outer Style button.
-    sp_text_toolbox_selection_changed( NULL, tbl );
+    // Outer style toggle set per mode so that line height widget should be enabled.
+    GtkAction* lineHeightAction = GTK_ACTION( g_object_get_data( tbl, "TextLineHeightAction" ) );
+    gtk_action_set_sensitive (lineHeightAction, true);
+
+    // Update "climb rate"
+    EgeAdjustmentAction *line_height_act =
+        reinterpret_cast<EgeAdjustmentAction *>(g_object_get_data(tbl, "TextLineHeightAction"));
+    GtkAdjustment *line_height_adj = ege_adjustment_action_get_adjustment( line_height_act );
+    UnitTracker *tracker = reinterpret_cast<UnitTracker*>(g_object_get_data(tbl, "tracker"));
+    Unit const *unit = tracker->getActiveUnit();
+
+    if (unit->abbr == "%") {
+        gtk_adjustment_set_step_increment (line_height_adj,  1.0);
+        gtk_adjustment_set_page_increment (line_height_adj, 10.0);
+    } else {
+        gtk_adjustment_set_step_increment (line_height_adj,  0.1);
+        gtk_adjustment_set_page_increment (line_height_adj,  1.0);
+    }
 
     DocumentUndo::done(desktop->getDocument(), SP_VERB_CONTEXT_TEXT,
                        _("Text: Change line spacing mode"));
 
+    gtk_widget_grab_focus (GTK_WIDGET(desktop->canvas));
+
     g_object_set_data( tbl, "freeze", GINT_TO_POINTER(FALSE) );
 }
 
@@ -1245,6 +1291,8 @@ static void sp_writing_mode_changed( GObject* tbl, int mode )
     }
     sp_repr_css_attr_unref (css);
 
+    gtk_widget_grab_focus (GTK_WIDGET(SP_ACTIVE_DESKTOP->canvas));
+
     g_object_set_data( tbl, "freeze", GINT_TO_POINTER(FALSE) );
 }
 
@@ -1297,6 +1345,8 @@ static void sp_text_orientation_changed( GObject* tbl, int mode )
     }
     sp_repr_css_attr_unref (css);
 
+    gtk_widget_grab_focus (GTK_WIDGET(SP_ACTIVE_DESKTOP->canvas));
+
     g_object_set_data( tbl, "freeze", GINT_TO_POINTER(FALSE) );
 }
 
@@ -1343,6 +1393,8 @@ static void sp_text_direction_changed( GObject *tbl, int mode )
     }
     sp_repr_css_attr_unref (css);
 
+    gtk_widget_grab_focus (GTK_WIDGET(SP_ACTIVE_DESKTOP->canvas));
+
     g_object_set_data( tbl, "freeze", GINT_TO_POINTER(FALSE) );
 }
 
@@ -1626,6 +1678,15 @@ static void sp_text_toolbox_selection_changed(Inkscape::Selection */*selection*/
             ege_adjustment_action_get_adjustment(EGE_ADJUSTMENT_ACTION( lineHeightAction ));
         gtk_adjustment_set_value( lineHeightAdjustment, height );
 
+        // Update "climb rate"
+        if (line_height_unit == SP_CSS_UNIT_PERCENT) {
+            gtk_adjustment_set_step_increment (lineHeightAdjustment,  1.0);
+            gtk_adjustment_set_page_increment (lineHeightAdjustment, 10.0);
+        } else {
+            gtk_adjustment_set_step_increment (lineHeightAdjustment,  0.1);
+            gtk_adjustment_set_page_increment (lineHeightAdjustment,  1.0);
+        }
+
         UnitTracker* tracker = reinterpret_cast<UnitTracker*>( g_object_get_data( tbl, "tracker" ) );
         if( line_height_unit == SP_CSS_UNIT_NONE ) {
             // Function 'sp_style_get_css_unit_string' returns 'px' for unit none.
@@ -1692,7 +1753,10 @@ static void sp_text_toolbox_selection_changed(Inkscape::Selection */*selection*/
         if (mode[0]  > 0 && mode[1] == 0 && mode[2] == 0 && mode[3] == 0) activeButtonLS = 0;
         if (mode[0] == 0 && mode[1]  > 0 && mode[2] == 0 && mode[3] == 0) activeButtonLS = 1;
         if (mode[0] == 0 && mode[1] == 0 && mode[2]  > 0 && mode[3] == 0) activeButtonLS = 2;
-
+        // std::cout << "  modes: " << mode[0]
+        //           << ", "<< mode[1]
+        //           << ", "<< mode[2]
+        //           << ", "<< mode[3] << std::endl;
         InkSelectOneAction* textLineSpacingAction =
             static_cast<InkSelectOneAction*>( g_object_get_data( tbl, "TextLineSpacingAction" ) );
         textLineSpacingAction->set_active( activeButtonLS );
@@ -1707,10 +1771,10 @@ static void sp_text_toolbox_selection_changed(Inkscape::Selection */*selection*/
             gtk_action_set_sensitive (lineHeightAction, true);
         }
 
-        // In Minimum mode, don't allow unit change (must remain unitless).
+        // In Minimum and Adaptive modes, don't allow unit change (must remain unitless).
         GtkAction* textLineHeightUnitsAction =
             static_cast<GtkAction*>(g_object_get_data( tbl, "TextLineHeightUnitsAction") );
-        if (activeButtonLS == 1 && outer) {
+        if (activeButtonLS == 0 || (activeButtonLS == 1 && outer)) {
             gtk_action_set_sensitive (textLineHeightUnitsAction, false);
         } else {
             gtk_action_set_sensitive (textLineHeightUnitsAction, true);
@@ -2238,7 +2302,7 @@ void sp_text_toolbox_prep(SPDesktop *desktop, GtkActionGroup* mainActions, GObje
             holder,                               /* dataKludge */
             FALSE,                                /* set alt-x keyboard shortcut? */
             NULL,                                 /* altx_mark */
-            0.0, 1000.0, 1.0, 10.0,               /* lower, upper, step (arrow up/down), page up/down */
+            0.0, 1000.0, 0.1, 1.0,                /* lower, upper, step (arrow up/down), page up/down */
             labels, values, G_N_ELEMENTS(labels), /* drop down menu */
             sp_text_lineheight_value_changed,     /* callback */
             NULL, // tracker,                     /* unit tracker */
@@ -2295,7 +2359,7 @@ void sp_text_toolbox_prep(SPDesktop *desktop, GtkActionGroup* mainActions, GObje
         InkSelectOneAction* act =
             InkSelectOneAction::create( "TextLineSpacingAction", // Name
                                         _("Line Spacing Mode"),   // Label
-                                        _("How should multiple lines be spaced."), // Tooltip
+                                        _("How should multiple baselines be spaced?\n Adapative: Line spacing adapts to font size.\n Minimum: Like Adaptive but with a set minimum.\n Even: Evenly spaced.\n Adjustable: No restrictions."), // Tooltip
                                         "Not Used",          // Icon
                                         store );             // Tree store
         act->use_radio( false );