[PATCH setup 04/14] Hoist pick() up to packagemeta

[Jon Turney <jon.turney@dronecode.org.uk>]

We are always writing packagemeta.desired.pick(bool, packagemeta).  This
kind of suggests something not quite right.

The pick flag means install/reinstall, so despite being stored per
packageversion, is only significant to download/install for the desired
version.

There's a slight wrinkle in that we want to also set/clear this flag for the
source packageversion.  We can't change this to point to packagemeta rather
than packageversion, as that may not be the same for all versions, so
instead just track this flag separately as srcpicked.

Note that there is still a complicated mapping between the state of desired
and pick and the action represented in the UI:

desired == empty, installed == desired : skip
desired == empty, installed != desired : uninstall
desired == installed, pick == true     : reinstall
desired == installed, pick == false    : keep
desired != installed, pick == true     : upgrade
desired != installed, pick == false    : invalid
---
 PickPackageLine.cc | 13 +++++------
 PickView.cc        |  8 +++----
 download.cc        | 12 +++++-----
 install.cc         | 19 ++++++++--------
 package_db.cc      |  2 +-
 package_meta.cc    | 67 ++++++++++++++++++++++++++++++++++++++----------------
 package_meta.h     | 11 ++++++++-
 package_version.cc | 16 -------------
 package_version.h  |  7 ------
 prereq.cc          | 11 +++++----
 10 files changed, 90 insertions(+), 76 deletions(-)

diff --git a/PickPackageLine.cc b/PickPackageLine.cc
index 60ece7f..95c1557 100644
--- a/PickPackageLine.cc
+++ b/PickPackageLine.cc
@@ -44,7 +44,7 @@ PickPackageLine::paint (HDC hdc, HRGN unused, int x, int y, int col_num, int sho
           /* current version */ pkg.desired == pkg.installed ||
           /* no source */ !pkg.desired.accessible())
         theView.DrawIcon (hdc, x + HMARGIN/2, by, theView.bm_checkna);
-      else if (pkg.desired.picked())
+      else if (pkg.picked())
         theView.DrawIcon (hdc, x + HMARGIN/2, by, theView.bm_checkyes);
       else
         theView.DrawIcon (hdc, x + HMARGIN/2, by, theView.bm_checkno);
@@ -67,7 +67,7 @@ PickPackageLine::paint (HDC hdc, HRGN unused, int x, int y, int col_num, int sho
           /* when no source mirror available */
           !pkg.desired.sourcePackage().accessible())
         theView.DrawIcon (hdc, x + HMARGIN/2, by, theView.bm_checkna);
-      else if (pkg.desired.sourcePackage().picked())
+      else if (pkg.srcpicked())
         theView.DrawIcon (hdc, x + HMARGIN/2, by, theView.bm_checkyes);
       else
         theView.DrawIcon (hdc, x + HMARGIN/2, by, theView.bm_checkno);
@@ -100,7 +100,7 @@ PickPackageLine::paint (HDC hdc, HRGN unused, int x, int y, int col_num, int sho
       /* Include the size of the binary package, and if selected, the source
          package as well.  */
       sz += picked.source()->size;
-      if (picked.sourcePackage().picked())
+      if (pkg.srcpicked())
         sz += picked.sourcePackage().source()->size;
 
       /* If size still 0, size must be unknown.  */
@@ -133,20 +133,19 @@ PickPackageLine::click (int const myrow, int const ClickedRow, int const x)
       && x <= theView.headers[theView.bintick_col + 1].x - HMARGIN / 2)
     {
       if (pkg.desired.accessible ())
-	pkg.desired.pick (!pkg.desired.picked (), &pkg);
+	pkg.pick (!pkg.picked ());
     }
   else if (x >= theView.headers[theView.srctick_col].x - HMARGIN / 2
 	   && x <= theView.headers[theView.srctick_col + 1].x - HMARGIN / 2)
     {
       if (pkg.desired.sourcePackage ().accessible ())
-	pkg.desired.sourcePackage ().pick (
-			!pkg.desired.sourcePackage ().picked (), NULL);
+	pkg.srcpick (!pkg.srcpicked ());
     }
   /* Unchecking binary while source is unchecked or vice versa is equivalent
      to uninstalling.  It's essential to set desired correctly, otherwise the
      package gets uninstalled without visual feedback to the user.  The package
      will not even show up in the "Pending" view! */
