setup libsolv UI: Silently install dependencies?

setup libsolv UI: Silently install dependencies?

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

Currently the libsolv version of setup silently installs added 
dependencies.  The user gets a chance to review them first only if 
libsolv finds problems and has to display the prerequisite page.

I'm not sure if this was a deliberate design decision, though I 
personally like it.  On the other hand, I suspect that some users won't 
like it because they're used to seeing a report about missing 
dependencies.  So I think we should make this behavior optional.

The attached patch is a start in that direction.  But it needs a 
followup patch to implement the mechanism for selecting the option.  One 
possibility is a checkbox on the chooser page, which then gets saved in 
setup.rc as a user option.  Are there other suggestions?

Ken

[-- Attachment #2: 0001-Let-the-user-review-added-dependencies.patch --]
[-- Type: text/plain, Size: 2388 bytes --]

From e3c5d8ac0970d8a6bcb870aff2e6fe0e00ab5b44 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Sun, 24 Dec 2017 16:05:33 -0500
Subject: [PATCH setup libsolv] Let the user review added dependencies

If libsolv finds no problems but there were added dependencies,
optionally activate the prerequisite page so that the user can go back
and see what was added.

This uses a new helper function prereq.cc:added_deps().

The option is contolled by a new bool data member
PrereqChecker::report_added_deps.  At the moment this is always
'true', for consistency with the currently released setup, but a
future commit will enable a way for the user to set the option.
---
 prereq.cc | 23 +++++++++++++++++++++--
 prereq.h  |  1 +
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/prereq.cc b/prereq.cc
index bf7661a..0c06a32 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -157,8 +157,26 @@ PrereqPage::OnUnattended ()
 // implements class PrereqChecker
 // ---------------------------------------------------------------------------
 
-// instantiate the static member
+// instantiate the static members
 bool PrereqChecker::use_test_packages;
+bool PrereqChecker::report_added_deps = true;
+
+static bool
+added_deps ()
+{
+  packagedb db;
+  const SolverTransactionList & trans = db.solution.transactions ();
+  for (SolverTransactionList::const_iterator i = trans.begin ();
+       i != trans.end (); i++)
+    if (i->type == SolverTransaction::transInstall)
+      {
+	packageversion pv = i->version;
+	packagemeta *pkg = db.findBinary (PackageSpecification (pv.Name ()));
+	if (!pkg->desired)
+	  return true;
+      }
+  return false;
+}
 
 bool
 PrereqChecker::isMet ()
@@ -174,7 +192,8 @@ PrereqChecker::isMet ()
   q.setTasks();
 
   // apply solver to those tasks and global state (use test, include source)
-  return db.solution.update(q, SolverSolution::keep, use_test_packages, IncludeSource);
+  return db.solution.update(q, SolverSolution::keep, use_test_packages, IncludeSource)
+    && !(report_added_deps && added_deps ());
 }
 
 /* Formats problems and solutions as a string for display to the user.  */
diff --git a/prereq.h b/prereq.h
index 5ae9323..a14789a 100644
--- a/prereq.h
+++ b/prereq.h
@@ -44,6 +44,7 @@ public:
 
 private:
   static bool use_test_packages;
+  static bool report_added_deps;
 };
 
 #endif /* SETUP_PREREQ_H */
-- 
2.15.1

Re: setup libsolv UI: Silently install dependencies?

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

On 12/24/2017 6:23 PM, Ken Brown wrote:
> Currently the libsolv version of setup silently installs added 
> dependencies.  The user gets a chance to review them first only if 
> libsolv finds problems and has to display the prerequisite page.
> 
> I'm not sure if this was a deliberate design decision, though I 
> personally like it.  On the other hand, I suspect that some users won't 
> like it because they're used to seeing a report about missing 
> dependencies.  So I think we should make this behavior optional.
> 
> The attached patch is a start in that direction.  But it needs a 
> followup patch to implement the mechanism for selecting the option.  One 
> possibility is a checkbox on the chooser page, which then gets saved in 
> setup.rc as a user option.

