[PATCH setup libsolv, v2] Let the user review added dependencies

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH setup libsolv, v2] Let the user review added dependencies

Ken Brown-6
If the solver found no problems but added packages to resolve
dependencies, ask the user whether they want to review the added
packages before proceeding.

If they answer Yes, go back to the chooser with the 'Pending' view
selected.  The implementation adds several new members to the
PrereqChecker class so that the latter can communicate with the
chooser page.
---
 choose.cc |  3 +++
 prereq.cc | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 prereq.h  |  8 ++++++++
 3 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/choose.cc b/choose.cc
index 1c79cca..846b169 100644
--- a/choose.cc
+++ b/choose.cc
@@ -286,6 +286,9 @@ ChooserPage::OnInit ()
   /* Set focus to search edittext control. */
   PostMessage (GetHWND (), WM_NEXTDLGCTL,
        (WPARAM) GetDlgItem (IDC_CHOOSE_SEARCH_EDIT), TRUE);
+
+  /* Store the HWNDs that the PrereqChecker will need.  */
+  PrereqChecker::setChooserHWNDs (GetHWND (), viewlist);
 }
 
 void
diff --git a/prereq.cc b/prereq.cc
index effa7cc..7821d63 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -35,6 +35,7 @@
 #include "msg.h"
 #include "Exception.h"
 #include "getopt++/BoolOption.h"
+#include "PickView.h"
 
 // Sizing information.
 static ControlAdjuster::ControlInfo PrereqControlsInfo[] = {
@@ -139,8 +140,8 @@ PrereqPage::whatNext ()
   return IDD_INSTATUS;
 }
 
-long
-PrereqPage::OnBack ()
+static void
+prepBack ()
 {
   // Add reinstall tasks
   PrereqChecker p;
@@ -149,7 +150,12 @@ PrereqPage::OnBack ()
   // Reset the package database to correspond to the solver's solution
   packagedb db;
   db.solution.trans2db();
+}
 
+long
+PrereqPage::OnBack ()
+{
+  prepBack ();
   return IDD_CHOOSE;
 }
 
@@ -170,6 +176,7 @@ PrereqPage::OnUnattended ()
 // instantiate the static members
 bool PrereqChecker::use_test_packages;
 SolverTasks PrereqChecker::q;
+HWND PrereqChecker::hChooser, PrereqChecker::hChooseView;
 
 bool
 PrereqChecker::isMet ()
@@ -204,6 +211,15 @@ PrereqChecker::augment ()
   db.solution.augmentTasks(q);
 }
 
+void
+PrereqChecker::setChooserView (PickView::views view)
+{
+  PostMessage (hChooseView, CB_SETCURSEL, (WPARAM)view, 0);
+  PostMessage (hChooser, WM_COMMAND,
+       MAKEWPARAM (IDC_CHOOSE_VIEW, CBN_SELCHANGE),
+       (LPARAM)hChooseView);
+}
+
 /* Formats problems and solutions as a string for display to the user.  */
 void
 PrereqChecker::getUnmetString (std::string &s)
@@ -224,6 +240,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)
 {
@@ -232,17 +265,34 @@ do_prereq_check_thread(HINSTANCE h, HWND owner)
 
   if (p.isMet ())
     {
-      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?"
+ "\r\n\r\n"
+ "Answering 'Yes' will take you back to the Package "
+ "Selection page, with the 'Pending' view selected.",
+ "Added Dependencies",
+ MB_YESNO | MB_DEFBUTTON2) == IDYES)
+ {
+  Progress.SetText1 ("Preparing package review...");
+  prepBack ();
+  p.setChooserView ();
+  retval = IDD_CHOOSE;
+ }
       else
- Progress.SetActivateTask (WM_APP_START_DOWNLOAD); // start download
-      retval = IDD_INSTATUS;
+ {
+  p.finalize();
+
+  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
     {
-      // rut-roh, some required things are not selected
+      // The solver reported problems.
       retval = IDD_PREREQ;
     }
 
diff --git a/prereq.h b/prereq.h
index 24e6de5..d60ef59 100644
--- a/prereq.h
+++ b/prereq.h
@@ -5,6 +5,8 @@
 #include "proppage.h"
 #include "PackageTrust.h"
 #include "package_meta.h"
+#include "win32.h"
+#include "PickView.h"
 
 using namespace std;
 
@@ -45,11 +47,17 @@ public:
 
   void augment ();
 
+  void setChooserView (PickView::views = PickView::views::PackagePending);
+
   static void setTestPackages (bool t) { use_test_packages = t; };
 
+  static void setChooserHWNDs (HWND hc, HWND hv)
+  { hChooser = hc; hChooseView = hv; };
+
 private:
   static bool use_test_packages;
   static SolverTasks q;
+  static HWND hChooser, hChooseView;
 };
 
 #endif /* SETUP_PREREQ_H */
--
2.15.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

