Simpler required selection input from a file.

Now a selection file option (-sf) assigns the contents of the file to
required selection option(s) that haven't yet been assigned if it has no
other context.  Main benefit is that a plain '-sf file.dat' (without any
other selection options specified) assigns the contents of file.dat to
required selections, similarly to how it worked in 4.5.

Resolves the main open issue for #656, error message and help
improvements remain.

Change-Id: I6311b548ab21ecf0f489f2743e2750411f86b3ff

diff --git a/src/gromacs/selection/selectionoption.cpp b/src/gromacs/selection/selectionoption.cpp
index 92f0958a151084ddb0a558765f5b9b84ab7963d4..dd6dd460687472e1e37505fd0411a93195ec6fff 100644 (file)
--- a/src/gromacs/selection/selectionoption.cpp
+++ b/src/gromacs/selection/selectionoption.cpp
@@ -74,6 +74,18 @@ SelectionOptionStorage::SelectionOptionStorage(const SelectionOption &settings)
 }
 
 
+void SelectionOptionStorage::setManager(SelectionOptionManager *manager)
+{
+    GMX_RELEASE_ASSERT(manager_ == NULL || manager_ == manager,
+                       "Manager cannot be changed once set");
+    if (manager_ == NULL)
+    {
+        manager->registerOption(this);
+        manager_ = manager;
+    }
+}
+
+
 std::string SelectionOptionStorage::formatSingleValue(const Selection &value) const
 {
     return value.selectionText();
@@ -106,6 +118,7 @@ void SelectionOptionStorage::addSelections(
     if (bFullValue)
     {
         commitValues();
+        setFlag(efSet);
     }
 }
 

diff --git a/src/gromacs/selection/selectionoptionmanager.cpp b/src/gromacs/selection/selectionoptionmanager.cpp
index d18b721f916eb7c9e523837e0a5dd1e77561ceb7..456d305c43c425fcacc819eaef51ea20f1e4c457 100644 (file)
--- a/src/gromacs/selection/selectionoptionmanager.cpp
+++ b/src/gromacs/selection/selectionoptionmanager.cpp
@@ -98,6 +98,8 @@ class SelectionOptionManager::Impl
 
         //! Collection for a list of selection requests.
         typedef std::vector<SelectionRequest> RequestList;
+        //! Collection for list of option storage objects.
+        typedef std::vector<SelectionOptionStorage *> OptionList;
 
         /*! \brief
          * Helper class that clears a request list on scope exit.
@@ -142,9 +144,17 @@ class SelectionOptionManager::Impl
          * from the list.
          */
         void placeSelectionsInRequests(const SelectionList &selections);
+        /*! \brief
+         * Adds a request for each required option that is not yet set.
+         *
+         * \throws    std::bad_alloc if out of memory.
+         */
+        void requestUnsetRequiredOptions();
 
         //! Selection collection to which selections are stored.
         SelectionCollection    &collection_;
+        //! List of selection options (storage objects) this manager manages.
+        OptionList              options_;
         //! List of selections requested for later parsing.
         RequestList             requests_;
 };
@@ -157,6 +167,11 @@ SelectionOptionManager::Impl::Impl(SelectionCollection *collection)
 void SelectionOptionManager::Impl::placeSelectionsInRequests(
         const SelectionList &selections)
 {
+    if (requests_.empty())
+    {
+        requestUnsetRequiredOptions();
+    }
+
     RequestsClearer clearRequestsOnExit(&requests_);
 
     SelectionList::const_iterator first = selections.begin();
@@ -195,6 +210,19 @@ void SelectionOptionManager::Impl::placeSelectionsInRequests(
     }
 }
 
+void SelectionOptionManager::Impl::requestUnsetRequiredOptions()
+{
+    OptionList::const_iterator i;
+    for (i = options_.begin(); i != options_.end(); ++i)
+    {
+        SelectionOptionStorage &storage = **i;
+        if (storage.isRequired() && !storage.isSet())
+        {
+            requests_.push_back(SelectionRequest(&storage));
+        }
+    }
+}
+
 
 /********************************************************************
  * SelectionOptionManager
@@ -215,6 +243,13 @@ SelectionOptionManager::selectionCollection()
     return impl_->collection_;
 }
 
+void
+SelectionOptionManager::registerOption(SelectionOptionStorage *storage)
+{
+    impl_->requests_.reserve(impl_->options_.size() + 1);
+    impl_->options_.push_back(storage);
+}
+
 void
 SelectionOptionManager::requestDelayedParsing(SelectionOptionStorage *storage)
 {

diff --git a/src/gromacs/selection/selectionoptionmanager.h b/src/gromacs/selection/selectionoptionmanager.h
index 7b025052f028f9bccfeb106774fb09e9cfbd3081..25d1f8e5e0cdf4ef5322d8438f300d648130c5b8 100644 (file)
--- a/src/gromacs/selection/selectionoptionmanager.h
+++ b/src/gromacs/selection/selectionoptionmanager.h
@@ -87,6 +87,19 @@ class SelectionOptionManager
          * Does not throw.
          */
         SelectionCollection &selectionCollection();