And here's a followup patch that implements that possibility.

[Side comment: Jon, in v2 of commit 9367283, you removed some newlines, 
saying that they weren't needed because wrapping would be done.  But I'm 
not seeing that wrapping; I have to scroll horizontally to see the whole 
text.]

Ken


[-- Attachment #2: 0001-Implement-user-option-to-report-added-dependencies.patch --]
[-- Type: text/plain, Size: 5158 bytes --]

From f3aadfead49f9ec17f04e0051d6d58f619c5092b Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Mon, 25 Dec 2017 14:04:20 -0500
Subject: [PATCH setup libsolv] Implement user option to report added
 dependencies

Add a checkbox IDC_CHOOSE_REPORT to IDD_CHOOSE_DIALOG that determines
the value of PrereqChecker::report_added_deps.  Save the user's choice
in a new user setting "report_user_deps".
---
 choose.cc  | 14 ++++++++++++++
 prereq.cc  |  2 +-
 prereq.h   |  2 ++
 res.rc     |  4 ++++
 resource.h |  2 ++
 5 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/choose.cc b/choose.cc
index ff57d85..d5e55b8 100644
--- a/choose.cc
+++ b/choose.cc
@@ -83,6 +83,7 @@ static ControlAdjuster::ControlInfo ChooserControlsInfo[] = {
   {IDC_CHOOSE_VIEWCAPTION,	CP_LEFT,    CP_TOP},
   {IDC_CHOOSE_LIST,		CP_STRETCH, CP_STRETCH},
   {IDC_CHOOSE_HIDE,             CP_LEFT,    CP_BOTTOM},
+  {IDC_CHOOSE_REPORT,           CP_LEFT,    CP_BOTTOM},
   {0, CP_LEFT, CP_TOP}
 };
 
@@ -128,6 +129,8 @@ ChooserPage::~ChooserPage ()
 	}
       UserSettings::instance().set ("chooser_window_settings", toset);
     }
+  UserSettings::instance().set ("report_added_deps",
+				PrereqChecker::reportAddedDeps () ? "yes" : "no");
 }
 
 void
