[PATCH setup 1/4] Build C++ code with -std=gnu++11

[PATCH setup 1/4] Build C++ code with -std=gnu++11

[Jon Turney]

Use BASECXXFLAGS rather then AM_CXXFLAGS to make AM_CFLAGS
Rationalize BASECXXFLAGS, adding -Werror and removing -Wno-uninitialized
(since the bug preventing it being used is long fixed)
Fix a bug detected by -Wuninitialized
Build C++ code with -std=gnu++11 -Wno-deprecated-declarations
---
 Makefile.am | 9 ++++-----
 ini.cc      | 2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 7fa61e9..044a1ce 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -21,13 +21,12 @@ SUBDIRS := @subdirs@ tests
 
 # We would like to use -Winline for C++ as well, but some STL code triggers
 # this warning. (Bug verified present in gcc-3.3)
-# -Wno-uninitialized added to deal with g++ 3.4.4's spurious STL warnings
-# (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22207)
-BASECXXFLAGS = -Wall -Wno-uninitialized -Wpointer-arith -Wcomments \
+BASECXXFLAGS = -Werror -Wall -Wpointer-arith -Wcomments \
 	       -Wcast-align -Wwrite-strings -fno-builtin-sscanf \
 	       -Wno-attributes
-AM_CXXFLAGS = -Werror $(BASECXXFLAGS) ${$(*F)_CXXFLAGS}
-AM_CFLAGS = $(AM_CXXFLAGS) -Wmissing-declarations -Winline \
+AM_CXXFLAGS = $(BASECXXFLAGS) -std=gnu++11 -Wno-deprecated-declarations \
+	      ${$(*F)_CXXFLAGS}
+AM_CFLAGS = $(BASECXXFLAGS) -Wmissing-declarations -Winline \
 	    -Wstrict-prototypes -Wmissing-prototypes
 AM_YFLAGS = -d
 AM_LFLAGS = -8
diff --git a/ini.cc b/ini.cc
index f925bf5..82990a2 100644
--- a/ini.cc
+++ b/ini.cc
@@ -270,7 +270,7 @@ do_remote_ini (HWND owner)
   size_t ini_count = 0;
   GuiParseFeedback myFeedback;
   IniDBBuilderPackage aBuilder (myFeedback);
-  io_stream *ini_file, *ini_sig_file;
+  io_stream *ini_file = NULL, *ini_sig_file;
 
   /* FIXME: Get rid of this io_stream pointer travesty.  The need to
      explicitly delete these things is ridiculous. */
-- 
2.8.3

[PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Jon Turney]

Clicking on a button to cycle around alternatives seems bad UI, so instead allow 
a specific filter view to be directly selected with a pop-up menu.

I'm not sure if this is a great improvement or not, but since I wrote it, here 
it is...

Jon Turney (4):
  Build C++ code with -std=gnu++11
  Change PickView::view into an enum
  Rename PickView::Package to PickView::PackagePending
  Use a pop-up menu to directly select chooser view filter

 Makefile.am |   9 +++--
 PickView.cc |  52 ++++++++--------------------
 PickView.h  |  43 ++++++------------------
 choose.cc   | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 choose.h    |   1 +
 ini.cc      |   2 +-
 res.rc      |  22 ++++++++++--
 resource.h  |  10 ++++++
 8 files changed, 164 insertions(+), 85 deletions(-)

-- 
2.8.3

[PATCH setup 2/4] Change PickView::view into an enum

[Jon Turney]

Change PickView::view from a class, into an enum

Use a C++11 scoped enum so we don't have to change all the uses of it's
values.
---
 PickView.cc | 44 ++++++++++++--------------------------------
 PickView.h  | 41 +++++++++--------------------------------
 2 files changed, 21 insertions(+), 64 deletions(-)

diff --git a/PickView.cc b/PickView.cc
index c784a2a..588cfec 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -52,15 +52,6 @@ static PickView::Header cat_headers[] = {
   {0, 0, 0, false}
 };
 
-// PickView:: views
-const PickView::views PickView::views::Unknown (0);
-const PickView::views PickView::views::PackageFull (1);
-const PickView::views PickView::views::Package (2);
-const PickView::views PickView::views::PackageKeeps (3);
-const PickView::views PickView::views::PackageSkips (4);
-const PickView::views PickView::views::PackageUserPicked (5);
-const PickView::views PickView::views::Category (6);
-
 ATOM PickView::WindowClassAtom = 0;
 
 // DoInsertItem - inserts an item into a header control.
