Fine-tuning for FileNameOption.

- Change behavior of required options with default values such that the
  default value is now used without an error if the option is not
  provided.  This is how it used to work with file name options.  For
  most other cases, this combination doesn't make much sense, but
  changed it such that it works consistently for all cases.
- Add FileNameOption::defaultBasename() as a more intuitive interface
  for providing a default for file name options.  The name makes it
  clear that no extension is expected, and the option also transparently
  treats required/optional arguments without the user needing to
  understand the difference between defaultValue() and
  defaultValueIfSet().

Closes #642; there is still some things to be improved, but should now
provide about the same features and about the same input options as
the old t_filenm-based system.

Change-Id: Ibcaf0667180e0b24b08604869d855baf23476681

diff --git a/share/template/template.cpp b/share/template/template.cpp
index ef2321c320cd5a455dbeda2553041eacfdbc3028..48e32c2d1942fca5c1e4d52904f7bf3a4ff5501c 100644 (file)
--- a/share/template/template.cpp
+++ b/share/template/template.cpp
@@ -136,7 +136,7 @@ AnalysisTemplate::initOptions(Options *options,
 
     options->addOption(FileNameOption("o")
         .filetype(eftPlot).outputFile()
-        .store(&fnDist_).defaultValueIfSet("avedist")
+        .store(&fnDist_).defaultBasename("avedist")
         .description("Average distances from reference group"));
 
     options->addOption(SelectionOption("reference")

diff --git a/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp b/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp
index 6fa71651f4c01a8aeed752e52699e0e2cc3775ea..d6d0807e923466d32b4fedee10c142f3252b3f2a 100644 (file)
--- a/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp
+++ b/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp
@@ -121,16 +121,16 @@ TEST_F(CommandLineHelpWriterTest, HandlesOptionTypes)
     options.addOption(FileNameOption("f")
                         .description("Input file description")
                         .filetype(eftTrajectory).inputFile().required()
-                        .defaultValue("traj"));
+                        .defaultBasename("traj"));
     options.addOption(FileNameOption("lib")
                         .description("Library file description")
                         .filetype(eftGenericData).inputFile().libraryFile()
-                        .defaultValueIfSet("libdata"));
+                        .defaultBasename("libdata"));
     options.addOption(FileNameOption("io")
                         .store(&filename)
                         .description("Input/Output file description")
                         .filetype(eftGenericData).inputOutputFile()
-                        .defaultValueIfSet("inout"));
+                        .defaultBasename("inout"));
     options.addOption(FileNameOption("o")
                         .description("Output file description")
                         .filetype(eftPlot).outputFile());
@@ -157,23 +157,23 @@ TEST_F(CommandLineHelpWriterTest, HandlesLongFileOptions)
     options.addOption(FileNameOption("f")
                         .description("File name option with a long value")
                         .filetype(eftTrajectory).inputFile().required()
-                        .defaultValue("path/to/long/trajectory/name"));
+                        .defaultBasename("path/to/long/trajectory/name"));
     options.addOption(FileNameOption("f2")
                         .description("File name option with a long value")
                         .filetype(eftTrajectory).inputFile().required()
-                        .defaultValue("path/to/long/trajectory"));
+                        .defaultBasename("path/to/long/trajectory"));
     options.addOption(FileNameOption("lib")
                         .description("File name option with a long value and type")
                         .filetype(eftTrajectory).inputFile().libraryFile()
-                        .defaultValue("path/to/long/trajectory/name"));
+                        .defaultBasename("path/to/long/trajectory/name"));
     options.addOption(FileNameOption("longfileopt")
                         .description("File name option with a long name")
                         .filetype(eftGenericData).inputFile()
-                        .defaultValue("deffile"));
+                        .defaultBasename("deffile"));
     options.addOption(FileNameOption("longfileopt2")
                         .description("File name option with multiple long fields")
                         .filetype(eftGenericData).inputFile().libraryFile()
-                        .defaultValue("path/to/long/file/name"));
+                        .defaultBasename("path/to/long/file/name"));
 
     gmx::CommandLineHelpWriter writer(options);
     checkHelp(&writer);