Brian Inglis
On 2018-01-22 10:37, Ken Brown wrote:
> If the solver found no problems but added packages to resolve
> dependencies, ask the user whether they want to review the added
> packages before proceeding.
>
> If they answer Yes, go back to the chooser with the 'Pending' view
> selected.  The implementation adds several new members to the
> PrereqChecker class so that the latter can communicate with the
> chooser page.

Would it not be better, quicker, and easier, and to just show the updated
Pending view directly and unconditionally, without another popup dialogue box?
I think that's what most of us would expect from a UI display: just update the
displayed packages, if not being asked for confirmation that we really want to
do something possibly detrimental.

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

Ken Brown-6
On 1/22/2018 4:51 PM, Brian Inglis wrote:

> On 2018-01-22 10:37, Ken Brown wrote:
>> If the solver found no problems but added packages to resolve
>> dependencies, ask the user whether they want to review the added
>> packages before proceeding.
>>
>> If they answer Yes, go back to the chooser with the 'Pending' view
>> selected.  The implementation adds several new members to the
>> PrereqChecker class so that the latter can communicate with the
>> chooser page.
>
> Would it not be better, quicker, and easier, and to just show the updated
> Pending view directly and unconditionally, without another popup dialogue box?
> I think that's what most of us would expect from a UI display: just update the
> displayed packages, if not being asked for confirmation that we really want to
> do something possibly detrimental.

Yes, I think you're right, in principle.  But I'm not sure exactly what
the UI should do.  If we simply update the displayed packages when the
user selects 'Next', won't the user wonder what's going on?  Can you
suggest a way to make it clear that we've updated the display to reflect
added dependencies and that we're waiting for final confirmation?

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

Brian Inglis
On 2018-01-22 15:40, Ken Brown wrote:

> On 1/22/2018 4:51 PM, Brian Inglis wrote:
>> On 2018-01-22 10:37, Ken Brown wrote:
>>> If the solver found no problems but added packages to resolve
>>> dependencies, ask the user whether they want to review the added
>>> packages before proceeding.
>>>
>>> If they answer Yes, go back to the chooser with the 'Pending' view
>>> selected.  The implementation adds several new members to the
>>> PrereqChecker class so that the latter can communicate with the
>>> chooser page.
>>
>> Would it not be better, quicker, and easier, and to just show the updated
>> Pending view directly and unconditionally, without another popup dialogue box?
>> I think that's what most of us would expect from a UI display: just update the
>> displayed packages, if not being asked for confirmation that we really want to
>> do something possibly detrimental.
>
> Yes, I think you're right, in principle.  But I'm not sure exactly what the UI
> should do.  If we simply update the displayed packages when the user selects
> 'Next', won't the user wonder what's going on?  Can you suggest a way to make it
> clear that we've updated the display to reflect added dependencies and that
> we're waiting for final confirmation?

I see what you mean: we don't have a status line or any way to highlight added
dependencies which we could use to convey that, so unless the addition of
dependencies to the Pending view is visually very obvious, we need to use a
dialogue box to pass on the message, before proceeding from the Pending view.

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

cyg Simple
On 1/22/2018 8:07 PM, Brian Inglis wrote:

> On 2018-01-22 15:40, Ken Brown wrote:
>> On 1/22/2018 4:51 PM, Brian Inglis wrote:
>>> On 2018-01-22 10:37, Ken Brown wrote:
>>>> If the solver found no problems but added packages to resolve
>>>> dependencies, ask the user whether they want to review the added
>>>> packages before proceeding.
>>>>
>>>> If they answer Yes, go back to the chooser with the 'Pending' view
>>>> selected.  The implementation adds several new members to the
>>>> PrereqChecker class so that the latter can communicate with the
>>>> chooser page.
>>>
>>> Would it not be better, quicker, and easier, and to just show the updated
>>> Pending view directly and unconditionally, without another popup dialogue box?
>>> I think that's what most of us would expect from a UI display: just update the
>>> displayed packages, if not being asked for confirmation that we really want to
>>> do something possibly detrimental.
>>
>> Yes, I think you're right, in principle.  But I'm not sure exactly what the UI
>> should do.  If we simply update the displayed packages when the user selects
>> 'Next', won't the user wonder what's going on?  Can you suggest a way to make it
>> clear that we've updated the display to reflect added dependencies and that
>> we're waiting for final confirmation?
>
> I see what you mean: we don't have a status line or any way to highlight added
> dependencies which we could use to convey that, so unless the addition of
> dependencies to the Pending view is visually very obvious, we need to use a
> dialogue box to pass on the message, before proceeding from the Pending view.
>

How about changing the background color when the pending view list
changes?  The typical case for added dependencies is to just accept
them; especially if the primary package is really wanted.  Maybe rotate
between three or four colors to indicate new data in the list for each
selection but just two is good as well.

Also maybe changing the text on the Next button to indicate the total
number of packages pending action.  It would be another visual clue.

--
cyg Simple
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

