From: Teemu Murtola Date: Mon, 29 Jun 2015 10:02:11 +0000 (+0300) Subject: Fix error wrapping in interactive selection input X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?p=alexxy%2Fgromacs.git;a=commitdiff_plain;h=812ab9cc9fc5380bd3ffcae9a10dff8a964fda21 Fix error wrapping in interactive selection input 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 --- diff --git a/src/gromacs/selection/selectioncollection.cpp b/src/gromacs/selection/selectioncollection.cpp index d124cf2800..b7ee4d0b2d 100644 --- a/src/gromacs/selection/selectioncollection.cpp +++ b/src/gromacs/selection/selectioncollection.cpp @@ -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_); diff --git a/src/gromacs/utility/exceptions.cpp b/src/gromacs/utility/exceptions.cpp index 1248b96ef7..cb683dafb2 100644 --- a/src/gromacs/utility/exceptions.cpp +++ b/src/gromacs/utility/exceptions.cpp @@ -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)); } } diff --git a/src/gromacs/utility/stringutil.cpp b/src/gromacs/utility/stringutil.cpp index 86abf82a08..54b3221492 100644 --- a/src/gromacs/utility/stringutil.cpp +++ b/src/gromacs/utility/stringutil.cpp @@ -57,15 +57,16 @@ 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; diff --git a/src/gromacs/utility/stringutil.h b/src/gromacs/utility/stringutil.h index e7e11a6d9e..004daae3e0 100644 --- a/src/gromacs/utility/stringutil.h +++ b/src/gromacs/utility/stringutil.h @@ -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. * diff --git a/src/gromacs/utility/tests/CMakeLists.txt b/src/gromacs/utility/tests/CMakeLists.txt index a002c762c3..c980a24363 100644 --- a/src/gromacs/utility/tests/CMakeLists.txt +++ b/src/gromacs/utility/tests/CMakeLists.txt @@ -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 + ) diff --git a/src/gromacs/utility/tests/refdata/TextLineWrapperTest_WrapsCorrectlyWithExtraWhitespace.xml b/src/gromacs/utility/tests/refdata/TextLineWrapperTest_WrapsCorrectlyWithExtraWhitespace.xml index f277fc930b..1bb9fb4227 100644 --- a/src/gromacs/utility/tests/refdata/TextLineWrapperTest_WrapsCorrectlyWithExtraWhitespace.xml +++ b/src/gromacs/utility/tests/refdata/TextLineWrapperTest_WrapsCorrectlyWithExtraWhitespace.xml @@ -1,12 +1,7 @@ - - + + + + diff --git a/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInParts.xml b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInParts.xml new file mode 100644 index 0000000000..703467f903 --- /dev/null +++ b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInParts.xml @@ -0,0 +1,8 @@ + + + + + diff --git a/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInPartsWithWrapper.xml b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInPartsWithWrapper.xml new file mode 100644 index 0000000000..703467f903 --- /dev/null +++ b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesLinesInPartsWithWrapper.xml @@ -0,0 +1,8 @@ + + + + + diff --git a/src/gromacs/utility/tests/refdata/TextWriterTest_WritesWrappedLines.xml b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesWrappedLines.xml new file mode 100644 index 0000000000..a87ee7d14d --- /dev/null +++ b/src/gromacs/utility/tests/refdata/TextWriterTest_WritesWrappedLines.xml @@ -0,0 +1,11 @@ + + + + + diff --git a/src/gromacs/utility/tests/stringutil.cpp b/src/gromacs/utility/tests/stringutil.cpp index ba1683d06d..7145fdb109 100644 --- a/src/gromacs/utility/tests/stringutil.cpp +++ b/src/gromacs/utility/tests/stringutil.cpp @@ -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 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 index 0000000000..d1cc1ed939 --- /dev/null +++ b/src/gromacs/utility/tests/textwriter.cpp @@ -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 + * \ingroup module_utility + */ +#include "gmxpre.h" + +#include "gromacs/utility/textwriter.h" + +#include + +#include + +#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 diff --git a/src/gromacs/utility/textwriter.cpp b/src/gromacs/utility/textwriter.cpp index 043261d529..4b1e58c7ed 100644 --- a/src/gromacs/utility/textwriter.cpp +++ b/src/gromacs/utility/textwriter.cpp @@ -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() diff --git a/src/gromacs/utility/textwriter.h b/src/gromacs/utility/textwriter.h index a46446ed61..5f700830f0 100644 --- a/src/gromacs/utility/textwriter.h +++ b/src/gromacs/utility/textwriter.h @@ -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. *