diff --git a/src/gromacs/options/abstractoption.cpp b/src/gromacs/options/abstractoption.cpp
index f4d5dfc108622c45ab5f43349d91689688fa0114..f935bc685a51b28c0ae8766dc44614e325623a42 100644 (file)
--- a/src/gromacs/options/abstractoption.cpp
+++ b/src/gromacs/options/abstractoption.cpp
@@ -141,7 +141,7 @@ void AbstractOptionStorage::finish()
 {
     GMX_RELEASE_ASSERT(!bInSet_, "finishSet() not called");
     processAll();
-    if (isRequired() && !isSet())
+    if (isRequired() && !(isSet() || hasFlag(efOption_ExplicitDefaultValue)))
     {
         GMX_THROW(InvalidInputError("Option is required, but not set"));
     }

diff --git a/src/gromacs/options/abstractoption.h b/src/gromacs/options/abstractoption.h
index ba2fccb2cfac3c76bbd6a5a3de6753df5027a716..5282a147b61959b676c0d54e6979f0f6c5ba296d 100644 (file)
--- a/src/gromacs/options/abstractoption.h
+++ b/src/gromacs/options/abstractoption.h
@@ -243,7 +243,13 @@ class OptionTemplate : public AbstractOption
         //! Hides the option from normal help output.
         MyClass &hidden(bool bHidden = true)
         { setFlag(efOption_Hidden, bHidden); return me(); }
-        //! Requires the option to be specified explicitly.
+        /*! \brief
+         * Requires the option to be specified explicitly.
+         *
+         * Note that if you specify defaultValue() together with required(),
+         * the user is not required to explicitly provide the option.
+         * In this case, required() only affects possible help output.
+         */
         MyClass &required(bool bRequired = true)
         { setFlag(efOption_Required, bRequired); return me(); }
         //! Allows the option to be specified multiple times.
@@ -264,8 +270,6 @@ class OptionTemplate : public AbstractOption
          * is no default value, the storage is not altered, which can also be
          * used to provide a default value.  The latter method has to be used
          * if the option can take multiple values.
-         * If required() is specified, only affects the default value shown in
-         * help output.
          *
          * \p defaultValue is copied when the option is created.
          */

diff --git a/src/gromacs/options/filenameoption.cpp b/src/gromacs/options/filenameoption.cpp
index 099c7968b9628bbeabc076d1a8b5e60499eef9bc..cee1e04b6344e19401a0287ea8f69d18036f0747 100644 (file)
--- a/src/gromacs/options/filenameoption.cpp
+++ b/src/gromacs/options/filenameoption.cpp
@@ -253,15 +253,18 @@ FileNameOptionStorage::FileNameOptionStorage(const FileNameOption &settings)
       bRead_(settings.bRead_), bWrite_(settings.bWrite_),
       bLibrary_(settings.bLibrary_)
 {
-    if (settings.defaultValue())
+    if (settings.defaultBasename_ != NULL)
     {
-        setDefaultValue(completeFileName(*settings.defaultValue(),
-                                         filetype_, false));
-    }
-    if (settings.defaultValueIfSet())
-    {
-        setDefaultValueIfSet(completeFileName(*settings.defaultValueIfSet(),
-                                              filetype_, false));
+        if (isRequired())
+        {
+            setDefaultValue(completeFileName(settings.defaultBasename_,
+                                             filetype_, false));
+        }
+        else
+        {
+            setDefaultValueIfSet(completeFileName(settings.defaultBasename_,
+                                                  filetype_, false));
+        }
     }
 }
 

diff --git a/src/gromacs/options/filenameoption.h b/src/gromacs/options/filenameoption.h
index 280d7a87ad59894395f608bc502bb21187597cb1..882b2e511bcac4cf9b4d090349c6322e77c22176 100644 (file)
--- a/src/gromacs/options/filenameoption.h
+++ b/src/gromacs/options/filenameoption.h
@@ -55,8 +55,6 @@ class FileNameOptionStorage;
  *
  * Public methods in this class do not throw.
  *
- * This class is currently a stub implementation.
- *
  * \inpublicapi
  * \ingroup module_options
  */
@@ -68,7 +66,7 @@ class FileNameOption : public OptionTemplate<std::string, FileNameOption>
 
         //! Initializes an option with the given name.
         explicit FileNameOption(const char *name)
-            : MyBase(name), filetype_(eftUnknown),
+            : MyBase(name), filetype_(eftUnknown), defaultBasename_(NULL),
               bRead_(false), bWrite_(false), bLibrary_(false)
         {
         }
@@ -95,14 +93,37 @@ class FileNameOption : public OptionTemplate<std::string, FileNameOption>
         /*! \brief
          * Tells that the file will be looked up in library directories in
          * addition to working directory.
+         *
+         * \todo
+         * Currently, this flag only affects the help output.  Callers must
+         * take care themselves to actually search the file in the library
+         * directories.  It would be nicer to do this searching within the
+         * file name option implementation.
          */
         MyClass &libraryFile() { bLibrary_ = true; return me(); }
+        /*! \brief
+         * Sets a default basename for the file option.
+         *
+         * Use this method instead of defaultValue() or defaultValueIfSet() to
+         * set a default value for a file name option.  No extension needs to
+         * be provided; it is automatically added based on filetype().
+         * The behavior is also adjusted based on required(): if the option is
+         * required, the value given to defaultBasename() is treated as for
+         * defaultValue(), otherwise it is treated as for defaultValueIfSet().
+         */
+        MyClass &defaultBasename(const char *basename)
+        { defaultBasename_ = basename; return me(); }
 
     private:
+        // Use defaultBasename() instead.
+        using MyBase::defaultValue;
+        using MyBase::defaultValueIfSet;
+
         //! Creates a FileNameOptionStorage object.
         virtual AbstractOptionStoragePointer createStorage() const;
 
         OptionFileType          filetype_;
+        const char             *defaultBasename_;
         bool                    bRead_;
         bool                    bWrite_;
         bool                    bLibrary_;

diff --git a/src/gromacs/options/optionflags.h b/src/gromacs/options/optionflags.h
index 77fe03ee06b54cff6976891020125be73e9a6fd2..659276d68241d58c41428f14157504cbbccf7df5 100644 (file)
--- a/src/gromacs/options/optionflags.h
+++ b/src/gromacs/options/optionflags.h
@@ -53,6 +53,10 @@ namespace gmx
  *
  * These flags are not part of the public interface, even though they are in an
  * installed header.  They are needed in a few template class implementations.
+ *
+ * \todo
+ * The flags related to default values are confusing, consider reorganizing
+ * them.
  */
 enum OptionFlag
 {
@@ -60,13 +64,15 @@ enum OptionFlag
     efOption_Set                        = 1<<0,
     //! The current value of the option is a programmatic default value.
     efOption_HasDefaultValue            = 1<<1,
+    //! An explicit default value has been provided for the option.
+    efOption_ExplicitDefaultValue      = 1<<2,
     /*! \brief
      * Next assignment to the option clears old values.
      *
      * This flag is set when a new option source starts, such that values
      * from the new source will overwrite old ones.
      */
-    efOption_ClearOnNextSet             = 1<<2,
+    efOption_ClearOnNextSet             = 1<<3,
     //! %Option is required to be set.
     efOption_Required                   = 1<<4,
     //! %Option can be specified multiple times.

diff --git a/src/gromacs/options/optionstoragetemplate.h b/src/gromacs/options/optionstoragetemplate.h
index 3aeceac07e3fecc25e36359350ea6120c0e473f1..e6f79c24a1e88dab019cb7761eb41b486c1d3fd1 100644 (file)
--- a/src/gromacs/options/optionstoragetemplate.h
+++ b/src/gromacs/options/optionstoragetemplate.h
@@ -447,6 +447,7 @@ void OptionStorageTemplate<T>::setDefaultValue(const T &value)
     }
     if (hasFlag(efOption_HasDefaultValue))
     {
+        setFlag(efOption_ExplicitDefaultValue);
         clear();
         clearSet();
         addValue(value);

diff --git a/src/gromacs/options/tests/filenameoption.cpp b/src/gromacs/options/tests/filenameoption.cpp
index 84f781ed630803bfb82b36f5db834e85d1b112d7..b95763b0a438b4d36973193ce0c74ed49c2bbb9a 100644 (file)
--- a/src/gromacs/options/tests/filenameoption.cpp
+++ b/src/gromacs/options/tests/filenameoption.cpp
@@ -72,14 +72,14 @@ TEST(FileNameOptionTest, AddsMissingExtension)
     EXPECT_EQ("testfile.xtc", value);
 }
 
-TEST(FileNameOptionTest, HandlesDefaultValueWithoutExtension)
+TEST(FileNameOptionTest, HandlesRequiredDefaultValueWithoutExtension)
 {
     gmx::Options           options(NULL, NULL);
     std::string            value;
     ASSERT_NO_THROW(options.addOption(
-                        FileNameOption("f").store(&value)
+                        FileNameOption("f").store(&value).required()
                             .filetype(gmx::eftGenericData).outputFile()
-                            .defaultValue("testfile")));
+                            .defaultBasename("testfile")));
     EXPECT_EQ("testfile.dat", value);
 
     gmx::OptionsAssigner assigner(&options);
