Add documentation and remove naming inconsistencies.

include/mapnik/text/itemizer.hpp

@@ -38,10 +38,13 @@
 namespace mapnik
 {
 
+
 struct text_item
 {
-    unsigned start; //First char
+    /** First char (UTF16 offset) */
-    unsigned end; //First char after this item
+    unsigned start;
+    /** Char _after_ the last char (UTF16 offset) */
+    unsigned end;
     UScriptCode script;
     char_properties_ptr format;
     UBiDiDirection rtl;
@@ -64,12 +67,12 @@ public:
     void add_text(UnicodeString str, char_properties_ptr format);
     std::list<text_item> const& itemize(unsigned start=0, unsigned end=0);
     void clear();
-    UnicodeString const& get_text() const { return text_; }
+    UnicodeString const& text() const { return text_; }
     /** Returns the start and end position of a certain line.
      *
      * Only forced line breaks with \n characters are handled here.
      */
-    std::pair<unsigned, unsigned> get_line(unsigned i) const;
+    std::pair<unsigned, unsigned> line(unsigned i) const;
     unsigned num_lines() const;
 private:
     template<typename T> struct run

include/mapnik/text/layout.hpp

@@ -51,7 +51,7 @@ public:
     typedef std::vector<glyph_info> glyph_vector;
     typedef glyph_vector::const_iterator const_iterator;
     /** Get glyph vector. */
-    glyph_vector const& get_glyphs() const { return glyphs_; }
+    glyph_vector const& glyphs() const { return glyphs_; }
     /** Append glyph. */
     void add_glyph(glyph_info const& glyph, double scale_factor_);
 
@@ -79,9 +79,9 @@ public:
     void set_first_line(bool first_line);
 
     /** Index of first UTF-16 char. */
-    unsigned get_first_char() const;
+    unsigned first_char() const;
     /** Index of last UTF-16 char. */
-    unsigned get_last_char() const;
+    unsigned last_char() const;
 
     /** Number of glyphs. */
     unsigned size() const;
@@ -108,7 +108,7 @@ public:
     void add_text(UnicodeString const& str, char_properties_ptr format);
 
     /** Returns the complete text stored in this layout.*/
-    UnicodeString const& get_text() const;
+    UnicodeString const& text() const;
 
     /** Processes the text into a list of glyphs, performing RTL/LTR handling, shaping and line breaking. */
     void layout(double wrap_width, unsigned text_ratio, bool wrap_before);

src/text/itemizer.cpp

@@ -69,7 +69,7 @@ void text_itemizer::clear()
     forced_line_breaks_.push_back(0);
 }
 
-std::pair<unsigned, unsigned> text_itemizer::get_line(unsigned i) const
+std::pair<unsigned, unsigned> text_itemizer::line(unsigned i) const
 {
 #ifdef MAPNIK_DEBUG
     if (i >= forced_line_breaks_.size()) return std::make_pair(0, 0);

src/text/layout.cpp

@@ -43,9 +43,9 @@ void text_layout::add_text(const UnicodeString &str, char_properties_ptr format)
     itemizer_.add_text(str, format);
 }
 
-const UnicodeString &text_layout::get_text() const
+const UnicodeString &text_layout::text() const
 {
-    return itemizer_.get_text();
+    return itemizer_.text();
 }
 
 void text_layout::layout(double wrap_width, unsigned text_ratio, bool wrap_before)
@@ -53,16 +53,19 @@ void text_layout::layout(double wrap_width, unsigned text_ratio, bool wrap_befor
     unsigned num_lines = itemizer_.num_lines();
     for (unsigned i = 0; i < num_lines; i++)
     {
-        std::pair<unsigned, unsigned> line_limits = itemizer_.get_line(i);
+        std::pair<unsigned, unsigned> line_limits = itemizer_.line(i);
         text_line_ptr line = boost::make_shared<text_line>(line_limits.first, line_limits.second);
-        shape_text(line);
         break_line(line, wrap_width, text_ratio, wrap_before); //Break line if neccessary
     }
 }
 
-
+/* In the Unicode string characters are always stored in logical order.
+ * This makes line breaking easy. One word is added to the current line at a time. Once the line is too long
+ * we either go back one step or inset the line break at the current position (depending on "wrap_before" setting).
+ * At the end everything that is left over is added as the final line. */
 void text_layout::break_line(text_line_ptr line, double wrap_width, unsigned text_ratio, bool wrap_before)
 {
+    shape_text(line);
     if (!wrap_width || line->width() < wrap_width)
     {
         add_line(line);
@@ -78,19 +81,22 @@ void text_layout::break_line(text_line_ptr line, double wrap_width, unsigned tex
         wrap_width = wrap_at;
     }
 
-    UnicodeString const& text = itemizer_.get_text();
+    UnicodeString const& text = itemizer_.text();
     Locale locale; //TODO: Is the default constructor correct?
     UErrorCode status = U_ZERO_ERROR;
     BreakIterator *breakitr = BreakIterator::createLineInstance(locale, status);
+
     //Not breaking the text if an error occurs is probably the best thing we can do.
     if (!U_SUCCESS(status)) {
         add_line(line);
         return;
     }
+
     breakitr->setText(text);
-    unsigned current_line_length = 0;
+
+    double current_line_length = 0;
     unsigned last_break_position = 0;
-    for (unsigned i=line->get_first_char(); i<line->get_last_char(); i++)
+    for (unsigned i=line->first_char(); i<line->last_char(); i++)
     {
         //TODO: character_spacing
         std::map<unsigned, double>::const_iterator width_itr = width_map_.find(i);
@@ -98,38 +104,44 @@ void text_layout::break_line(text_line_ptr line, double wrap_width, unsigned tex
         {
             current_line_length += width_itr->second;
         }
-        if (current_line_length > wrap_width)
+        if (current_line_length <= wrap_width) continue;
+        /***********************************************/
+
+
+        unsigned break_position = wrap_before ? breakitr->preceding(i) : breakitr->following(i);
+        /* Break iterator operates on the whole string, while we only look at one line. So we need to
+         * clamp break values. */
+        if (break_position < line->first_char()) break_position = line->first_char();
+        if (break_position > line->last_char()) break_position = line->last_char();
+
+        if (break_position <= last_break_position || break_position == BreakIterator::DONE)
         {
-            unsigned break_position = wrap_before ? breakitr->preceding(i) : breakitr->following(i);
+            //A single word is longer than the maximum line width.
-            if (break_position <= last_break_position || break_position == BreakIterator::DONE)
+            //Violate line width requirement and choose next break position
+            break_position = breakitr->following(i);
+            if (break_position == BreakIterator::DONE)
             {
-                //A single word is longer than the maximum line width.
+                break_position = line->last_char();
-                //Violate line width requirement and choose next break position
+                MAPNIK_LOG_WARN(text_layout) << "Unexpected result in break_line. Trying to recover...\n";
-                break_position = breakitr->following(i);
-                if (break_position == BreakIterator::DONE)
-                {
-                    break_position = line->get_last_char();
-                    MAPNIK_LOG_WARN(text_layout) << "Unexpected result in break_line. Trying to recover...\n";
-                }
             }
-            text_line_ptr new_line = boost::make_shared<text_line>(last_break_position, break_position);
-            clear_cluster_widths(last_break_position, break_position);
-            shape_text(new_line);
-            add_line(new_line);
-            last_break_position = break_position;
-            i = break_position - 1;
-            current_line_length = 0;
         }
+        text_line_ptr new_line = boost::make_shared<text_line>(last_break_position, break_position);
+        clear_cluster_widths(last_break_position, break_position);
+        shape_text(new_line);
+        add_line(new_line);
+        last_break_position = break_position;
+        i = break_position - 1;
+        current_line_length = 0;
     }
     if (last_break_position == 0)
     {
-        //No line breaks => no reshaping
+        //No line breaks => no reshaping required
         add_line(line);
     }
-    else if (last_break_position != line->get_last_char())
+    else if (last_break_position != line->last_char())
     {
-        text_line_ptr new_line = boost::make_shared<text_line>(last_break_position, line->get_last_char());
+        text_line_ptr new_line = boost::make_shared<text_line>(last_break_position, line->last_char());
-        clear_cluster_widths(last_break_position, line->get_last_char());
+        clear_cluster_widths(last_break_position, line->last_char());
         shape_text(new_line);
         add_line(new_line);
     }
@@ -257,12 +269,12 @@ void text_line::set_first_line(bool first_line)
     first_line_ = first_line;
 }
 
-unsigned text_line::get_first_char() const
+unsigned text_line::first_char() const
 {
     return first_char_;
 }
 
-unsigned text_line::get_last_char() const
+unsigned text_line::last_char() const
 {
     return last_char_;
 }

src/text/layout_shape_dummy.cpp

@@ -49,9 +49,9 @@ namespace mapnik
 
 void text_layout::shape_text(text_line_ptr line)
 {
-    unsigned start = line->get_first_char();
+    unsigned start = line->first_char();
-    unsigned end = line->get_last_char();
+    unsigned end = line->last_char();
-    UnicodeString const& text = itemizer_.get_text();
+    UnicodeString const& text = itemizer_.text();
 
     size_t length = end - start;
 

src/text/layout_shape_hb.cpp

@@ -38,9 +38,9 @@ namespace mapnik
 
 void text_layout::shape_text(text_line_ptr line)
 {
-    unsigned start = line->get_first_char();
+    unsigned start = line->first_char();
-    unsigned end = line->get_last_char();
+    unsigned end = line->last_char();
-    UnicodeString const& text = itemizer_.get_text();
+    UnicodeString const& text = itemizer_.text();
 
     size_t length = end - start;
     if (!length) return;

src/text/placement_finder.cpp

@@ -256,7 +256,7 @@ bool placement_finder::find_point_placement(pixel_position pos)
     if (collision(bbox)) return false;
     /* add_marker first checks for collision and then updates the detector.*/
     if (has_marker_ && !add_marker(glyphs, pos)) return false;
-    if (layout_.size()) detector_.insert(bbox, layout_.get_text());
+    if (layout_.size()) detector_.insert(bbox, layout_.text());
 
     /* IMPORTANT NOTE:
        x and y are relative to the center of the text
@@ -375,7 +375,7 @@ bool placement_finder::single_line_placement(vertex_cache &pp, text_upright_e or
 
     glyph_positions_ptr glyphs = boost::make_shared<glyph_positions>();
     std::vector<box2d<double> > bboxes;
-    bboxes.reserve(layout_.get_text().length());
+    bboxes.reserve(layout_.text().length());
     unsigned upside_down_glyph_count = 0;
 
 
@@ -445,7 +445,7 @@ bool placement_finder::single_line_placement(vertex_cache &pp, text_upright_e or
         //See comment above
         offset -= sign * (*line_itr)->height()/2;
     }
-    if (upside_down_glyph_count > layout_.get_text().length()/2)
+    if (upside_down_glyph_count > layout_.text().length()/2)
     {
         if (orientation == UPRIGHT_AUTO)
         {
@@ -461,7 +461,7 @@ bool placement_finder::single_line_placement(vertex_cache &pp, text_upright_e or
     }
     BOOST_FOREACH(box2d<double> bbox, bboxes)
     {
-        detector_.insert(bbox, layout_.get_text());
+        detector_.insert(bbox, layout_.text());
     }
     placements_.push_back(glyphs);
     return true;