+        /*! \brief
+         * Adds a selection option to be managed.
+         *
+         * \param     storage  Storage object for the option to register.
+         * \throws    std::bad_alloc if out of memory.
+         *
+         * This is only for internal use by the selection module.
+         * It is not possible to obtain a SelectionOptionStorage pointer
+         * through any public or library API.
+         *
+         * Strong exception safety.
+         */
+        void registerOption(SelectionOptionStorage *storage);
         /*! \brief
          * Adds a selection option for delayed user input.
          *
@@ -136,9 +149,13 @@ class SelectionOptionManager
          *        not the last (in which case it is not possible to determine
          *        which selections belong to which request).
          *
-         * This method behaves as parseRequestedFromStdin(), but reads the
-         * selections from a file instead of standard input.
-         * This is used to implement SelectionFileOption.
+         * This method behaves as parseRequestedFromStdin(), with two
+         * exceptions:
+         *  -# It reads the selections from a file instead of standard input.
+         *  -# If no requests are pending, assigns values to all required
+         *     options that have not yet been set.
+         *
+         * This method used to implement SelectionFileOption.
          *
          * \see parseRequestedFromStdin()
          */

diff --git a/src/gromacs/selection/selectionoptionstorage.h b/src/gromacs/selection/selectionoptionstorage.h
index 9de8e3b0212b091f052a347ee8d1824733d62c33..ee223dff21f5514d5244b06413e5a57d4ac9e185 100644 (file)
--- a/src/gromacs/selection/selectionoptionstorage.h
+++ b/src/gromacs/selection/selectionoptionstorage.h
@@ -69,10 +69,7 @@ class SelectionOptionStorage : public OptionStorageTemplate<Selection>
         virtual std::string formatSingleValue(const Selection &value) const;
 
         //! \copydoc SelectionOptionInfo::setManager()