@@ -90,14 +90,14 @@ TEST(FileNameOptionTest, HandlesDefaultValueWithoutExtension)
     EXPECT_EQ("testfile.dat", value);
 }
 
-TEST(FileNameOptionTest, HandlesDefaultValueIfSetWithoutExtension)
+TEST(FileNameOptionTest, HandlesOptionalDefaultValueWithoutExtension)
 {
     gmx::Options           options(NULL, NULL);
     std::string            value;
     ASSERT_NO_THROW(options.addOption(
                         FileNameOption("f").store(&value)
                             .filetype(gmx::eftIndex).outputFile()
-                            .defaultValueIfSet("testfile")));
+                            .defaultBasename("testfile")));
     EXPECT_TRUE(value.empty());
 
     gmx::OptionsAssigner assigner(&options);

diff --git a/src/gromacs/options/tests/optionsassigner.cpp b/src/gromacs/options/tests/optionsassigner.cpp
index 60f927481a8bf23cfb0e00f8f72cb26c565d67af..e843968963d7922a64a4c5a3b0829187398af746 100644 (file)
--- a/src/gromacs/options/tests/optionsassigner.cpp
+++ b/src/gromacs/options/tests/optionsassigner.cpp
@@ -70,6 +70,19 @@ TEST(OptionsAssignerTest, HandlesMissingRequiredParameter)
     EXPECT_THROW(options.finish(), gmx::InvalidInputError);
 }
 