@@ -152,7 +143,11 @@ PickView::note_width (PickView::Header *hdrs, HDC dc,
 void
 PickView::cycleViewMode ()
 {
-  setViewMode (++view_mode);
+  PickView::views _value = (PickView::views)((int)view_mode + 1);
+  if (_value > PickView::views::Category)
+    _value = PickView::views::PackageFull;
+
+  setViewMode (_value);
 }
 
 void
@@ -235,25 +230,19 @@ PickView::setViewMode (views mode)
 const char *
 PickView::mode_caption ()
 {
-  return view_mode.caption ();
-}
-
-const char *
-PickView::views::caption ()
-{
-  switch (_value)
+  switch (view_mode)
     {
-    case 1:
+    case views::PackageFull:
       return "Full";
-    case 2:
+    case views::Package:
       return "Pending";
-    case 3:
+    case views::PackageKeeps:
       return "Up To Date";
-    case 4:
+    case views::PackageSkips:
       return "Not Installed";
-    case 5:
+    case views::PackageUserPicked:
       return "Picked";
-    case 6:
+    case views::Category:
       return "Category";
     default:
       return "";
@@ -348,15 +337,6 @@ PickView::insert_category (Category *cat, bool collapsed)
     delete &catline;
 }
 
-PickView::views&
-PickView::views::operator++ ()
-{
-  ++_value;
-  if (_value > Category._value)
-    _value = 1;
-  return *this;
-}
-
 int
 PickView::click (int row, int x)
 {
diff --git a/PickView.h b/PickView.h
index fd20dc9..054ab4a 100644
--- a/PickView.h
+++ b/PickView.h
@@ -40,7 +40,7 @@ class PickView : public Window
 public:
   virtual bool Create (Window * Parent = NULL, DWORD Style = WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CLIPCHILDREN, RECT * r = NULL);
   virtual bool registerWindowClass ();
-  class views;
+  enum class views;
   class Header;
   int num_columns;
   void defaultTrust (trusts trust);
@@ -96,38 +96,15 @@ public:
     return listheader;
   }
 
-  class views
+  enum class views
   {
-  public:
-    static const views Unknown;
-    static const views PackageFull;
-    static const views Package;
-    static const views PackageKeeps;
-    static const views PackageSkips;
-    static const views PackageUserPicked;
-    static const views Category;
-      views ():_value (0)
-    {
-    };
-    views (int aInt)
-    {
-      _value = aInt;
-      if (_value < 0 || _value > 6)
-	_value = 0;
-    }
-    views & operator++ ();
-    bool operator == (views const &rhs)
-    {
-      return _value == rhs._value;
-    }
-    bool operator != (views const &rhs)
-    {
-      return _value != rhs._value;
-    }
-    const char *caption ();
-
-  private:
-    int _value;
+    Unknown,
+    PackageFull,
+    Package,
+    PackageKeeps,
+    PackageSkips,
+    PackageUserPicked,
+    Category,
   };
 
   class Header
-- 
2.8.3

[PATCH setup 3/4] Rename PickView::Package to PickView::PackagePending

[Jon Turney]

Rename PickView::Package to PickView::PackagePending, as that's what that
view is
---
 PickView.cc | 8 ++++----
 PickView.h  | 2 +-
 choose.cc   | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/PickView.cc b/PickView.cc
index 588cfec..25d43a2 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -83,7 +83,7 @@ PickView::set_header_column_order (views vm)
 {
   if (vm == views::Unknown)
     return -1;
-  else if (vm == views::PackageFull || vm == views::Package
+  else if (vm == views::PackageFull || vm == views::PackagePending
            || vm == views::PackageKeeps || vm == views::PackageSkips
            || vm == views::PackageUserPicked)
     {
@@ -181,7 +181,7 @@ PickView::setViewMode (views mode)
               (view_mode == PickView::views::PackageFull)
 
               // "Pending" : packages that are being added/removed/upgraded
-              || (view_mode == PickView::views::Package &&
+              || (view_mode == PickView::views::PackagePending &&
                   ((!pkg.desired && pkg.installed) ||         // uninstall
                     (pkg.desired &&
                       (pkg.desired.picked () ||               // install bin
@@ -234,7 +234,7 @@ PickView::mode_caption ()
     {
     case views::PackageFull:
       return "Full";
-    case views::Package:
+    case views::PackagePending:
       return "Pending";
     case views::PackageKeeps:
       return "Up To Date";
@@ -815,7 +815,7 @@ PickView::WindowProc (UINT message, WPARAM wParam, LPARAM lParam)
                         lastWindowRect.width ()) != 0)
               {
                 cat_headers[set_header_column_order (views::Category)].width += dx;
-                pkg_headers[set_header_column_order (views::Package)].width += dx;
+                pkg_headers[set_header_column_order (views::PackagePending)].width += dx;
                 set_header_column_order (view_mode);
                 set_headers ();
                 ::MoveWindow (listheader, -scroll_ulc_x, 0,
diff --git a/PickView.h b/PickView.h
index 054ab4a..c07249b 100644
--- a/PickView.h
+++ b/PickView.h
@@ -100,7 +100,7 @@ public:
   {
     Unknown,
     PackageFull,
-    Package,
+    PackagePending,
     PackageKeeps,
     PackageSkips,
     PackageUserPicked,
diff --git a/choose.cc b/choose.cc
index 1c97f59..3c7f4f8 100644
--- a/choose.cc
+++ b/choose.cc
@@ -152,7 +152,7 @@ ChooserPage::createListview ()
   chooser->init(PickView::views::Category);
   chooser->Show(SW_SHOW);
   chooser->setViewMode (UpgradeAlsoOption || hasManualSelections ?
-			PickView::views::Package : PickView::views::Category);
+			PickView::views::PackagePending : PickView::views::Category);
   if (!SetDlgItemText (GetHWND (), IDC_CHOOSE_VIEWCAPTION, chooser->mode_caption ()))
     Log (LOG_BABBLE) << "Failed to set View button caption %ld" <<
 	 GetLastError () << endLog;
-- 
2.8.3

[PATCH setup 4/4] Use a pop-up menu to directly select chooser view filter

[Jon Turney]

Rather that cycling through views, popup a menu to choose the view when the
view button is clicked

Future work:

Hopefully this does reduce the average number of clicks needed, but
popping-up on mouse button down, rather than mouse button click is perhaps
what we really want here, but seems to be rather hard to achieve in a
propsheet.
---
 PickView.cc |  16 ++++-----
 PickView.h  |   2 +-
 choose.cc   | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 choose.h    |   1 +
 res.rc      |  22 +++++++++++--
 resource.h  |  10 ++++++
 6 files changed, 141 insertions(+), 18 deletions(-)

diff --git a/PickView.cc b/PickView.cc
index 25d43a2..3702b92 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -141,16 +141,6 @@ PickView::note_width (PickView::Header *hdrs, HDC dc,
 }
 
 void
-PickView::cycleViewMode ()
-{
-  PickView::views _value = (PickView::views)((int)view_mode + 1);
-  if (_value > PickView::views::Category)
-    _value = PickView::views::PackageFull;
-
-  setViewMode (_value);
-}
-
-void
 PickView::setViewMode (views mode)
 {
   view_mode = mode;
@@ -227,6 +217,12 @@ PickView::setViewMode (views mode)
   InvalidateRect (GetHWND(), &r, TRUE);
 }
 
+PickView::views
+PickView::getViewMode ()
+{
+  return view_mode;
+}
+
 const char *
 PickView::mode_caption ()
 {
diff --git a/PickView.h b/PickView.h
index c07249b..332383a 100644
--- a/PickView.h
+++ b/PickView.h
@@ -44,8 +44,8 @@ public:
   class Header;
   int num_columns;
   void defaultTrust (trusts trust);
-  void cycleViewMode ();
   void setViewMode (views mode);
+  views getViewMode ();
   void DrawIcon (HDC hdc, int x, int y, HANDLE hIcon);
   void paint (HWND hwnd);
   LRESULT CALLBACK list_click (HWND hwnd, BOOL dblclk, int x, int y, UINT hitCode);
diff --git a/choose.cc b/choose.cc
index 3c7f4f8..180c788 100644
--- a/choose.cc
+++ b/choose.cc
@@ -385,6 +385,10 @@ ChooserPage::changeTrust(trusts aTrust)
 bool
 ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 {
+#if DEBUG
+  Log (LOG_BABBLE) << "OnMesageCmd " << id << " " << hwndctl << " " << code << endLog;
+#endif
+
   if (code == EN_CHANGE && id == IDC_CHOOSE_SEARCH_EDIT)
     {
       SetTimer(GetHWND (), timer_id, SEARCH_TIMER_DELAY, (TIMERPROC) NULL);
@@ -423,11 +427,7 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
       break;
 
     case IDC_CHOOSE_VIEW:
-      chooser->cycleViewMode ();
-      if (!SetDlgItemText
-        (GetHWND (), IDC_CHOOSE_VIEWCAPTION, chooser->mode_caption ()))
-      Log (LOG_BABBLE) << "Failed to set View button caption " <<
-           GetLastError () << endLog;
+      selectView();
       break;
 
     case IDC_CHOOSE_HIDE:
@@ -442,6 +442,104 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
   return true;
 }
 
+static void
+setMenuItemState (HMENU hMenu, UINT item, UINT state)
+{
+  MENUITEMINFO mii;
+  memset(&mii, 0, sizeof(MENUITEMINFO));
+  mii.cbSize = sizeof(MENUITEMINFO);
+  mii.fMask = MIIM_STATE;
+  mii.fState = state;
+  SetMenuItemInfo (hMenu, item, FALSE, &mii);
+}
+
+void
+ChooserPage::selectView (void)
+{
+  HMENU hMenu = LoadMenu (GetModuleHandle (NULL), MAKEINTRESOURCE (IDM_CHOOSE_VIEW));
+  hMenu = GetSubMenu (hMenu, 0);
+
+  // mark the current view mode as selected
+  int item = 0;
+  PickView::views view_mode = chooser->getViewMode ();
+
+  switch (view_mode)
+    {
+    case PickView::views::PackageFull:
+      item = IDM_VIEW_FULL;
+      break;
+    case PickView::views::PackagePending:
+      item = IDM_VIEW_PENDING;
+      break;
+    case PickView::views::PackageKeeps:
+      item = IDM_VIEW_UPTODATE;
+      break;
+    case PickView::views::PackageSkips:
+      item = IDM_VIEW_NOT_INSTALLED;
+      break;
+    case PickView::views::PackageUserPicked:
+      item = IDM_VIEW_PICKED;
+      break;
+    case PickView::views::Category:
+      item = IDM_VIEW_CATEGORY;
+      break;
+    case PickView::views::Unknown:
+      item = 0;
+    }
+
+  if (item)
+    setMenuItemState (hMenu, item, MFS_CHECKED);
+
+  // place the menu over the 'view' button
+  HWND hButton = ::GetDlgItem (GetHWND (), IDC_CHOOSE_VIEW);
+  RECT rect;
+  ::GetWindowRect (hButton, &rect);
+
+  item = TrackPopupMenu (hMenu,
+                         TPM_LEFTALIGN | TPM_TOPALIGN | TPM_RETURNCMD | TPM_LEFTBUTTON | TPM_NOANIMATION,
+                         rect.left, rect.top,
+                         0, GetHWND (), NULL);
+
+  DestroyMenu (hMenu);
+
+#if DEBUG
+  Log (LOG_BABBLE) << "TrackPopupMenu returned " << item << endLog;
+#endif
+
+  // menu cancelled without making a selection
+  if (item == 0)
+    return;
+
+  // switch to the selected view
+  switch (item)
+    {
+    case IDM_VIEW_FULL:
+      view_mode = PickView::views::PackageFull;
+      break;
+    case IDM_VIEW_PENDING:
+      view_mode = PickView::views::PackagePending;
+      break;
+    case IDM_VIEW_UPTODATE:
+      view_mode = PickView::views::PackageKeeps;
+      break;
+    case IDM_VIEW_NOT_INSTALLED:
+      view_mode = PickView::views::PackageSkips;
+      break;
+    case IDM_VIEW_PICKED:
+      view_mode = PickView::views::PackageUserPicked;
+      break;
+    case IDM_VIEW_CATEGORY:
+      view_mode = PickView::views::Category;
+      break;
+    }
+
+  chooser->setViewMode (view_mode);
+  if (!SetDlgItemText
+      (GetHWND (), IDC_CHOOSE_VIEWCAPTION, chooser->mode_caption ()))
+    Log (LOG_BABBLE) << "Failed to set View button caption " <<
+      GetLastError () << endLog;
+}
+
 INT_PTR CALLBACK
 ChooserPage::OnMouseWheel (UINT message, WPARAM wParam, LPARAM lParam)
 {
diff --git a/choose.h b/choose.h
index 46f0f35..8832f5e 100644
--- a/choose.h
+++ b/choose.h
@@ -59,6 +59,7 @@ private:
   void logResults();
   void setPrompt(char const *aPrompt);
   void PlaceDialog (bool);
+  void selectView (void);
 
   PickView *chooser;
   static HWND ins_dialog;
diff --git a/res.rc b/res.rc
index 2fae133..10ec375 100644
--- a/res.rc
+++ b/res.rc
@@ -536,8 +536,8 @@ BEGIN
        "considered the most stable. (RECOMMENDED)"
     IDS_TRUSTEXP_TOOLTIP    "Globally select the most recent version, even if "
        "that version is considered Experimental or for test use by the maintainer."
-    IDS_VIEWBUTTON_TOOLTIP  "Cycles the package view.  This determines "
-       "which packages are shown in the chooser below.\r\n"
+    IDS_VIEWBUTTON_TOOLTIP  "Select the package view.  This determines "
+       "which packages are shown below.\r\n"
        "\r\n"
        "Category: Group by package category.  Click on '+' to expand.\r\n"
        "\r\n"
@@ -574,3 +574,21 @@ BEGIN
     IDS_ELEVATED       "Hand installation over to elevated child process."
     IDS_INSTALLEDB_VERSION "Unknown INSTALLED.DB version"
 END
+
+/////////////////////////////////////////////////////////////////////////////
+//
+// Menu
+//
+
+IDM_CHOOSE_VIEW MENUEX
+BEGIN
+    POPUP "ViewMenu"
+    BEGIN
+        MENUITEM "&Full",          IDM_VIEW_FULL,          MFT_STRING | MFT_RADIOCHECK
+        MENUITEM "&Pending",       IDM_VIEW_PENDING,       MFT_STRING | MFT_RADIOCHECK
+        MENUITEM "&Up To Date",    IDM_VIEW_UPTODATE,      MFT_STRING | MFT_RADIOCHECK
+        MENUITEM "&Not Installed", IDM_VIEW_NOT_INSTALLED, MFT_STRING | MFT_RADIOCHECK
+        MENUITEM "P&icked",        IDM_VIEW_PICKED,        MFT_STRING | MFT_RADIOCHECK
+        MENUITEM "&Category",      IDM_VIEW_CATEGORY,      MFT_STRING | MFT_RADIOCHECK
+    END
+END
diff --git a/resource.h b/resource.h
index 68e8023..e2312ac 100644
--- a/resource.h
+++ b/resource.h
@@ -175,3 +175,13 @@
 #define IDC_FILE_INUSE_EDIT               590
 #define IDC_FILE_INUSE_MSG                591
 #define IDC_FILE_INUSE_HELP               592
+
+// Menus
+
+#define IDM_CHOOSE_VIEW                   900
+#define IDM_VIEW_FULL                     901
+#define IDM_VIEW_PENDING                  902
+#define IDM_VIEW_UPTODATE                 903
+#define IDM_VIEW_NOT_INSTALLED            904
+#define IDM_VIEW_PICKED                   905
+#define IDM_VIEW_CATEGORY                 906
-- 
2.8.3

Re: [PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Corinna Vinschen]

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

On Aug 24 15:15, Jon Turney wrote:
> Clicking on a button to cycle around alternatives seems bad UI, so instead allow 
> a specific filter view to be directly selected with a pop-up menu.
> 
> I'm not sure if this is a great improvement or not, but since I wrote it, here 
> it is...

Nice idea and works fine in my limited testing.

One question: Usually the popup menu itself reflects the current choice.
So, wouldn't it make more sense to have the text "View" just as plain
text to the left, and the actual popup menu next right to it, showing
the current choice?  Yes?  No?  Shut up?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Jon Turney]

On 24/08/2016 17:50, Corinna Vinschen wrote:
> On Aug 24 15:15, Jon Turney wrote:
>> Clicking on a button to cycle around alternatives seems bad UI, so instead allow
>> a specific filter view to be directly selected with a pop-up menu.
>>
>> I'm not sure if this is a great improvement or not, but since I wrote it, here
>> it is...
>
> Nice idea and works fine in my limited testing.
>
> One question: Usually the popup menu itself reflects the current choice.
> So, wouldn't it make more sense to have the text "View" just as plain
> text to the left, and the actual popup menu next right to it, showing
> the current choice?  Yes?  No?  Shut up?

Yeah, there is probably better visual language to be used here.

If the popup menu itself reflects the current choice, perhaps we don't 
need a caption with the current choice at all (i.e. just a button that 
says "View" and that's all...)

I'd really like that button to have a 'â–¾' to indicate it has a 
drop-down, but that seems hard to achieve.

(This might be done with the BS_SPLITBUTTON style (which combines a 
button and dropdown menu), but that is sadly only supported by common 
controls on Vista or later.)

Re: [PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Yaakov Selkowitz]

On 2016-08-25 14:11, Jon Turney wrote:
> On 24/08/2016 17:50, Corinna Vinschen wrote:
>> One question: Usually the popup menu itself reflects the current choice.
>> So, wouldn't it make more sense to have the text "View" just as plain
>> text to the left, and the actual popup menu next right to it, showing
>> the current choice?  Yes?  No?  Shut up?
>
> Yeah, there is probably better visual language to be used here.
>
> If the popup menu itself reflects the current choice, perhaps we don't
> need a caption with the current choice at all (i.e. just a button that
> says "View" and that's all...)
>
> I'd really like that button to have a 'â–¾' to indicate it has a
> drop-down, but that seems hard to achieve.
>
> (This might be done with the BS_SPLITBUTTON style (which combines a
> button and dropdown menu), but that is sadly only supported by common
> controls on Vista or later.)

We're dropping Cygwin support for XP anyway, so why not from the 
installer as well?

-- 
Yaakov

Re: [PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Corinna Vinschen]

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

On Aug 25 15:34, Yaakov Selkowitz wrote:
> On 2016-08-25 14:11, Jon Turney wrote:
> > On 24/08/2016 17:50, Corinna Vinschen wrote:
> > > One question: Usually the popup menu itself reflects the current choice.
> > > So, wouldn't it make more sense to have the text "View" just as plain
> > > text to the left, and the actual popup menu next right to it, showing
> > > the current choice?  Yes?  No?  Shut up?
> > 
> > Yeah, there is probably better visual language to be used here.
> > 
> > If the popup menu itself reflects the current choice, perhaps we don't
> > need a caption with the current choice at all (i.e. just a button that
> > says "View" and that's all...)
> > 
> > I'd really like that button to have a '▾' to indicate it has a
> > drop-down, but that seems hard to achieve.
> > 
> > (This might be done with the BS_SPLITBUTTON style (which combines a
> > button and dropdown menu), but that is sadly only supported by common
> > controls on Vista or later.)
> 
> We're dropping Cygwin support for XP anyway, so why not from the installer
> as well?

ACK.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Jon Turney]

On 26/08/2016 09:07, Corinna Vinschen wrote:
> On Aug 25 15:34, Yaakov Selkowitz wrote:
>> On 2016-08-25 14:11, Jon Turney wrote:
>>> On 24/08/2016 17:50, Corinna Vinschen wrote:
>>>> One question: Usually the popup menu itself reflects the current choice.
>>>> So, wouldn't it make more sense to have the text "View" just as plain
>>>> text to the left, and the actual popup menu next right to it, showing
>>>> the current choice?  Yes?  No?  Shut up?
>>>
>>> Yeah, there is probably better visual language to be used here.
>>>
>>> If the popup menu itself reflects the current choice, perhaps we don't
>>> need a caption with the current choice at all (i.e. just a button that
>>> says "View" and that's all...)
>>>
>>> I'd really like that button to have a 'â–¾' to indicate it has a
>>> drop-down, but that seems hard to achieve.
>>>
>>> (This might be done with the BS_SPLITBUTTON style (which combines a
>>> button and dropdown menu), but that is sadly only supported by common
>>> controls on Vista or later.)

Two more patches follow this mail, which do that.

I'm still not totally happy with this.  It could give the impression 
that something is supposed to happen when the "View" part of the the 
button is clicked, and there is no keyboard shortcut to open the view 
selection menu.

>> We're dropping Cygwin support for XP anyway, so why not from the installer
>> as well?
>
> ACK.

I guess so, if a splitbutton is the right solution.

Should this be done using ld flags --major-os-version 6 
--minor-os-version 0?  Or some sort of runtime check?

[PATCH setup 5/6] Simplify view mode indication

[Jon Turney]

We don't really need a bit of text saying what the current view mode is,
when it's indicated in the pop-up menu, so remove that control.

This makes PickView::mode_caption() unused, so remove that as well.
---
 PickView.cc | 22 ----------------------
 PickView.h  |  1 -
 choose.cc   |  8 --------
 res.rc      |  6 +-----
 resource.h  |  1 -
 5 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/PickView.cc b/PickView.cc
index 3702b92..b079b08 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -223,28 +223,6 @@ PickView::getViewMode ()
   return view_mode;
 }
 
-const char *
-PickView::mode_caption ()
-{
-  switch (view_mode)
-    {
-    case views::PackageFull:
-      return "Full";
-    case views::PackagePending:
-      return "Pending";
-    case views::PackageKeeps:
-      return "Up To Date";
-    case views::PackageSkips:
-      return "Not Installed";
-    case views::PackageUserPicked:
-      return "Picked";
-    case views::Category:
-      return "Category";
-    default:
-      return "";
-    }
-}
-
 /* meant to be called on packagemeta::categories */
 bool
 isObsolete (set <std::string, casecompare_lt_op> &categories)
diff --git a/PickView.h b/PickView.h
index 332383a..fc4fbad 100644
--- a/PickView.h
+++ b/PickView.h
@@ -57,7 +57,6 @@ public:
   PickView (Category & cat);
   void init(views _mode);
   ~PickView();
-  const char *mode_caption ();
   void setObsolete (bool doit);
   void insert_pkg (packagemeta &);
   void insert_category (Category *, bool);
diff --git a/choose.cc b/choose.cc
index 180c788..19c1fc9 100644
--- a/choose.cc
+++ b/choose.cc
@@ -85,7 +85,6 @@ static ControlAdjuster::ControlInfo ChooserControlsInfo[] = {
   {IDC_CHOOSE_EXP, 		CP_RIGHT,   CP_TOP},
   {IDC_CHOOSE_VIEW, 		CP_LEFT,    CP_TOP},
   {IDC_LISTVIEW_POS, 		CP_RIGHT,   CP_TOP},
-  {IDC_CHOOSE_VIEWCAPTION,	CP_LEFT,    CP_TOP},
   {IDC_CHOOSE_LIST,		CP_STRETCH, CP_STRETCH},
   {IDC_CHOOSE_HIDE,             CP_LEFT,    CP_BOTTOM},
   {0, CP_LEFT, CP_TOP}
@@ -153,9 +152,6 @@ ChooserPage::createListview ()
   chooser->Show(SW_SHOW);
   chooser->setViewMode (UpgradeAlsoOption || hasManualSelections ?
 			PickView::views::PackagePending : PickView::views::Category);
-  if (!SetDlgItemText (GetHWND (), IDC_CHOOSE_VIEWCAPTION, chooser->mode_caption ()))
-    Log (LOG_BABBLE) << "Failed to set View button caption %ld" <<
-	 GetLastError () << endLog;
 
   /* FIXME: do we need to init the desired fields ? */
   static int ta[] = { IDC_CHOOSE_KEEP, IDC_CHOOSE_CURR, IDC_CHOOSE_EXP, 0 };
@@ -534,10 +530,6 @@ ChooserPage::selectView (void)
     }
 
   chooser->setViewMode (view_mode);
-  if (!SetDlgItemText
-      (GetHWND (), IDC_CHOOSE_VIEWCAPTION, chooser->mode_caption ()))
-    Log (LOG_BABBLE) << "Failed to set View button caption " <<
-      GetLastError () << endLog;
 }
 
 INT_PTR CALLBACK
diff --git a/res.rc b/res.rc
index 10ec375..0d124a6 100644
--- a/res.rc
+++ b/res.rc
@@ -319,9 +319,7 @@ END
 // Left-aligned controls.
 #define SETUP_VIEW_X		(7)
 #define SETUP_VIEW_W		(26)
-#define SETUP_VIEWCAP_X		(SETUP_VIEW_X + SETUP_VIEW_W +5)
-#define SETUP_VIEWCAP_W		(40)
-#define SETUP_SEARCH_X		(SETUP_VIEWCAP_X + SETUP_VIEWCAP_W + 80)
+#define SETUP_SEARCH_X		(SETUP_VIEW_X + SETUP_VIEW_W + 125)
 #define SETUP_SEARCH_W		(32)
 #define SETUP_SEARCHTEXT_X	(SETUP_SEARCH_X + SETUP_SEARCH_W + 2)
 #define SETUP_SEARCHTEXT_W	(60)
@@ -336,8 +334,6 @@ FONT 8, "MS Shell Dlg"
 BEGIN
     PUSHBUTTON      "&View", IDC_CHOOSE_VIEW, SETUP_VIEW_X, 30, SETUP_VIEW_W,
                     14, WS_EX_RIGHT
-    LTEXT           "", IDC_CHOOSE_VIEWCAPTION, SETUP_VIEWCAP_X, 33,
-                    SETUP_VIEWCAP_W, 10
     RTEXT           "&Search", IDC_STATIC, SETUP_SEARCH_X, 33, SETUP_SEARCH_W,
                     10, SS_CENTERIMAGE, WS_EX_RIGHT
     EDITTEXT        IDC_CHOOSE_SEARCH_EDIT, SETUP_SEARCHTEXT_X, 30,
diff --git a/resource.h b/resource.h
index e2312ac..91b5b52 100644
--- a/resource.h
+++ b/resource.h
@@ -132,7 +132,6 @@
 #define IDC_DLS_PPROGRESS_TEXT            544
 #define IDC_DLS_IPROGRESS_TEXT            545
 #define IDC_CHOOSE_INST_TEXT              546
-#define IDC_CHOOSE_VIEWCAPTION            547
 #define IDC_CHOOSE_LISTHEADER             548
 #define IDC_INS_BL_PACKAGE                549
 #define IDC_INS_BL_TOTAL                  550
-- 
2.8.3

[PATCH setup 6/6] Use a splitbutton to show the view choosing popup menu

[Jon Turney]

Pass WM_NOTIFY notifications for child controls of the property page through
to OnMessageCommand as well.

Future-proof RootPage::OnMessageCmd() to check the notification code.

Give IDC_CHOOSE_VIEW button the BS_SPLITBUTTON style, and handle the
BCN_DROPDOWN notification from that to show the view choosing menu.
---
 choose.cc   | 34 +++++++++++++++++++---------------
 proppage.cc | 10 +++++++---
 res.rc      |  6 +++---
 root.cc     |  3 +++
 4 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/choose.cc b/choose.cc
index 19c1fc9..ecfd5f1 100644
--- a/choose.cc
+++ b/choose.cc
@@ -385,17 +385,18 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
   Log (LOG_BABBLE) << "OnMesageCmd " << id << " " << hwndctl << " " << code << endLog;
 #endif
 
-  if (code == EN_CHANGE && id == IDC_CHOOSE_SEARCH_EDIT)
+  if (code == EN_CHANGE)
     {
-      SetTimer(GetHWND (), timer_id, SEARCH_TIMER_DELAY, (TIMERPROC) NULL);
-      return true;
-    }
-  else if (code != BN_CLICKED && code != EN_CHANGE)
-    {
-      // Not a click notification, we don't care.
-      return false;
+      if (id == IDC_CHOOSE_SEARCH_EDIT)
+        {
+          SetTimer(GetHWND (), timer_id, SEARCH_TIMER_DELAY, (TIMERPROC) NULL);
+          return true;
+        }
+      else
+        return false;
     }
-
+  else if (code == BN_CLICKED)
+  {
   switch (id)
     {
     case IDC_CHOOSE_CLEAR_SEARCH:
@@ -422,10 +423,6 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
         changeTrust (TRUST_TEST);
       break;
 
-    case IDC_CHOOSE_VIEW:
-      selectView();
-      break;
-
     case IDC_CHOOSE_HIDE:
       chooser->setObsolete (!IsButtonChecked (id));
       break;
@@ -433,9 +430,16 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
       // Wasn't recognized or handled.
       return false;
     }
-
-  // Was handled since we never got to default above.
   return true;
+  }
+  else if (code == BCN_DROPDOWN)
+    {
+      if (id == IDC_CHOOSE_VIEW)
+        selectView();
+    }
+
+  // we don't care.
+  return false;
 }
 
 static void
diff --git a/proppage.cc b/proppage.cc
index c03e5f7..31f439f 100644
--- a/proppage.cc
+++ b/proppage.cc
@@ -139,7 +139,9 @@ PropertyPage::DialogProc (UINT message, WPARAM wParam, LPARAM lParam)
           return TRUE;
         }
       case WM_NOTIFY:
-        switch (((NMHDR FAR *) lParam)->code)
+        {
+        NMHDR *pNmHdr = (NMHDR *) lParam;
+        switch (pNmHdr->code)
         {
           case PSN_APPLY:
             {
@@ -256,10 +258,12 @@ PropertyPage::DialogProc (UINT message, WPARAM wParam, LPARAM lParam)
             }
           default:
             {
-              // Unrecognized notification
-              return FALSE;
+              // Pass unrecognized notifications to WM_COMMAND handler
+              return OnMessageCmd (pNmHdr->idFrom, pNmHdr->hwndFrom,
+                                   pNmHdr->code);
             }
         }
+        }
         break;
       case WM_COMMAND:
         {
diff --git a/res.rc b/res.rc
index 0d124a6..2cdacfb 100644
--- a/res.rc
+++ b/res.rc
@@ -318,8 +318,8 @@ END
 
 // Left-aligned controls.
 #define SETUP_VIEW_X		(7)
-#define SETUP_VIEW_W		(26)
-#define SETUP_SEARCH_X		(SETUP_VIEW_X + SETUP_VIEW_W + 125)
+#define SETUP_VIEW_W		(33)
+#define SETUP_SEARCH_X		(SETUP_VIEW_X + SETUP_VIEW_W + 2)
 #define SETUP_SEARCH_W		(32)
 #define SETUP_SEARCHTEXT_X	(SETUP_SEARCH_X + SETUP_SEARCH_W + 2)
 #define SETUP_SEARCHTEXT_W	(60)
@@ -333,7 +333,7 @@ CAPTION "Cygwin Setup - Select Packages"
 FONT 8, "MS Shell Dlg"
 BEGIN
     PUSHBUTTON      "&View", IDC_CHOOSE_VIEW, SETUP_VIEW_X, 30, SETUP_VIEW_W,
-                    14, WS_EX_RIGHT
+                    14, BS_SPLITBUTTON
     RTEXT           "&Search", IDC_STATIC, SETUP_SEARCH_X, 33, SETUP_SEARCH_W,
                     10, SS_CENTERIMAGE, WS_EX_RIGHT
     EDITTEXT        IDC_CHOOSE_SEARCH_EDIT, SETUP_SEARCHTEXT_X, 30,
diff --git a/root.cc b/root.cc
index edf7a91..80f3162 100644
--- a/root.cc
+++ b/root.cc
@@ -227,6 +227,9 @@ directory_contains_wrong_version (HWND h)
 bool
 RootPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 {
+  if ((code != BN_CLICKED) && (code != EN_CHANGE))
+    return false;
+
   switch (id)
     {
 
-- 
2.8.3

Re: [PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Corinna Vinschen]

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

On Aug 26 15:41, Jon Turney wrote:
> On 26/08/2016 09:07, Corinna Vinschen wrote:
> > On Aug 25 15:34, Yaakov Selkowitz wrote:
> > > On 2016-08-25 14:11, Jon Turney wrote:
> > > > On 24/08/2016 17:50, Corinna Vinschen wrote:
> > > > > One question: Usually the popup menu itself reflects the current choice.
> > > > > So, wouldn't it make more sense to have the text "View" just as plain
> > > > > text to the left, and the actual popup menu next right to it, showing
> > > > > the current choice?  Yes?  No?  Shut up?
> > > > 
> > > > Yeah, there is probably better visual language to be used here.
> > > > 
> > > > If the popup menu itself reflects the current choice, perhaps we don't
> > > > need a caption with the current choice at all (i.e. just a button that
> > > > says "View" and that's all...)
> > > > 
> > > > I'd really like that button to have a '▾' to indicate it has a
> > > > drop-down, but that seems hard to achieve.
> > > > 
> > > > (This might be done with the BS_SPLITBUTTON style (which combines a
> > > > button and dropdown menu), but that is sadly only supported by common
> > > > controls on Vista or later.)
> 
> Two more patches follow this mail, which do that.
> 
> I'm still not totally happy with this.  It could give the impression that
> something is supposed to happen when the "View" part of the the button is
> clicked, and there is no keyboard shortcut to open the view selection menu.

Looks like I didn't explain well enough what I meant.  What I had in
mind was a pull-down menu just as you did, but which reflects the
current state in its text.  The word "View" would just show up in fixed
text field left of it.  I. e.:

         +--------------------+
   View: | Category       | v |
         +--------------------+

After the user chooses "Pending":

         +--------------------+
   View: | Pending        | v |
         +--------------------+

I think this is what most users expect from this pull-down menu style,
isn't it?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH setup 0/4] Use a pop-up menu to select chooser view filter

[Jon Turney]

On 26/08/2016 16:25, Corinna Vinschen wrote:
> On Aug 26 15:41, Jon Turney wrote:
>> On 26/08/2016 09:07, Corinna Vinschen wrote:
>>> On Aug 25 15:34, Yaakov Selkowitz wrote:
>>>> On 2016-08-25 14:11, Jon Turney wrote:
>>>>> On 24/08/2016 17:50, Corinna Vinschen wrote:
>>>>>> One question: Usually the popup menu itself reflects the current choice.
>>>>>> So, wouldn't it make more sense to have the text "View" just as plain
>>>>>> text to the left, and the actual popup menu next right to it, showing
>>>>>> the current choice?  Yes?  No?  Shut up?
>>>>>
>>>>> Yeah, there is probably better visual language to be used here.
>>>>>
>>>>> If the popup menu itself reflects the current choice, perhaps we don't
>>>>> need a caption with the current choice at all (i.e. just a button that
>>>>> says "View" and that's all...)
>>>>>
>>>>> I'd really like that button to have a 'â–¾' to indicate it has a
>>>>> drop-down, but that seems hard to achieve.
>>>>>
>>>>> (This might be done with the BS_SPLITBUTTON style (which combines a
>>>>> button and dropdown menu), but that is sadly only supported by common
>>>>> controls on Vista or later.)
>>
>> Two more patches follow this mail, which do that.
>>
>> I'm still not totally happy with this.  It could give the impression that
>> something is supposed to happen when the "View" part of the the button is
>> clicked, and there is no keyboard shortcut to open the view selection menu.
>
> Looks like I didn't explain well enough what I meant.  What I had in
> mind was a pull-down menu just as you did, but which reflects the
> current state in its text.  The word "View" would just show up in fixed
> text field left of it.  I. e.:
>
>          +--------------------+
>    View: | Category       | v |
>          +--------------------+
>
> After the user chooses "Pending":
>
>          +--------------------+
>    View: | Pending        | v |
>          +--------------------+
>
> I think this is what most users expect from this pull-down menu style,
> isn't it?

Ah, right.  Using a drop-down list is the correct thing to do here.

I had the idea in the back of my mind that in future it might be 
worthwhile to add other actions here (e.g. expand/collapse all in 
category view, unpacking the fixed filter set we support into the tests 
they are composed of, etc.), where a menu would be more appropriate, but 
I guess we can cross that bridge if we get to it...

See the replacement [4/4] to follow this mail.

[PATCH setup 4/4] Use a drop-down list to directly select chooser view filter

[Jon Turney]

Rather than a button for cycling through views, use a drop-down list to
choose the view

Remove not very useful PickView::views::Unknown enum value so that enum can
start from 0, so it can be used directly as a drop-down list index.

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 PickView.cc | 28 +++++++++++-----------------
 PickView.h  |  7 +++----
 choose.cc   | 52 ++++++++++++++++++++++++++++++++++------------------
 choose.h    |  1 +
 res.rc      | 20 ++++++++++----------
 5 files changed, 59 insertions(+), 49 deletions(-)

diff --git a/PickView.cc b/PickView.cc
index 25d43a2..fc6f8c2 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -81,11 +81,9 @@ DoInsertItem (HWND hwndHeader, int iInsertAfter, int nWidth, LPSTR lpsz)
 int
 PickView::set_header_column_order (views vm)
 {
-  if (vm == views::Unknown)
-    return -1;
-  else if (vm == views::PackageFull || vm == views::PackagePending
-           || vm == views::PackageKeeps || vm == views::PackageSkips
-           || vm == views::PackageUserPicked)
+  if (vm == views::PackageFull || vm == views::PackagePending
+      || vm == views::PackageKeeps || vm == views::PackageSkips
+      || vm == views::PackageUserPicked)
     {
       headers = pkg_headers;
       current_col = 0;
@@ -141,16 +139,6 @@ PickView::note_width (PickView::Header *hdrs, HDC dc,
 }
 
 void
-PickView::cycleViewMode ()
-{
-  PickView::views _value = (PickView::views)((int)view_mode + 1);
-  if (_value > PickView::views::Category)
-    _value = PickView::views::PackageFull;
-
-  setViewMode (_value);
-}
-
-void
 PickView::setViewMode (views mode)
 {
   view_mode = mode;
@@ -227,10 +215,16 @@ PickView::setViewMode (views mode)
   InvalidateRect (GetHWND(), &r, TRUE);
 }
 
+PickView::views
+PickView::getViewMode ()
+{
+  return view_mode;
+}
+
 const char *
-PickView::mode_caption ()
+PickView::mode_caption (views mode)
 {
-  switch (view_mode)
+  switch (mode)
     {
     case views::PackageFull:
       return "Full";
diff --git a/PickView.h b/PickView.h
index c07249b..298f844 100644
--- a/PickView.h
+++ b/PickView.h
@@ -44,8 +44,8 @@ public:
   class Header;
   int num_columns;
   void defaultTrust (trusts trust);
-  void cycleViewMode ();
   void setViewMode (views mode);
+  views getViewMode ();
   void DrawIcon (HDC hdc, int x, int y, HANDLE hIcon);
   void paint (HWND hwnd);
   LRESULT CALLBACK list_click (HWND hwnd, BOOL dblclk, int x, int y, UINT hitCode);
@@ -57,7 +57,7 @@ public:
   PickView (Category & cat);
   void init(views _mode);
   ~PickView();
-  const char *mode_caption ();
+  static const char *mode_caption (views mode);
   void setObsolete (bool doit);
   void insert_pkg (packagemeta &);
   void insert_category (Category *, bool);
@@ -98,8 +98,7 @@ public:
 
   enum class views
   {
-    Unknown,
-    PackageFull,
+    PackageFull = 0,
     PackagePending,
     PackageKeeps,
     PackageSkips,
diff --git a/choose.cc b/choose.cc
index 3c7f4f8..687addf 100644
--- a/choose.cc
+++ b/choose.cc
@@ -153,9 +153,7 @@ ChooserPage::createListview ()
   chooser->Show(SW_SHOW);
   chooser->setViewMode (UpgradeAlsoOption || hasManualSelections ?
 			PickView::views::PackagePending : PickView::views::Category);
-  if (!SetDlgItemText (GetHWND (), IDC_CHOOSE_VIEWCAPTION, chooser->mode_caption ()))
-    Log (LOG_BABBLE) << "Failed to set View button caption %ld" <<
-	 GetLastError () << endLog;
+  SendMessage (GetDlgItem (IDC_CHOOSE_VIEW), CB_SETCURSEL, (WPARAM)chooser->getViewMode(), 0);
 
   /* FIXME: do we need to init the desired fields ? */
   static int ta[] = { IDC_CHOOSE_KEEP, IDC_CHOOSE_CURR, IDC_CHOOSE_EXP, 0 };
@@ -241,6 +239,16 @@ ChooserPage::OnInit ()
 {
   CheckDlgButton (GetHWND (), IDC_CHOOSE_HIDE, BST_CHECKED);
 
+  /* Populate view dropdown list with choices */
+  HWND viewlist = GetDlgItem (IDC_CHOOSE_VIEW);
+  SendMessage (viewlist, CB_RESETCONTENT, 0, 0);
+  for (int view = (int)PickView::views::PackageFull;
+       view <= (int)PickView::views::Category;
+       view++)
+    {
+      SendMessage(viewlist, CB_ADDSTRING, 0, (LPARAM)PickView::mode_caption((PickView::views)view));
+    }
+
   SetBusy ();
   if (source == IDC_SOURCE_DOWNLOAD || source == IDC_SOURCE_LOCALDIR)
     packagemeta::ScanDownloadedFiles (MirrorOption);
@@ -385,17 +393,17 @@ ChooserPage::changeTrust(trusts aTrust)
 bool
 ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 {
+#if DEBUG
+  Log (LOG_BABBLE) << "OnMesageCmd " << id << " " << hwndctl << " " << code << endLog;
+#endif
+
   if (code == EN_CHANGE && id == IDC_CHOOSE_SEARCH_EDIT)
     {
       SetTimer(GetHWND (), timer_id, SEARCH_TIMER_DELAY, (TIMERPROC) NULL);
       return true;
     }
-  else if (code != BN_CLICKED && code != EN_CHANGE)
-    {
-      // Not a click notification, we don't care.
-      return false;
-    }
-
+  else if (code == BN_CLICKED)
+  {
   switch (id)
     {
     case IDC_CHOOSE_CLEAR_SEARCH:
@@ -422,14 +430,6 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
         changeTrust (TRUST_TEST);
       break;
 
-    case IDC_CHOOSE_VIEW:
-      chooser->cycleViewMode ();
-      if (!SetDlgItemText
-        (GetHWND (), IDC_CHOOSE_VIEWCAPTION, chooser->mode_caption ()))
-      Log (LOG_BABBLE) << "Failed to set View button caption " <<
-           GetLastError () << endLog;
-      break;
-
     case IDC_CHOOSE_HIDE:
       chooser->setObsolete (!IsButtonChecked (id));
       break;
@@ -437,9 +437,25 @@ ChooserPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
       // Wasn't recognized or handled.
       return false;
     }
-
   // Was handled since we never got to default above.
   return true;
+  }
+  else if (code == CBN_SELCHANGE)
+    {
+      if (id == IDC_CHOOSE_VIEW)
+        {
+          // switch to the selected view
+          LRESULT view_mode = SendMessage (GetDlgItem (IDC_CHOOSE_VIEW),
+                                           CB_GETCURSEL, 0, 0);
+          if (view_mode != CB_ERR)
+            chooser->setViewMode ((PickView::views)view_mode);
+
+          return true;
+        }
+    }
+
+  // We don't care.
+  return false;
 }
 
 INT_PTR CALLBACK
diff --git a/choose.h b/choose.h
index 46f0f35..8832f5e 100644
--- a/choose.h
+++ b/choose.h
@@ -59,6 +59,7 @@ private:
   void logResults();
   void setPrompt(char const *aPrompt);
   void PlaceDialog (bool);
+  void selectView (void);
 
   PickView *chooser;
   static HWND ins_dialog;
diff --git a/res.rc b/res.rc
index 2fae133..3ddf751 100644
--- a/res.rc
+++ b/res.rc
@@ -318,10 +318,10 @@ END
 
 // Left-aligned controls.
 #define SETUP_VIEW_X		(7)
-#define SETUP_VIEW_W		(26)
-#define SETUP_VIEWCAP_X		(SETUP_VIEW_X + SETUP_VIEW_W +5)
-#define SETUP_VIEWCAP_W		(40)
-#define SETUP_SEARCH_X		(SETUP_VIEWCAP_X + SETUP_VIEWCAP_W + 80)
+#define SETUP_VIEW_W		(20)
+#define SETUP_VIEWLIST_X		(SETUP_VIEW_X + SETUP_VIEW_W + 2)
+#define SETUP_VIEWLIST_W		(60)
+#define SETUP_SEARCH_X		(SETUP_VIEWLIST_X + SETUP_VIEWLIST_W + 2)
 #define SETUP_SEARCH_W		(32)
 #define SETUP_SEARCHTEXT_X	(SETUP_SEARCH_X + SETUP_SEARCH_W + 2)
 #define SETUP_SEARCHTEXT_W	(60)
@@ -334,10 +334,10 @@ STYLE DS_MODALFRAME | DS_3DLOOK | WS_CHILD | WS_VISIBLE | WS_CAPTION |
 CAPTION "Cygwin Setup - Select Packages"
 FONT 8, "MS Shell Dlg"
 BEGIN
-    PUSHBUTTON      "&View", IDC_CHOOSE_VIEW, SETUP_VIEW_X, 30, SETUP_VIEW_W,
-                    14, WS_EX_RIGHT
-    LTEXT           "", IDC_CHOOSE_VIEWCAPTION, SETUP_VIEWCAP_X, 33,
-                    SETUP_VIEWCAP_W, 10
+    LTEXT           "View:", IDC_CHOOSE_VIEWCAPTION, SETUP_VIEW_X, 33,
+                    SETUP_VIEW_W, 10
+    COMBOBOX        IDC_CHOOSE_VIEW, SETUP_VIEWLIST_X, 30, SETUP_VIEWLIST_W, 84,
+                    CBS_DROPDOWNLIST
     RTEXT           "&Search", IDC_STATIC, SETUP_SEARCH_X, 33, SETUP_SEARCH_W,
                     10, SS_CENTERIMAGE, WS_EX_RIGHT
     EDITTEXT        IDC_CHOOSE_SEARCH_EDIT, SETUP_SEARCHTEXT_X, 30,
@@ -536,8 +536,8 @@ BEGIN
        "considered the most stable. (RECOMMENDED)"
     IDS_TRUSTEXP_TOOLTIP    "Globally select the most recent version, even if "
        "that version is considered Experimental or for test use by the maintainer."
-    IDS_VIEWBUTTON_TOOLTIP  "Cycles the package view.  This determines "
-       "which packages are shown in the chooser below.\r\n"
+    IDS_VIEWBUTTON_TOOLTIP  "Select the package view.  This determines "
+       "which packages are shown below.\r\n"
        "\r\n"
        "Category: Group by package category.  Click on '+' to expand.\r\n"
        "\r\n"
-- 
2.8.3

Re: [PATCH setup 4/4] Use a drop-down list to directly select chooser view filter

[Corinna Vinschen]

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

On Aug 29 11:04, Jon Turney wrote:
> Rather than a button for cycling through views, use a drop-down list to
> choose the view
> 
> Remove not very useful PickView::views::Unknown enum value so that enum can
> start from 0, so it can be used directly as a drop-down list index.

Almost Perfect.  Please apply, without the colon after "View" (compare
with "Search").

Btw.

What still bugs me is that the static text elements do not vertically
align with the texts in the input elements.  I. e., the word "View" is a
dot or two lower than, say, the word "Category" in the drop down menu.
Same for "Search" and the input field.  Can we fix this easily?  Any
such patch is pre-approved.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH setup 4/4] Use a drop-down list to directly select chooser view filter

[Jon Turney]

On 30/08/2016 14:01, Corinna Vinschen wrote:
> On Aug 29 11:04, Jon Turney wrote:
>> Rather than a button for cycling through views, use a drop-down list to
>> choose the view
>>
>> Remove not very useful PickView::views::Unknown enum value so that enum can
>> start from 0, so it can be used directly as a drop-down list index.
>
> Almost Perfect.  Please apply, without the colon after "View" (compare
> with "Search").

Done.

> What still bugs me is that the static text elements do not vertically
> align with the texts in the input elements.  I. e., the word "View" is a
> dot or two lower than, say, the word "Category" in the drop down menu.
> Same for "Search" and the input field.  Can we fix this easily?  Any
> such patch is pre-approved.

I don't quite understand why the text doesn't look centered on the 
control at the moment.

The controls are the standard 14 DLUs high, 1 vertical DLU = 1/8th of 
the average character height, and (14 - 8)/2 = 3 DLUs is used as the 
offset between the top of the control and the top of the label.