Fix error wrapping in interactive selection input
authorTeemu Murtola <teemu.murtola@gmail.com>
Mon, 29 Jun 2015 10:02:11 +0000 (13:02 +0300)
committerTeemu Murtola <teemu.murtola@gmail.com>
Sat, 4 Jul 2015 19:02:07 +0000 (21:02 +0200)
Resolve a TODO that was introduced when making the selection prompting
use text streams about wrapping error messages like they used to be
wrapped.

The use of console width within the generic error formatting code is a
bit ugly, but that needs to be resolved separately (and the old code
also had the console width hardcoded in a similar level).

Change-Id: Ia832a4884eef9bded50431fb5008d8191f52c246

14 files changed:
src/gromacs/selection/selectioncollection.cpp
src/gromacs/utility/exceptions.cpp
src/gromacs/utility/stringutil.cpp
src/gromacs/utility/stringutil.h
src/gromacs/utility/tests/CMakeLists.txt
src/gromacs/utility/tests/refdata/TextLineWrapperTest_WrapsCorrectlyWithExtraWhitespace.xml
src/gromacs/utility/tests/refdata/TextWriterTest_WritesLines.xml [new file with mode: 0644]
src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInParts.xml [new file with mode: 0644]
src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInPartsWithWrapper.xml [new file with mode: 0644]
src/gromacs/utility/tests/refdata/TextWriterTest_WritesWrappedLines.xml [new file with mode: 0644]
src/gromacs/utility/tests/stringutil.cpp
src/gromacs/utility/tests/textwriter.cpp [new file with mode: 0644]
src/gromacs/utility/textwriter.cpp
src/gromacs/utility/textwriter.h