+TEST(OptionsAssignerTest, HandlesRequiredParameterWithDefaultValue)
+{
+    gmx::Options options(NULL, NULL);
+    int value = 0;
+    using gmx::IntegerOption;
+    ASSERT_NO_THROW(options.addOption(
+                        IntegerOption("p").store(&value).required()
+                            .defaultValue(1)));
+
+    EXPECT_NO_THROW(options.finish());
+    EXPECT_EQ(1, value);
+}
+
 TEST(OptionsAssignerTest, HandlesInvalidMultipleParameter)
 {
     gmx::Options options(NULL, NULL);

diff --git a/src/gromacs/trajectoryanalysis/modules/angle.cpp b/src/gromacs/trajectoryanalysis/modules/angle.cpp
index 68bb4b4c1d6ea6863b1037094413fde2a9752015..dd8c1f9b955c9e9caadcf7f3799d14024550aeec 100644 (file)
--- a/src/gromacs/trajectoryanalysis/modules/angle.cpp
+++ b/src/gromacs/trajectoryanalysis/modules/angle.cpp
@@ -131,10 +131,10 @@ Angle::initOptions(Options *options, TrajectoryAnalysisSettings * /*settings*/)
     options->setDescription(concatenateStrings(desc));
 
     options->addOption(FileNameOption("o").filetype(eftPlot).outputFile()
-                           .store(&fnAngle_).defaultValueIfSet("angle")
+                           .store(&fnAngle_).defaultBasename("angle")
                            .description("Computed angles"));
     options->addOption(FileNameOption("od").filetype(eftPlot).outputFile()
-                           .store(&fnDump_).defaultValueIfSet("angdump")
+                           .store(&fnDump_).defaultBasename("angdump")
                            .description("Individual angles on separate lines"));
 
     options->addOption(StringOption("g1").enumValue(cGroup1TypeEnum)

diff --git a/src/gromacs/trajectoryanalysis/modules/distance.cpp b/src/gromacs/trajectoryanalysis/modules/distance.cpp
index 423e831494938f5c21009e673831f6f1670d1e05..7f662f60b86b2ede77e7025be060acb8788e9cbe 100644 (file)
--- a/src/gromacs/trajectoryanalysis/modules/distance.cpp
+++ b/src/gromacs/trajectoryanalysis/modules/distance.cpp
@@ -91,7 +91,7 @@ Distance::initOptions(Options *options, TrajectoryAnalysisSettings * /*settings*
     options->setDescription(concatenateStrings(desc));
 
     options->addOption(FileNameOption("o").filetype(eftPlot).outputFile()
-                           .store(&fnDist_).defaultValueIfSet("dist")
+                           .store(&fnDist_).defaultBasename("dist")
                            .description("Computed distances"));
     options->addOption(SelectionOption("select").required().valueCount(2)
                            .store(sel_));

diff --git a/src/gromacs/trajectoryanalysis/modules/select.cpp b/src/gromacs/trajectoryanalysis/modules/select.cpp
index f54be0efb10d28e9fc5fa423bad41e1ed4cb6960..76c8d300803d034f6ac402591f975c1aac9b7a36 100644 (file)
--- a/src/gromacs/trajectoryanalysis/modules/select.cpp
+++ b/src/gromacs/trajectoryanalysis/modules/select.cpp
@@ -316,19 +316,19 @@ Select::initOptions(Options *options, TrajectoryAnalysisSettings * /*settings*/)
     options->setDescription(concatenateStrings(desc));
 
     options->addOption(FileNameOption("os").filetype(eftPlot).outputFile()
-                           .store(&fnSize_).defaultValueIfSet("size")
+                           .store(&fnSize_).defaultBasename("size")
                            .description("Number of positions in each selection"));
     options->addOption(FileNameOption("oc").filetype(eftPlot).outputFile()
-                           .store(&fnFrac_).defaultValueIfSet("frac")
+                           .store(&fnFrac_).defaultBasename("frac")
                            .description("Covered fraction for each selection"));
     options->addOption(FileNameOption("oi").filetype(eftGenericData).outputFile()
-                           .store(&fnIndex_).defaultValueIfSet("index")
+                           .store(&fnIndex_).defaultBasename("index")
                            .description("Indices selected by each selection"));
     options->addOption(FileNameOption("on").filetype(eftIndex).outputFile()
-                           .store(&fnNdx_).defaultValueIfSet("index")
+                           .store(&fnNdx_).defaultBasename("index")
                            .description("Index file from the selection"));
     options->addOption(FileNameOption("om").filetype(eftPlot).outputFile()
-                           .store(&fnMask_).defaultValueIfSet("mask")
+                           .store(&fnMask_).defaultBasename("mask")
                            .description("Mask for selected positions"));
 
     options->addOption(SelectionOption("select").storeVector(&sel_)

diff --git a/src/gromacs/trajectoryanalysis/runnercommon.cpp b/src/gromacs/trajectoryanalysis/runnercommon.cpp
index 838522f21909b73bb93f463960dc80766ab3de31..e33b4adca07394ce5f8fa9041b706f7d9e3847b3 100644 (file)
--- a/src/gromacs/trajectoryanalysis/runnercommon.cpp
+++ b/src/gromacs/trajectoryanalysis/runnercommon.cpp
@@ -182,17 +182,17 @@ TrajectoryAnalysisRunnerCommon::initOptions(Options *options)
     options->addOption(FileNameOption("f")
                            .filetype(eftTrajectory).inputFile()
                            .store(&impl_->trjfile_)
-                           .defaultValueIfSet("traj")
+                           .defaultBasename("traj")
                            .description("Input trajectory or single configuration"));
     options->addOption(FileNameOption("s")
                            .filetype(eftTopology).inputFile()
                            .store(&impl_->topfile_)
-                           .defaultValueIfSet("topol")
+                           .defaultBasename("topol")
                            .description("Input structure"));
     options->addOption(FileNameOption("n")
                            .filetype(eftIndex).inputFile()
                            .store(&impl_->ndxfile_)
-                           .defaultValueIfSet("index")
+                           .defaultBasename("index")
                            .description("Extra index groups"));
     options->addOption(SelectionFileOption("sf"));