@@ -255,6 +258,13 @@ ChooserPage::OnInit ()
 {
   CheckDlgButton (GetHWND (), IDC_CHOOSE_HIDE, BST_CHECKED);
 
+  const char *fg_ret =
+    UserSettings::instance().get ("report_added_deps");
+  bool report_added_deps = !fg_ret || strcmp (fg_ret, "no" ) != 0;
+  PrereqChecker::setAddedDeps (report_added_deps);
+  CheckDlgButton (GetHWND (), IDC_CHOOSE_REPORT,
+		  report_added_deps ? BST_CHECKED : BST_UNCHECKED);
+
   /* Populate view dropdown list with choices */
   HWND viewlist = GetDlgItem (IDC_CHOOSE_VIEW);
   SendMessage (viewlist, CB_RESETCONTENT, 0, 0);
@@ -279,6 +289,7 @@ ChooserPage::OnInit ()
   AddTooltip (IDC_CHOOSE_VIEW, IDS_VIEWBUTTON_TOOLTIP);
   AddTooltip (IDC_CHOOSE_HIDE, IDS_HIDEOBS_TOOLTIP);
   AddTooltip (IDC_CHOOSE_SEARCH_EDIT, IDS_SEARCH_TOOLTIP);
+  AddTooltip (IDC_CHOOSE_REPORT, IDS_REPORTDEPS_TOOLTIP);
 
   /* Set focus to search edittext control. */
   PostMessage (GetHWND (), WM_NEXTDLGCTL,
@@ -536,6 +547,9 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
     case IDC_CHOOSE_HIDE:
       chooser->setObsolete (!IsButtonChecked (id));
       break;
+    case IDC_CHOOSE_REPORT:
+      PrereqChecker::setAddedDeps (IsButtonChecked (id));
+      break;
     default:
       // Wasn't recognized or handled.
       return false;
diff --git a/prereq.cc b/prereq.cc
index 0c06a32..231e6dc 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -159,7 +159,7 @@ PrereqPage::OnUnattended ()
 
 // instantiate the static members
 bool PrereqChecker::use_test_packages;
-bool PrereqChecker::report_added_deps = true;
+bool PrereqChecker::report_added_deps;
 
 static bool
 added_deps ()
diff --git a/prereq.h b/prereq.h
index a14789a..c79d306 100644
--- a/prereq.h
+++ b/prereq.h
@@ -41,6 +41,8 @@ public:
   void getUnmetString (std::string &s);
 
   static void setTestPackages (bool t) { use_test_packages = t; };
+  static void setAddedDeps (bool t) { report_added_deps = t; };
+  static bool reportAddedDeps () { return report_added_deps; };
 
 private:
   static bool use_test_packages;
diff --git a/res.rc b/res.rc
index ee4270a..7d0de79 100644
--- a/res.rc
+++ b/res.rc
@@ -363,6 +363,8 @@ BEGIN
                     WS_VISIBLE, 7, 45, SETUP_STANDARD_DIALOG_W - 14, 122
     CONTROL         "&Hide obsolete packages", IDC_CHOOSE_HIDE,
                     "Button", BS_AUTOCHECKBOX | WS_TABSTOP, 7, 167, 160, 14
+    CONTROL         "&Report added dependencies", IDC_CHOOSE_REPORT,
+                    "Button", BS_AUTOCHECKBOX | WS_TABSTOP, 167, 167, 160, 14
     ICON            IDI_CYGWIN, IDC_HEADICON, SETUP_HEADICON_X, 0, 21, 20
     LTEXT           "Select the packages you want setup to install.",
                     IDC_CHOOSE_INST_TEXT, 21, 9, 239, 16, NOT WS_GROUP
@@ -588,6 +590,8 @@ BEGIN
        "with names that begin with '_'.  Such packages are usually empty "
        "placeholders for packages that have been removed or renamed, or are "
        "infrastructure packages that are handled automatically."
+    IDS_REPORTDEPS_TOOLTIP "If packages are added to satisfy dependencies, "
+       "pause to allow review of them."
     IDS_SIG_INVALID    "Mirror Error:  Setup.ini signature %s from %s failed to verify.\nPossible corrupt mirror?  Setup.ini rejected."
     IDS_CRYPTO_ERROR   "Internal Error:  gcrypt library error %d %s"
     IDS_SEARCH_TOOLTIP "Search for this string in package names."
diff --git a/resource.h b/resource.h
index e385c9e..73cf06c 100644
--- a/resource.h
+++ b/resource.h
@@ -40,6 +40,7 @@
 #define IDS_INSTALLEDB_VERSION            140
 #define IDS_DOWNLOAD_INCOMPLETE_EXIT      141
 #define IDS_TRUSTSYNC_TOOLTIP             142
+#define IDS_REPORTDEPS_TOOLTIP            143
 
 // Dialogs
 
@@ -180,3 +181,4 @@
 #define IDC_DOWNLOAD_EDIT                 594
 #define IDC_CHOOSE_DO_SEARCH              595
 #define IDC_CHOOSE_SYNC                   596
+#define IDC_CHOOSE_REPORT                 597
-- 
2.15.1

Re: setup libsolv UI: Silently install dependencies?

[Jon Turney]

On 24/12/2017 23:23, Ken Brown wrote:
> Currently the libsolv version of setup silently installs added 
> dependencies.  The user gets a chance to review them first only if 
> libsolv finds problems and has to display the prerequisite page.
> 
> I'm not sure if this was a deliberate design decision, though I 
> personally like it.  On the other hand, I suspect that some users won't 
> like it because they're used to seeing a report about missing 
> dependencies.  So I think we should make this behavior optional.

Yeah.  Ideally we'd show what we're going to do, before we do it.

This is a bit more complex than before, as the solver might decide to 
remove or upgrade packages, rather than just install extra ones.

> The attached patch is a start in that direction.  But it needs a 
> followup patch to implement the mechanism for selecting the option.  One 
> possibility is a checkbox on the chooser page, which then gets saved in 
> setup.rc as a user option.  Are there other suggestions?

But, somewhat confused by this.  Isn't this just going to show the 
(empty) problem report?

Re: setup libsolv UI: Silently install dependencies?

[Ken Brown]

On 1/5/2018 10:48 AM, Jon Turney wrote:
> On 24/12/2017 23:23, Ken Brown wrote:
>> Currently the libsolv version of setup silently installs added 
>> dependencies.  The user gets a chance to review them first only if 
>> libsolv finds problems and has to display the prerequisite page.
>>
>> I'm not sure if this was a deliberate design decision, though I 
>> personally like it.  On the other hand, I suspect that some users 
>> won't like it because they're used to seeing a report about missing 
>> dependencies.  So I think we should make this behavior optional.
> 
> Yeah.  Ideally we'd show what we're going to do, before we do it.
> 
> This is a bit more complex than before, as the solver might decide to 
> remove or upgrade packages, rather than just install extra ones.
> 
>> The attached patch is a start in that direction.  But it needs a 
>> followup patch to implement the mechanism for selecting the option.  
>> One possibility is a checkbox on the chooser page, which then gets 
>> saved in setup.rc as a user option.  Are there other suggestions?
> 
> But, somewhat confused by this.  Isn't this just going to show the 
> (empty) problem report?

No, it shows a report that says, "Click Back and select the Pending view 
to review the default problem solutions, including packages that were 
added to resolve dependencies."  This isn't ideal, because it forces the 
user to select 'Back', and even then the user can't see explicitly why 
packages were added.  It would be better if we could get libsolv to give 
us a report about added dependencies.  Do you know if this is possible?

Ken

Re: setup libsolv UI: Silently install dependencies?

[Jon Turney]

On 25/12/2017 19:18, Ken Brown wrote:
> [Side comment: Jon, in v2 of commit 9367283, you removed some newlines, 
> saying that they weren't needed because wrapping would be done.  But I'm 
> not seeing that wrapping; I have to scroll horizontally to see the whole 
> text.]

Yeah, what I wrote was nonsense.

I think it's a bit visually inconsistent to put fixed linebreaks into 
this text.  I think it would be more consistent to either (i) wrap at 
the width of the edittext control, or (ii) not wrapped at all (like the 
problems)

I'm not sure if (ii) is achievable, though (perhaps with an 
EditWordBreakProc function?).

Or maybe just remove the hscroll style so everything gets wrapped?

I think I'll just put it back the way you had it, for the moment.

Re: setup libsolv UI: Silently install dependencies?

[Jon Turney]

On 05/01/2018 16:04, Ken Brown wrote:
> On 1/5/2018 10:48 AM, Jon Turney wrote:
>> On 24/12/2017 23:23, Ken Brown wrote:
>>> Currently the libsolv version of setup silently installs added 
>>> dependencies.  The user gets a chance to review them first only if 
>>> libsolv finds problems and has to display the prerequisite page.
>>>
>>> I'm not sure if this was a deliberate design decision, though I 
>>> personally like it.  On the other hand, I suspect that some users 
>>> won't like it because they're used to seeing a report about missing 
>>> dependencies.  So I think we should make this behavior optional.
>>
>> Yeah.  Ideally we'd show what we're going to do, before we do it.
>>
>> This is a bit more complex than before, as the solver might decide to 
>> remove or upgrade packages, rather than just install extra ones.
>>
>>> The attached patch is a start in that direction.  But it needs a 
>>> followup patch to implement the mechanism for selecting the option. 
>>> One possibility is a checkbox on the chooser page, which then gets 
>>> saved in setup.rc as a user option.  Are there other suggestions?
>>
>> But, somewhat confused by this.  Isn't this just going to show the 
>> (empty) problem report?
> 
> No, it shows a report that says, "Click Back and select the Pending view 
> to review the default problem solutions, including packages that were 
> added to resolve dependencies."  This isn't ideal, because it forces the 
> user to select 'Back', and even then the user can't see explicitly why 
> packages were added.  It would be better if we could get libsolv to give 

Ah, right.

> us a report about added dependencies.  Do you know if this is possible?

Hmmm.. there's the SOLVER_USERINSTALLED flag we can apply to a task. 
Perhaps that is passed through, so we can see what's been installed 
directly vs. dependencies.

Otherwise I guess you could to compare input and output task lists to 
see what has been added by the solver?

I'm not sure it's ideal to make a text report of this, though. 
Displaying it in the chooser somehow seems a better fit.

(i.e. after you've made your selections, you get something like the 
"pending" view in the chooser again, showing what will be done, which 
you can either confirm or go back from)

Re: setup libsolv UI: Silently install dependencies?

[Ken Brown]

On 1/9/2018 8:32 AM, Jon Turney wrote:
> On 05/01/2018 16:04, Ken Brown wrote:
>> On 1/5/2018 10:48 AM, Jon Turney wrote:
>>> On 24/12/2017 23:23, Ken Brown wrote:
>>>> Currently the libsolv version of setup silently installs added 
>>>> dependencies.  The user gets a chance to review them first only if 
>>>> libsolv finds problems and has to display the prerequisite page.
>>>>
>>>> I'm not sure if this was a deliberate design decision, though I 
>>>> personally like it.  On the other hand, I suspect that some users 
>>>> won't like it because they're used to seeing a report about missing 
>>>> dependencies.  So I think we should make this behavior optional.
>>>
>>> Yeah.  Ideally we'd show what we're going to do, before we do it.
>>>
>>> This is a bit more complex than before, as the solver might decide to 
>>> remove or upgrade packages, rather than just install extra ones.
>>>
>>>> The attached patch is a start in that direction.  But it needs a 
>>>> followup patch to implement the mechanism for selecting the option. 
>>>> One possibility is a checkbox on the chooser page, which then gets 
>>>> saved in setup.rc as a user option.  Are there other suggestions?
>>>
>>> But, somewhat confused by this.  Isn't this just going to show the 
>>> (empty) problem report?
>>
>> No, it shows a report that says, "Click Back and select the Pending 
>> view to review the default problem solutions, including packages that 
>> were added to resolve dependencies."  This isn't ideal, because it 
>> forces the user to select 'Back', and even then the user can't see 
>> explicitly why packages were added.  It would be better if we could 
>> get libsolv to give 
> 
> Ah, right.
> 
>> us a report about added dependencies.  Do you know if this is possible?
> 
> Hmmm.. there's the SOLVER_USERINSTALLED flag we can apply to a task. 
> Perhaps that is passed through, so we can see what's been installed 
> directly vs. dependencies.
> 
> Otherwise I guess you could to compare input and output task lists to 
> see what has been added by the solver?
> 
> I'm not sure it's ideal to make a text report of this, though. 
> Displaying it in the chooser somehow seems a better fit.
> 
> (i.e. after you've made your selections, you get something like the 
> "pending" view in the chooser again, showing what will be done, which 
> you can either confirm or go back from)

Yes, that does seem better.

Ken

Re: setup libsolv UI: Silently install dependencies?

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On 1/9/2018 8:32 AM, Jon Turney wrote:
> I'm not sure it's ideal to make a text report of this, though. 
> Displaying it in the chooser somehow seems a better fit.
> 
> (i.e. after you've made your selections, you get something like the 
> "pending" view in the chooser again, showing what will be done, which 
> you can either confirm or go back from)

Here's another attempt, lightly tested, which might still need some 
improvements.  It goes back to the chooser, but it doesn't set the 
pending view.  I'm not sure how to do that without a lot of refactoring, 
but maybe I'm missing something easy.

Ken

[-- Attachment #2: 0001-Let-the-user-review-added-dependencies.patch --]
[-- Type: text/plain, Size: 2464 bytes --]

From d183ee53ff0471f22fe2412381e8507e4c109b1e Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Mon, 15 Jan 2018 12:18:30 -0500
Subject: [PATCH] Let the user review added dependencies

If the solver finds no problems but added packages to resolve
dependencies, give the user a chance to go back to the chooser and
review what's about to happen.
---
 prereq.cc | 46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/prereq.cc b/prereq.cc
index a03e79b..4053373 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -140,13 +140,18 @@ PrereqPage::whatNext ()
   return IDD_INSTATUS;
 }
 
-long
-PrereqPage::OnBack ()
+static void
+prepBack ()
 {
   // Reset the package database to correspond to the solver's solution
   packagedb db;
   db.solution.trans2db();
+}
 
+long
+PrereqPage::OnBack ()
+{
+  prepBack ();
   return IDD_CHOOSE;
 }
 
@@ -213,6 +218,23 @@ PrereqChecker::getUnmetString (std::string &s)
 // progress page glue
 // ---------------------------------------------------------------------------
 
+static bool
+added_deps ()
+{
+  packagedb db;
+  const SolverTransactionList & trans = db.solution.transactions ();
+  for (SolverTransactionList::const_iterator i = trans.begin ();
+       i != trans.end (); i++)
+    if (i->type == SolverTransaction::transInstall)
+      {
+	packageversion pv = i->version;
+	packagemeta *pkg = db.findBinary (PackageSpecification (pv.Name ()));
+	if (!pkg->desired)
+	  return true;
+      }
+  return false;
+}
+
 static int
 do_prereq_check_thread(HINSTANCE h, HWND owner)
 {
@@ -223,11 +245,23 @@ do_prereq_check_thread(HINSTANCE h, HWND owner)
     {
       p.finalize();
 
-      if (source == IDC_SOURCE_LOCALDIR)
-	Progress.SetActivateTask (WM_APP_START_INSTALL);  // install
+      if (added_deps () && !unattended_mode
+	  && MessageBox (owner, "Packages were added to resolve dependencies.  "
+			 "Do you want to review them before proceeding?",
+			 "Added Dependencies",
+			 MB_YESNO | MB_DEFBUTTON2) == IDYES)
+	{
+	  prepBack ();
+	  retval = IDD_CHOOSE;
+	}
       else
-	Progress.SetActivateTask (WM_APP_START_DOWNLOAD); // start download
-      retval = IDD_INSTATUS;
+	{
+	  if (source == IDC_SOURCE_LOCALDIR)
+	    Progress.SetActivateTask (WM_APP_START_INSTALL);  // install
+	  else
+	    Progress.SetActivateTask (WM_APP_START_DOWNLOAD); // start download
+	  retval = IDD_INSTATUS;
+	}
     }
   else
     {
-- 
2.15.1

Re: setup libsolv UI: Silently install dependencies?

[Ken Brown]

On 1/15/2018 12:31 PM, Ken Brown wrote:
> On 1/9/2018 8:32 AM, Jon Turney wrote:
>> I'm not sure it's ideal to make a text report of this, though. 
>> Displaying it in the chooser somehow seems a better fit.
>>
>> (i.e. after you've made your selections, you get something like the 
>> "pending" view in the chooser again, showing what will be done, which 
>> you can either confirm or go back from)
> 
> Here's another attempt, lightly tested, which might still need some 
> improvements.  It goes back to the chooser, but it doesn't set the 
> pending view.

I've added code that allows the PrereqChecker to set the pending view 
before going back.  I'll send a revised patch shortly.

Ken