-        void setManager(SelectionOptionManager *manager)
-        {
-            manager_ = manager;
-        }
+        void setManager(SelectionOptionManager *manager);
 
         /*! \brief
          * Adds selections to the storage.

diff --git a/src/gromacs/selection/tests/selectionoption.cpp b/src/gromacs/selection/tests/selectionoption.cpp
index 710c74ca88b82667426462857c2feb540f7619cd..4d33f9bf8c0680db1fc26410e090c21ad55985df 100644 (file)
--- a/src/gromacs/selection/tests/selectionoption.cpp
+++ b/src/gromacs/selection/tests/selectionoption.cpp
@@ -339,9 +339,13 @@ SelectionFileOptionTest::SelectionFileOptionTest()
 TEST_F(SelectionFileOptionTest, HandlesSingleSelectionOptionFromFile)
 {
     gmx::SelectionList sel;
+    gmx::SelectionList reqsel;
     using gmx::SelectionOption;
     ASSERT_NO_THROW(_options.addOption(
                         SelectionOption("sel").storeVector(&sel).multiValue()));
+    ASSERT_NO_THROW(_options.addOption(
+                        SelectionOption("reqsel").storeVector(&reqsel)
+                            .multiValue().required()));
     setManager();
 
     gmx::OptionsAssigner assigner(&_options);
@@ -358,6 +362,7 @@ TEST_F(SelectionFileOptionTest, HandlesSingleSelectionOptionFromFile)
     ASSERT_EQ(2U, sel.size());
     EXPECT_STREQ("resname RA RB", sel[0].selectionText());
     EXPECT_STREQ("resname RB RC", sel[1].selectionText());
+    ASSERT_EQ(0U, reqsel.size());
 }
 
 
@@ -430,6 +435,101 @@ TEST_F(SelectionFileOptionTest, HandlesTwoSelectionOptionsFromSingleFile)
 }
 
 
+TEST_F(SelectionFileOptionTest, HandlesRequiredOptionFromFile)
+{
+    gmx::SelectionList sel;
+    gmx::SelectionList optsel;
+    using gmx::SelectionOption;
+    ASSERT_NO_THROW(_options.addOption(
+                        SelectionOption("sel").storeVector(&sel)
+                            .multiValue().required()));
+    ASSERT_NO_THROW(_options.addOption(
+                        SelectionOption("optsel").storeVector(&optsel)
+                            .multiValue()));
+    setManager();
+
+    gmx::OptionsAssigner assigner(&_options);
+    EXPECT_NO_THROW(assigner.start());
+    ASSERT_NO_THROW(assigner.startOption("sf"));
+    EXPECT_NO_THROW(assigner.appendValue(gmx::test::getTestFilePath("selfile.dat")));
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.startOption("optsel"));
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(_options.finish());
+    EXPECT_NO_THROW(_manager.parseRequestedFromString("resname RC RD"));
+
+    // These should match the contents of selfile.dat
+    ASSERT_EQ(2U, sel.size());
+    EXPECT_STREQ("resname RA RB", sel[0].selectionText());
+    EXPECT_STREQ("resname RB RC", sel[1].selectionText());
+    ASSERT_EQ(1U, optsel.size());
+    EXPECT_STREQ("resname RC RD", optsel[0].selectionText());
+}
+
+
+// TODO: Is this the best possible behavior, or should it error out?
+TEST_F(SelectionFileOptionTest, HandlesRequiredOptionFromFileWithOtherOptionSet)
+{
+    gmx::SelectionList sel1;
+    gmx::SelectionList sel2;
+    using gmx::SelectionOption;
+    ASSERT_NO_THROW(_options.addOption(
+                        SelectionOption("sel1").storeVector(&sel1)
+                            .multiValue().required()));
+    ASSERT_NO_THROW(_options.addOption(
+                        SelectionOption("sel2").storeVector(&sel2)
+                            .multiValue().required()));
+    setManager();
+
+    gmx::OptionsAssigner assigner(&_options);
+    EXPECT_NO_THROW(assigner.start());
+    EXPECT_NO_THROW(assigner.startOption("sel1"));
+    EXPECT_NO_THROW(assigner.appendValue("resname RC RD"));
+    EXPECT_NO_THROW(assigner.finishOption());
+    ASSERT_NO_THROW(assigner.startOption("sf"));
+    EXPECT_NO_THROW(assigner.appendValue(gmx::test::getTestFilePath("selfile.dat")));
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(_options.finish());
+
+    // These should match the contents of selfile.dat
+    ASSERT_EQ(2U, sel2.size());
+    EXPECT_STREQ("resname RA RB", sel2[0].selectionText());
+    EXPECT_STREQ("resname RB RC", sel2[1].selectionText());
+    ASSERT_EQ(1U, sel1.size());
+    EXPECT_STREQ("resname RC RD", sel1[0].selectionText());
+}
+
+
+TEST_F(SelectionFileOptionTest, HandlesTwoRequiredOptionsFromSingleFile)
+{
+    gmx::SelectionList sel1;
+    gmx::SelectionList sel2;
+    using gmx::SelectionOption;
+    ASSERT_NO_THROW(_options.addOption(
+                        SelectionOption("sel1").storeVector(&sel1).required()));
+    ASSERT_NO_THROW(_options.addOption(
+                        SelectionOption("sel2").storeVector(&sel2).required()));
+    setManager();
+
+    gmx::OptionsAssigner assigner(&_options);
+    std::string value(gmx::test::getTestFilePath("selfile.dat"));
+    EXPECT_NO_THROW(assigner.start());
+    ASSERT_NO_THROW(assigner.startOption("sf"));
+    EXPECT_NO_THROW(assigner.appendValue(value));
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(_options.finish());
+
+    // These should match the contents of selfile.dat
+    ASSERT_EQ(1U, sel1.size());
+    EXPECT_STREQ("resname RA RB", sel1[0].selectionText());
+    ASSERT_EQ(1U, sel2.size());
+    EXPECT_STREQ("resname RB RC", sel2[0].selectionText());
+}
+
+
 TEST_F(SelectionFileOptionTest, GivesErrorWithNoFile)
 {
     gmx::SelectionList sel;