-  if (!pkg.desired.picked () && !pkg.desired.sourcePackage ().picked ())
+  if (!pkg.picked () && !pkg.srcpicked ())
     pkg.desired = packageversion ();
   return 0;
 }
diff --git a/PickView.cc b/PickView.cc
index 222bcb8..4c728f8 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -175,13 +175,13 @@ PickView::setViewMode (views mode)
               || (view_mode == PickView::views::PackagePending &&
                   ((!pkg.desired && pkg.installed) ||         // uninstall
                     (pkg.desired &&
-                      (pkg.desired.picked () ||               // install bin
-                       pkg.desired.sourcePackage ().picked ())))) // src
+                      (pkg.picked () ||               // install bin
+                       pkg.srcpicked ())))) // src
               
               // "Up to date" : installed packages that will not be changed
               || (view_mode == PickView::views::PackageKeeps &&
-                  (pkg.installed && pkg.desired && !pkg.desired.picked ()
-                    && !pkg.desired.sourcePackage ().picked ()))
+                  (pkg.installed && pkg.desired && !pkg.picked ()
+                    && !pkg.srcpicked ()))
 
               // "Not installed"
               || (view_mode == PickView::views::PackageSkips &&
diff --git a/download.cc b/download.cc
index 80615f3..a2237a7 100644
--- a/download.cc
+++ b/download.cc
@@ -208,18 +208,18 @@ do_download_thread (HINSTANCE h, HWND owner)
        i != db.packages.end (); ++i)
     {
       packagemeta & pkg = *(i->second);
-      if (pkg.desired.picked () || pkg.desired.sourcePackage ().picked ())
+      if (pkg.picked () || pkg.srcpicked ())
 	{
 	  packageversion version = pkg.desired;
 	  packageversion sourceversion = version.sourcePackage();
 	  try 
 	    {
-    	      if (version.picked())
+	      if (pkg.picked())
 		{
 		    if (!check_for_cached (*version.source()))
 		      total_download_bytes += version.source()->size;
 		}
-    	      if (sourceversion.picked () || IncludeSource)
+	      if (pkg.srcpicked () || IncludeSource)
 		{
 		    if (!check_for_cached (*sourceversion.source()))
 		      total_download_bytes += sourceversion.source()->size;
@@ -243,16 +243,16 @@ do_download_thread (HINSTANCE h, HWND owner)
        i != db.packages.end (); ++i)
     {
       packagemeta & pkg = *(i->second);
-      if (pkg.desired.picked () || pkg.desired.sourcePackage ().picked ())
+      if (pkg.picked () || pkg.srcpicked ())
 	{
 	  int e = 0;
 	  packageversion version = pkg.desired;
 	  packageversion sourceversion = version.sourcePackage();
-	  if (version.picked())
+	  if (pkg.picked())
 	    {
 		e += download_one (*version.source(), owner);
 	    }
-	  if (sourceversion && (sourceversion.picked() || IncludeSource))
+	  if (sourceversion && (pkg.srcpicked() || IncludeSource))
 	    {
 		e += download_one (*sourceversion.source (), owner);
 	    }
diff --git a/install.cc b/install.cc
index cd3128c..fee2e19 100644
--- a/install.cc
+++ b/install.cc
@@ -740,12 +740,12 @@ do_install_thread (HINSTANCE h, HWND owner)
   {
     packagemeta & pkg = *(i->second);
 
-    if (pkg.desired.picked())
+    if (pkg.picked())
     {
       md5sum_total_bytes += pkg.desired.source()->size;
     }
 
-    if (pkg.desired.sourcePackage ().picked() || IncludeSource)
+    if (pkg.srcpicked() || IncludeSource)
     {
       md5sum_total_bytes += pkg.desired.sourcePackage ().source()->size;
     }
@@ -759,7 +759,7 @@ do_install_thread (HINSTANCE h, HWND owner)
   {
     packagemeta & pkg = *(i->second);
 
-    if (pkg.desired.picked())
+    if (pkg.picked())
     {
       try
       {
@@ -768,9 +768,9 @@ do_install_thread (HINSTANCE h, HWND owner)
       catch (Exception *e)
       {
         if (yesno (owner, IDS_SKIP_PACKAGE, e->what()) == IDYES)
-          pkg.desired.pick (false, &pkg);
+          pkg.pick (false);
       }
-      if (pkg.desired.picked())
+      if (pkg.picked())
       {
         md5sum_total_bytes_sofar += pkg.desired.source()->size;
         total_bytes += pkg.desired.source()->size;
@@ -778,7 +778,7 @@ do_install_thread (HINSTANCE h, HWND owner)
       }
     }
 
-    if (pkg.desired.sourcePackage ().picked() || IncludeSource)
+    if (pkg.srcpicked() || IncludeSource)
     {
       bool skiprequested = false ;
       try
@@ -790,10 +790,10 @@ do_install_thread (HINSTANCE h, HWND owner)
         if (yesno (owner, IDS_SKIP_PACKAGE, e->what()) == IDYES)
 	{
 	  skiprequested = true ; //(err occurred,) skip pkg desired
-          pkg.desired.sourcePackage ().pick (false, &pkg);
+          pkg.srcpick (false);
 	}
       }
-      if (pkg.desired.sourcePackage().picked() || (IncludeSource && !skiprequested))
+      if (pkg.srcpicked() || (IncludeSource && !skiprequested))
       {
         md5sum_total_bytes_sofar += pkg.desired.sourcePackage ().source()->size;
         total_bytes += pkg.desired.sourcePackage ().source()->size;
@@ -801,8 +801,9 @@ do_install_thread (HINSTANCE h, HWND owner)
       }
     }
 
+    /* Upgrade or reinstall */
     if ((pkg.installed && pkg.desired != pkg.installed)
-        || pkg.installed.picked ())
+        || pkg.picked ())
     {
       uninstall_q.push_back (&pkg);
     }
diff --git a/package_db.cc b/package_db.cc
index 3d6d0de..4e22953 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -445,7 +445,7 @@ packagedb::defaultTrust (trusts trust)
         {
           pkg.desired = pkg.trustp (true, trust);
           if (pkg.desired)
-            pkg.desired.pick (pkg.desired.accessible() && pkg.desired != pkg.installed, &pkg);
+            pkg.pick (pkg.desired.accessible() && pkg.desired != pkg.installed);
         }
       else
         pkg.desired = packageversion ();
diff --git a/package_meta.cc b/package_meta.cc
index f37340b..55fe471 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -385,9 +385,9 @@ packagemeta::action_caption () const
     return "Uninstall";
   else if (!desired)
     return "Skip";
-  else if (desired == installed && desired.picked())
+  else if (desired == installed && picked())
     return packagedb::task == PackageDB_Install ? "Reinstall" : "Retrieve";
-  else if (desired == installed && desired.sourcePackage() && desired.sourcePackage().picked())
+  else if (desired == installed && desired.sourcePackage() && srcpicked())
     /* FIXME: Redo source should come up if the tarball is already present locally */
     return "Source";
   else if (desired == installed)	/* and neither src nor bin */
@@ -405,15 +405,15 @@ packagemeta::set_action (trusts const trust)
   /* Keep the picked settings of the former desired version, if any, and make
      sure at least one of them is picked.  If both are unpicked, pick the
      binary version. */
-  bool source_picked = desired && desired.sourcePackage().picked ();
-  bool binary_picked = !desired || desired.picked () || !source_picked;
+  bool source_picked = desired && srcpicked ();
+  bool binary_picked = !desired || picked () || !source_picked;
 
   /* If we're on "Keep" on the installed version, and the version is available,
      switch to "Reinstall". */
-  if (desired && desired == installed && !desired.picked ()
+  if (desired && desired == installed && !picked ()
       && desired.accessible ())
     {
-      desired.pick (true, this);
+      pick (true);
       return;
     }
 
@@ -445,9 +445,8 @@ packagemeta::set_action (trusts const trust)
       /* If the next version is the installed version, unpick it.  This will
 	 have the desired effect to show the package in "Keep" mode.  See also
 	 above for the code switching to "Reinstall". */
-      desired.pick (desired != installed && binary_picked, this);
-      desired.sourcePackage ().pick (desired.sourcePackage().accessible ()
-				     && source_picked, NULL);
+      pick (desired != installed && binary_picked);
+      srcpick (desired.sourcePackage().accessible () && source_picked);
     }
   else
     desired = packageversion ();
@@ -469,8 +468,8 @@ packagemeta::set_action (_actions action, packageversion const &default_version)
 	  desired = default_version;
 	  if (desired)
 	    {
-	      desired.pick (desired != installed, this);
-	      desired.sourcePackage ().pick (false, NULL);
+	      pick (desired != installed);
+	      srcpick (false);
 	    }
 	}
       else
@@ -486,18 +485,18 @@ packagemeta::set_action (_actions action, packageversion const &default_version)
 	    if (desired.accessible ())
 	      {
 		user_picked = true;
-		desired.pick (true, this);
-		desired.sourcePackage ().pick (false, NULL);
+		pick (true);
+		srcpick (false);
 	      }
 	    else
 	      {
-		desired.pick (false, NULL);
-		desired.sourcePackage ().pick (true, NULL);
+		pick (false);
+		srcpick (true);
 	      }
 	  else
 	    {
-	      desired.pick (false, NULL);
-	      desired.sourcePackage ().pick (false, NULL);
+	      pick (false);
+	      srcpick (false);
 	    }
 	}
       return;
@@ -507,8 +506,8 @@ packagemeta::set_action (_actions action, packageversion const &default_version)
       desired = installed;
       if (desired)
 	{
-	  desired.pick (true, this);
-	  desired.sourcePackage ().pick (false, NULL);
+	  pick (true);
+	  srcpick (false);
 	}
     }
   else if (action == Uninstall_action)
@@ -518,6 +517,34 @@ packagemeta::set_action (_actions action, packageversion const &default_version)
 }
 
 bool
+packagemeta::picked () const
+{
+  return _picked;
+}
+
+void
+packagemeta::pick (bool picked)
+{
+  _picked = picked;
+
+  // side effect: display message when picked (if not already seen)
+  if (picked)
+    this->message.display ();
+}
+
+bool
+packagemeta::srcpicked () const
+{
+  return _srcpicked;
+}
+
+void
+packagemeta::srcpick (bool picked)
+{
+  _srcpicked = picked;
+}
+
+bool
 packagemeta::accessible () const
 {
   for (set<packageversion>::iterator i=versions.begin();
@@ -607,7 +634,7 @@ packagemeta::logSelectionStatus() const
   const std::string installed =
    pkg.installed ? pkg.installed.Canonical_version () : "none";
 
-  Log (LOG_BABBLE) << "[" << pkg.name << "] action=" << action << " trust=" << trust << " installed=" << installed << " src?=" << (pkg.desired && pkg.desired.sourcePackage().picked() ? "yes" : "no") << endLog;
+  Log (LOG_BABBLE) << "[" << pkg.name << "] action=" << action << " trust=" << trust << " installed=" << installed << " src?=" << (pkg.desired && srcpicked() ? "yes" : "no") << endLog;
   if (pkg.categories.size ())
     Log (LOG_BABBLE) << "     categories=" << for_each(pkg.categories.begin(), pkg.categories.end(), StringConcatenator(", ")).result << endLog;
 #if 0
diff --git a/package_meta.h b/package_meta.h
index 640c253..dbd8eb9 100644
--- a/package_meta.h
+++ b/package_meta.h
@@ -35,7 +35,8 @@ public:
   static void ScanDownloadedFiles (bool);
   packagemeta (packagemeta const &);
   packagemeta (const std::string& pkgname)
-  : name (pkgname), key(pkgname), user_picked (false)
+    : name (pkgname), key(pkgname), user_picked (false),
+    _picked(false), _srcpicked(false)
   {
   }
 
@@ -134,6 +135,12 @@ public:
   /* What version does the user want ? */
   packageversion desired;
 
+  bool picked() const;   /* true if desired version is to be (re-)installed */
+  void pick(bool); /* trigger an install/reinstall */
+
+  bool srcpicked() const;   /* true if source for desired version is to be installed */
+  void srcpick(bool);
+
   packagemessage message;
 
   /* can one or more versions be installed? */
@@ -153,6 +160,8 @@ protected:
 private:
   std::string trustLabel(packageversion const &) const;
   std::vector <Script> scripts_;
+  bool _picked; /* true if desired version is to be (re)installed */
+  bool _srcpicked;
 };
 
 #endif /* SETUP_PACKAGE_META_H */
diff --git a/package_version.cc b/package_version.cc
index 60aae06..bb68229 100644
--- a/package_version.cc
+++ b/package_version.cc
@@ -53,8 +53,6 @@ public:
   const std::string LDesc () {return std::string();}
   void set_ldesc (const std::string& ) {}
   void uninstall (){}
-  void pick(bool const &newValue){/* Ignore attempts to pick this!. Throw an exception here if you want to detect such attemtps instead */}
-  virtual bool accessible () const {return false;}
 };
 static _defaultversion defaultversion;
 
@@ -237,20 +235,6 @@ packageversion::depends() const
   return data->depends;
 }
 
-bool
-packageversion::picked () const
-{
-  return data->picked;
-}
-
-void 
-packageversion::pick (bool aBool, packagemeta *pkg)
-{
-  data->pick(aBool);
-  if (pkg && aBool)
-    pkg->message.display ();
-}
-
 void
 packageversion::uninstall ()
 {
diff --git a/package_version.h b/package_version.h
index 0f83fdc..9351f26 100644
--- a/package_version.h
+++ b/package_version.h
@@ -111,9 +111,6 @@ public:
   void setDepends(const PackageDepends);
   const PackageDepends depends() const;
 
-  bool picked() const;   /* true if this version is to be installed */
-  void pick(bool, packagemeta *); /* trigger an install/reinsall */
-
   void uninstall ();
   /* invariant: never null */
   packagesource *source() const; /* where can we source the file from */
@@ -168,10 +165,6 @@ public:
 
   PackageDepends depends;
 
-  virtual void pick(bool const &newValue) { picked = newValue;}
-  bool picked;	/* non zero if this version is to be installed */
-		/* This will also trigger reinstalled if it is set */
-
   virtual void uninstall () = 0;
   packagesource source; /* where can we source the file from */
 
diff --git a/prereq.cc b/prereq.cc
index 0e7fdf5..16af7fa 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -302,20 +302,21 @@ PrereqChecker::selectMissing ()
   map <packagemeta *, vector <packagemeta *>, packagemeta_ltcomp>::iterator i;
   for (i = unmet.begin(); i != unmet.end(); i++)
     {
-      packageversion vers = i->first->trustp (false, theTrust);
-      i->first->desired = vers;
-      vers.sourcePackage ().pick (false, NULL);
+      packagemeta *pkg = i->first;
+      packageversion vers = pkg->trustp (false, theTrust);
+      pkg->desired = vers;
+      pkg->srcpick (false);
 
       if (vers == i->first->installed)
         {
-          vers.pick (false, NULL);
+          pkg->pick (false);
           Log (LOG_PLAIN) << "Adding required dependency " << i->first->name <<
                ": Selecting already-installed version " <<
                i->first->installed.Canonical_version () << "." << endLog;
         }
       else
         {
-          vers.pick (vers.accessible (), i->first);
+          pkg->pick (vers.accessible ());
           Log (LOG_PLAIN) << "Adding required dependency " << i->first->name <<
               ": Selecting version " << vers.Canonical_version () <<
               " for installation." << endLog;
-- 
2.12.3