index d124cf2800bcc15109af43e2dcc3f4caa375b1bf..b7ee4d0b2d67b75835196179a9a6a4878c6f5727 100644 (file)
@@ -690,6 +690,7 @@ SelectionCollection::parseInteractive(int                count,
     if (statusStream != NULL)
     {
         statusWriter.reset(new TextWriter(statusStream));
+        statusWriter->wrapperSettings().setLineLength(78);
     }
     _gmx_sel_init_lexer(&scanner, &impl_->sc_, statusWriter.get(),
                         count, impl_->bExternalGroupsSet_, impl_->grps_);
index 1248b96ef7bec6b3933b59dcf2f9f92fed33f569..cb683dafb298772770a0a40534e2a967ce335ef7 100644 (file)
@@ -322,20 +322,19 @@ class MessageWriterTextWriter : public MessageWriterInterface
 
         virtual void writeLine(const char *text, int indent)
         {
-            // TODO: Line wrapping.
-            writer_->writeString(std::string(indent, ' '));
+            writer_->wrapperSettings().setIndent(indent);
             writer_->writeLine(text);
         }
         virtual void writeErrNoInfo(int errorNumber, const char *funcName,
                                     int indent)
         {
-            writeLine(formatString("Reason: %s", std::strerror(errorNumber)).c_str(),
-                      indent);
+            writer_->wrapperSettings().setIndent(indent);
+            writer_->writeLine(formatString("Reason: %s", std::strerror(errorNumber)));
             if (funcName != NULL)
             {
-                writeLine(formatString("(call to %s() returned error code %d)",
-                                       funcName, errorNumber).c_str(),
-                          indent);
+                writer_->writeLine(
+                        formatString("(call to %s() returned error code %d)",
+                                     funcName, errorNumber));
             }
         }
 
index 86abf82a085e9c370569609e6a6d188f994eb06f..54b32214926a49b5cd98658a72dbbbc0fe5f284b 100644 (file)
 namespace gmx
 {
 
-bool endsWith(const std::string &str, const char *suffix)
+bool endsWith(const char *str, const char *suffix)
 {
-    if (suffix == NULL || suffix[0] == '\0')
+    if (isNullOrEmpty(suffix))
     {
         return true;
     }
-    size_t length = std::strlen(suffix);
-    return (str.length() >= length
-            && str.compare(str.length() - length, length, suffix) == 0);
+    const size_t strLength    = std::strlen(str);
+    const size_t suffixLength = std::strlen(suffix);
+    return (strLength >= suffixLength
+            && std::strcmp(&str[strLength - suffixLength], suffix) == 0);
 }
 
 std::string stripSuffixIfPresent(const std::string &str, const char *suffix)
@@ -247,7 +248,7 @@ replaceAllWords(const std::string &input, const std::string &from,
 
 TextLineWrapperSettings::TextLineWrapperSettings()
     : maxLength_(0), indent_(0), firstLineIndent_(-1),
-      bStripLeadingWhitespace_(false), continuationChar_('\0')
+      bKeepFinalSpaces_(false), continuationChar_('\0')
 {
 }
 
@@ -256,13 +257,19 @@ TextLineWrapperSettings::TextLineWrapperSettings()
  * TextLineWrapper
  */
 
+bool TextLineWrapper::isTrivial() const
+{
+    return settings_.lineLength() == 0 && settings_.indent() == 0
+           && settings_.firstLineIndent_ <= 0;
+}
+
 size_t
 TextLineWrapper::findNextLine(const char *input, size_t lineStart) const
 {
     size_t inputLength = std::strlen(input);
     bool   bFirstLine  = (lineStart == 0 || input[lineStart - 1] == '\n');
     // Ignore leading whitespace if necessary.
-    if (!bFirstLine || settings_.bStripLeadingWhitespace_)
+    if (!bFirstLine)
     {
         lineStart += std::strspn(input + lineStart, " ");
         if (lineStart >= inputLength)
@@ -307,7 +314,7 @@ TextLineWrapper::formatLine(const std::string &input,
     size_t inputLength = input.length();
     bool   bFirstLine  = (lineStart == 0 || input[lineStart - 1] == '\n');
     // Strip leading whitespace if necessary.
-    if (!bFirstLine || settings_.bStripLeadingWhitespace_)
+    if (!bFirstLine)
     {
         lineStart = input.find_first_not_of(' ', lineStart);
         if (lineStart >= inputLength)
@@ -318,9 +325,12 @@ TextLineWrapper::formatLine(const std::string &input,
     int  indent        = (bFirstLine ? settings_.firstLineIndent() : settings_.indent());
     bool bContinuation = (lineEnd < inputLength && input[lineEnd - 1] != '\n');
     // Strip trailing whitespace.
-    while (lineEnd > lineStart && std::isspace(input[lineEnd - 1]))
+    if (!settings_.bKeepFinalSpaces_ || lineEnd < inputLength || input[inputLength - 1] == '\n')
     {
-        --lineEnd;
+        while (lineEnd > lineStart && std::isspace(input[lineEnd - 1]))
+        {
+            --lineEnd;
+        }
     }
 
     const size_t lineLength = lineEnd - lineStart;
index e7e11a6d9ef5c9bf5d1d3a04a7a7dd22add7df77..004daae3e0353708bfa75d731a5fe6fd2db4b0db 100644 (file)
@@ -59,7 +59,7 @@ namespace gmx
  *
  * Does not throw.
  */
-bool inline isNullOrEmpty(const char *str)
+static inline bool isNullOrEmpty(const char *str)
 {
     return str == NULL || str[0] == '\0';
 }
@@ -74,12 +74,12 @@ bool inline isNullOrEmpty(const char *str)
  * Returns true if \p prefix is empty.
  * Does not throw.
  */
-bool inline startsWith(const std::string &str, const std::string &prefix)
+static inline bool startsWith(const std::string &str, const std::string &prefix)
 {
     return str.compare(0, prefix.length(), prefix) == 0;
 }
 //! \copydoc startsWith(const std::string &, const std::string &)
-bool inline startsWith(const char *str, const char *prefix)
+static inline bool startsWith(const char *str, const char *prefix)
 {
     return std::strncmp(str, prefix, std::strlen(prefix)) == 0;
 }
@@ -94,7 +94,12 @@ bool inline startsWith(const char *str, const char *prefix)
  * Returns true if \p suffix is NULL or empty.
  * Does not throw.
  */
-bool endsWith(const std::string &str, const char *suffix);
+bool endsWith(const char *str, const char *suffix);
+//! \copydoc endsWith(const char *, const char *)
+static inline bool endsWith(const std::string &str, const char *suffix)
+{
+    return endsWith(str.c_str(), suffix);
+}
 
 /*! \brief
  * Removes a suffix from a string.
@@ -335,7 +340,7 @@ class TextLineWrapperSettings
          *  - No maximum line width (only explicit line breaks).
          *  - No indentation.
          *  - No continuation characters.
-         *  - Ignore whitespace after an explicit newline.
+         *  - Do not keep final spaces in input strings.
          */
         TextLineWrapperSettings();
 
@@ -366,18 +371,18 @@ class TextLineWrapperSettings
          */
         void setFirstLineIndent(int indent) { firstLineIndent_ = indent; }
         /*! \brief
-         * Sets whether to remove spaces after an explicit newline.
+         * Sets whether final spaces in input should be kept.
          *
-         * \param[in] bStrip  If true, spaces after newline are ignored.
+         * \param[in] bKeep  Whether to keep spaces at the end of the input.
          *
-         * If not removed, the space is added to the indentation set with
-         * setIndent().
-         * The default is to not strip such whitespace.
+         * This means that wrapping a string that ends in spaces also keeps
+         * those spaces in the output.  This allows using the wrapper for
+         * partial lines where the initial part of the line may end in a space.
+         * By default, all trailing whitespace is removed.  Note that this
+         * option does not affect spaces before an explicit newline: those are
+         * always removed.
          */
-        void setStripLeadingWhitespace(bool bStrip)
-        {
-            bStripLeadingWhitespace_ = bStrip;
-        }
+        void setKeepFinalSpaces(bool bKeep) { bKeepFinalSpaces_ = bKeep; }
         /*! \brief
          * Sets a continuation marker for wrapped lines.
          *
@@ -424,8 +429,8 @@ class TextLineWrapperSettings
          * If -1, \a indent_ is used.
          */
         int                     firstLineIndent_;
-        //! Whether to ignore or preserve space after a newline.
-        bool                    bStripLeadingWhitespace_;
+        //! Whether to keep spaces at end of input.
+        bool                    bKeepFinalSpaces_;
         //! If not \c '\0', mark each wrapping point with this character.
         char                    continuationChar_;
 
@@ -503,6 +508,9 @@ class TextLineWrapper
          */
         TextLineWrapperSettings &settings() { return settings_; }
 
+        //! Returns true if the wrapper would not modify the input string.
+        bool isTrivial() const;
+
         /*! \brief
          * Finds the next line to be wrapped.
          *
index a002c762c3f3d91d2d001eca20826f516356bf27..c980a24363ae2883e27f62dcf377ce9c0b1e2117 100644 (file)
@@ -35,4 +35,6 @@
 gmx_add_unit_test(UtilityUnitTests utility-test
                   arrayref.cpp
                   bitmask32.cpp bitmask64.cpp bitmask128.cpp
-                  stringutil.cpp)
+                  stringutil.cpp
+                  textwriter.cpp
+                  )
index f277fc930b97c4ba9836c38f9b0d203ea8c22603..1bb9fb422799f30449e826cc210bca64490a9c0a 100644 (file)
@@ -1,12 +1,7 @@
 <?xml version="1.0"?>
 <?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
 <ReferenceData>
-  <String Name="WrappedAt14StripLeading"><![CDATA[
-A quick brown
-fox jumps
-over the lazy
-dog]]></String>
-  <String Name="WrappedAt14PreserveLeading"><![CDATA[
+  <String Name="WrappedAt14"><![CDATA[
  A quick brown
 fox jumps
  over the lazy
diff --git a/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLines.xml b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLines.xml
new file mode 100644 (file)
index 0000000..25f4a2f
--- /dev/null
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <String Name="Output"><![CDATA[
+Explicit newline
+Implicit newline
+Explicit newline
+Implicit newline
+
+]]></String>
+</ReferenceData>
diff --git a/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInParts.xml b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInParts.xml
new file mode 100644 (file)
index 0000000..703467f
--- /dev/null
@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <String Name="Output"><![CDATA[
+Partial spaced line
+Partial spaced line
+]]></String>
+</ReferenceData>
diff --git a/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInPartsWithWrapper.xml b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInPartsWithWrapper.xml
new file mode 100644 (file)
index 0000000..703467f
--- /dev/null
@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <String Name="Output"><![CDATA[
+Partial spaced line
+Partial spaced line
+]]></String>
+</ReferenceData>
diff --git a/src/gromacs/utility/tests/refdata/TextWriterTest_WritesWrappedLines.xml b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesWrappedLines.xml
new file mode 100644 (file)
index 0000000..a87ee7d
--- /dev/null
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <String Name="Output"><![CDATA[
+  Wrapped and
+  indented text
+  Wrapped and
+  indented text
+
+]]></String>
+</ReferenceData>
index ba1683d06d236ae5a99f15c9506bb203c601a243..7145fdb109a48cddf46ce992ad2fa340e5fce318 100644 (file)
@@ -256,7 +256,23 @@ TEST_F(TextLineWrapperTest, HandlesEmptyStrings)
     EXPECT_EQ("", wrapper.wrapToString(""));
     EXPECT_EQ("", wrapper.wrapToString("   "));
     EXPECT_TRUE(wrapper.wrapToVector("").empty());
-    EXPECT_TRUE(wrapper.wrapToString("   ").empty());
+    {
+        std::vector<std::string> wrapped(wrapper.wrapToVector("   "));
+        ASSERT_EQ(1U, wrapped.size());
+        EXPECT_EQ("", wrapped[0]);
+    }
+}
+
+TEST_F(TextLineWrapperTest, HandlesTrailingWhitespace)
+{
+    gmx::TextLineWrapper wrapper;
+
+    EXPECT_EQ("line", wrapper.wrapToString("line   "));
+    EXPECT_EQ("line\n", wrapper.wrapToString("line   \n"));
+
+    wrapper.settings().setKeepFinalSpaces(true);
+    EXPECT_EQ("line   ", wrapper.wrapToString("line   "));
+    EXPECT_EQ("line\n", wrapper.wrapToString("line   \n"));
 }
 
 TEST_F(TextLineWrapperTest, HandlesTrailingNewlines)
@@ -368,14 +384,10 @@ TEST_F(TextLineWrapperTest, HandlesContinuationCharacter)
 TEST_F(TextLineWrapperTest, WrapsCorrectlyWithExtraWhitespace)
 {
     gmx::TextLineWrapper wrapper;
-
     wrapper.settings().setLineLength(14);
-    wrapper.settings().setStripLeadingWhitespace(true);
-    checkText(wrapper.wrapToString(g_wrapTextWhitespace),
-              "WrappedAt14StripLeading");
-    wrapper.settings().setStripLeadingWhitespace(false);
+
     checkText(wrapper.wrapToString(g_wrapTextWhitespace),
-              "WrappedAt14PreserveLeading");
+              "WrappedAt14");
 }
 
 } // namespace
diff --git a/src/gromacs/utility/tests/textwriter.cpp b/src/gromacs/utility/tests/textwriter.cpp
new file mode 100644 (file)
index 0000000..d1cc1ed
--- /dev/null
@@ -0,0 +1,121 @@
+/*
+ * This file is part of the GROMACS molecular simulation package.
+ *
+ * Copyright (c) 2015, by the GROMACS development team, led by
+ * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
+ * and including many others, as listed in the AUTHORS file in the
+ * top-level source directory and at http://www.gromacs.org.
+ *
+ * GROMACS is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1
+ * of the License, or (at your option) any later version.
+ *
+ * GROMACS is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with GROMACS; if not, see
+ * http://www.gnu.org/licenses, or write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA.
+ *
+ * If you want to redistribute modifications to GROMACS, please
+ * consider that scientific software is very special. Version
+ * control is crucial - bugs must be traceable. We will be happy to
+ * consider code for inclusion in the official distribution, but
+ * derived work must not be called official GROMACS. Details are found
+ * in the README & COPYING files - if they are missing, get the
+ * official version at http://www.gromacs.org.
+ *
+ * To help us fund GROMACS development, we humbly ask that you cite
+ * the research papers on the package. Check out http://www.gromacs.org.
+ */
+/*! \internal \file
+ * \brief
+ * Tests for gmx::TextWriter.
+ *
+ * \author Teemu Murtola <teemu.murtola@gmail.com>
+ * \ingroup module_utility
+ */
+#include "gmxpre.h"
+
+#include "gromacs/utility/textwriter.h"
+
+#include <string>
+
+#include <gtest/gtest.h>
+
+#include "gromacs/utility/stringstream.h"
+#include "gromacs/utility/stringutil.h"
+
+#include "testutils/stringtest.h"
+
+namespace
+{
+
+class TextWriterTest : public gmx::test::StringTestBase
+{
+    public:
+        TextWriterTest() : writer_(&stream_)
+        {
+        }
+
+        void checkOutput()
+        {
+            checkText(stream_.toString(), "Output");
+        }
+
+        gmx::StringOutputStream  stream_;
+        gmx::TextWriter          writer_;
+};
+
+TEST_F(TextWriterTest, WritesLines)
+{
+    writer_.writeLine("Explicit newline\n");
+    writer_.writeLine("Implicit newline");
+    writer_.writeLine(std::string("Explicit newline\n"));
+    writer_.writeLine(std::string("Implicit newline"));
+    writer_.writeLine();
+    checkOutput();
+}
+
+TEST_F(TextWriterTest, WritesLinesInParts)
+{
+    writer_.writeString("Partial ");
+    writer_.writeString("spaced");
+    writer_.writeString(" line");
+    writer_.writeLine();
+    writer_.writeString(std::string("Partial "));
+    writer_.writeString(std::string("spaced"));
+    writer_.writeString(std::string(" line"));
+    writer_.writeLine();
+    checkOutput();
+}
+
+TEST_F(TextWriterTest, WritesWrappedLines)
+{
+    writer_.wrapperSettings().setIndent(2);
+    writer_.wrapperSettings().setLineLength(15);
+    writer_.writeLine("Wrapped and indented text");
+    writer_.writeLine(std::string("Wrapped and indented text"));
+    writer_.writeLine();
+    checkOutput();
+}
+
+TEST_F(TextWriterTest, WritesLinesInPartsWithWrapper)
+{
+    writer_.wrapperSettings().setLineLength(50);
+    writer_.writeString("Partial ");
+    writer_.writeString("spaced");
+    writer_.writeString(" line");
+    writer_.writeLine();
+    writer_.writeString(std::string("Partial "));
+    writer_.writeString(std::string("spaced"));
+    writer_.writeString(std::string(" line"));
+    writer_.writeLine();
+    checkOutput();
+}
+
+} // namespace
index 043261d529c58a3f92d1ba8781fc1a03167f1400..4b1e58c7ed5de65cbb969b36a8f140df4d7f6a51 100644 (file)
@@ -47,6 +47,7 @@
 
 #include "gromacs/utility/filestream.h"
 #include "gromacs/utility/nodelete.h"
+#include "gromacs/utility/stringutil.h"
 #include "gromacs/utility/textstream.h"
 
 namespace gmx
@@ -58,9 +59,16 @@ class TextWriter::Impl
         explicit Impl(const TextOutputStreamPointer &stream)
             : stream_(stream)
         {
+            wrapper_.settings().setKeepFinalSpaces(true);
+        }
+
+        void writeWrappedString(const std::string &str)
+        {
+            stream_->write(wrapper_.wrapToString(str).c_str());
         }
 
         TextOutputStreamPointer stream_;
+        TextLineWrapper         wrapper_;
 };
 
 // static
@@ -101,21 +109,32 @@ TextOutputStream &TextWriter::stream()
     return *impl_->stream_;
 }
 
+TextLineWrapperSettings &TextWriter::wrapperSettings()
+{
+    return impl_->wrapper_.settings();
+}
+
 void TextWriter::writeString(const char *str)
 {
-    impl_->stream_->write(str);
+    if (impl_->wrapper_.isTrivial())
+    {
+        impl_->stream_->write(str);
+    }
+    else
+    {
+        impl_->writeWrappedString(str);
+    }
 }
 
 void TextWriter::writeString(const std::string &str)
 {
-    impl_->stream_->write(str.c_str());
+    impl_->writeWrappedString(str);
 }
 
 void TextWriter::writeLine(const char *line)
 {
-    const size_t length = std::strlen(line);
     writeString(line);
-    if (length == 0 || line[length-1] != '\n')
+    if (!endsWith(line, "\n"))
     {
         writeLine();
     }
@@ -123,7 +142,11 @@ void TextWriter::writeLine(const char *line)
 
 void TextWriter::writeLine(const std::string &line)
 {
-    writeLine(line.c_str());
+    writeString(line);
+    if (!endsWith(line, "\n"))
+    {
+        writeLine();
+    }
 }
 
 void TextWriter::writeLine()
index a46446ed617eb9dfffc219c4957f63b6614b586d..5f700830f0c0fa46deeba1f61417f02f82949522 100644 (file)
@@ -53,6 +53,8 @@
 namespace gmx
 {
 
+class TextLineWrapperSettings;
+
 /*! \libinternal \brief
  * Writes text into a TextOutputStream.
  *
@@ -136,6 +138,15 @@ class TextWriter
         //! Returns the underlying stream for this writer.
         TextOutputStream &stream();
 
+        /*! \brief
+         * Allows adjusting wrapping settings for the writer.
+         *
+         * \todo
+         * Wrapping is not currently implemented for code that writes partial
+         * lines with writeString().
+         */
+        TextLineWrapperSettings &wrapperSettings();
+
         /*! \brief
          * Writes a string to the stream.
          *