Jon TURNEY
In reply to this post by Ken Brown-6
On 22/01/2018 17:37, Ken Brown wrote:
> If the solver found no problems but added packages to resolve
> dependencies, ask the user whether they want to review the added
> packages before proceeding.
>
> If they answer Yes, go back to the chooser with the 'Pending' view
> selected.  The implementation adds several new members to the
> PrereqChecker class so that the latter can communicate with the
> chooser page.

As discussed, this approach could be confusing.

Attached is a slightly different approach, which adds a new page to
review and confirm what actions we're going to take.

For the moment, this just contains a simple text report, but I guess
this could be extended e.g. to use a grid control, or give reasons for
why packages are being installed.

0001-Add-a-new-page-to-let-the-user-review-and-confirm-ac.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

Ken Brown-6
On 1/24/2018 3:30 PM, Jon Turney wrote:

> On 22/01/2018 17:37, Ken Brown wrote:
>> If the solver found no problems but added packages to resolve
>> dependencies, ask the user whether they want to review the added
>> packages before proceeding.
>>
>> If they answer Yes, go back to the chooser with the 'Pending' view
>> selected.  The implementation adds several new members to the
>> PrereqChecker class so that the latter can communicate with the
>> chooser page.
>
> As discussed, this approach could be confusing.
>
> Attached is a slightly different approach, which adds a new page to
> review and confirm what actions we're going to take.
>
> For the moment, this just contains a simple text report, but I guess
> this could be extended e.g. to use a grid control, or give reasons for
> why packages are being installed.

I like this a lot.  It's *much* better than my approach.

My only suggestion is that you rethink what the confirm page should do
in "download only" mode.  Maybe that page could even be skipped.  But if
you think the user should confirm the download, there's no need to
mention "uninstall" actions, and "install" actions should be reported as
"download".

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

Ken Brown-6
On 1/24/2018 10:25 PM, Ken Brown wrote:

> On 1/24/2018 3:30 PM, Jon Turney wrote:
>> On 22/01/2018 17:37, Ken Brown wrote:
>>> If the solver found no problems but added packages to resolve
>>> dependencies, ask the user whether they want to review the added
>>> packages before proceeding.
>>>
>>> If they answer Yes, go back to the chooser with the 'Pending' view
>>> selected.  The implementation adds several new members to the
>>> PrereqChecker class so that the latter can communicate with the
>>> chooser page.
>>
>> As discussed, this approach could be confusing.
>>
>> Attached is a slightly different approach, which adds a new page to
>> review and confirm what actions we're going to take.
>>
>> For the moment, this just contains a simple text report, but I guess
>> this could be extended e.g. to use a grid control, or give reasons for
>> why packages are being installed.
>
> I like this a lot.  It's *much* better than my approach.
>
> My only suggestion is that you rethink what the confirm page should do
> in "download only" mode.  Maybe that page could even be skipped.  But if
> you think the user should confirm the download, there's no need to
> mention "uninstall" actions, and "install" actions should be reported as
> "download".

One other minor detail: What should happen if nothing is being done?  I
don't think we necessarily want to skip the confirm page, because maybe
the user was expecting something to happen.  But it looks a little
strange to just display an empty report.  Maybe the report should say
something like "No changes will be made" or, in download mode, "Nothing
will be downloaded".

Ken

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup libsolv, v2] Let the user review added dependencies

Jon TURNEY
On 25/01/2018 03:57, Ken Brown wrote:

> On 1/24/2018 10:25 PM, Ken Brown wrote:
>> On 1/24/2018 3:30 PM, Jon Turney wrote:
>>> On 22/01/2018 17:37, Ken Brown wrote:
>>>> If the solver found no problems but added packages to resolve
>>>> dependencies, ask the user whether they want to review the added
>>>> packages before proceeding.
>>>>
>>>> If they answer Yes, go back to the chooser with the 'Pending' view
>>>> selected.  The implementation adds several new members to the
>>>> PrereqChecker class so that the latter can communicate with the
>>>> chooser page.
>>>
>>> As discussed, this approach could be confusing.
>>>
>>> Attached is a slightly different approach, which adds a new page to
>>> review and confirm what actions we're going to take.
>>>
>>> For the moment, this just contains a simple text report, but I guess
>>> this could be extended e.g. to use a grid control, or give reasons
>>> for why packages are being installed.
>>
>> I like this a lot.  It's *much* better than my approach.
>>
>> My only suggestion is that you rethink what the confirm page should do
>> in "download only" mode.  Maybe that page could even be skipped.  But
>> if you think the user should confirm the download, there's no need to
>> mention "uninstall" actions, and "install" actions should be reported
>> as "download".
>
> One other minor detail: What should happen if nothing is being done?  I
> don't think we necessarily want to skip the confirm page, because maybe
> the user was expecting something to happen.  But it looks a little
> strange to just display an empty report.  Maybe the report should say
> something like "No changes will be made" or, in download mode, "Nothing
> will be downloaded".

Good points. I adjusted the output as you suggest.