Patch: Setup.exe - search for package

[Dave Korn]

Dave Korn wrote:
> Andrew Punch wrote:
>> Hi,
>>
>> I have attached a patch for searching packages in the package selection
>> screen. The patch is against version 2.573.2.3 - I couldn't get the CVS
>> head to build due to a libtool version problem.
> 
>   Thanks for contributing this.  I'm currently up-porting it to CVS HEAD and
> giving it some testing.  None of the other maintainers seem to have any
> objections, so I'll commit it once I'm sure it works right.  (Your ChangeLog
> entry isn't in the standard format but I'll fix that up for you.)

  Ok, it looks pretty good.  Functionality wise, I have a couple of thoughts:
I think the search box needs a label attached and a tool-tip, and IMO I think
it might look nicer if it was left-aligned away from the view controls; what
do you (and everyone else) think?  Secondly, every time you change the
search-box contents in the category view, it collapses all expanded
categories; I'd like to avoid that if we can, but haven't yet looked to see how.

  Also, a few stylistic issues to bear in mind for any potential future
occasion you're submitting a patch (don't worry about them now, I'll tidy it
up as part of porting it to HEAD):

- Standard system header includes should go at the top of lists of #includes,
not the end:

--- setup-2.573.2.3/PickView.h	2006-05-24 23:01:34.000000000 +1000
+++ setup-new/PickView.h	2009-03-31 21:14:00.375000000 +1100
@@ -19,6 +19,7 @@
 #include "win32.h"
 #include "window.h"
 #include "RECTWrapper.h"
+#include <string>

 #define HMARGIN         10
 #define ROW_MARGIN      5
@@ -82,6 +83,13 @@

- There should be a space between any function name and the following
open-bracket:

@@ -82,6 +83,13 @@
   int header_height;
   PickCategoryLine contents;
   void scroll (HWND hwnd, int which, int *var, int code, int howmany);
+
+  void SetPackageFilter(const std::string &filterString)
+  {
+	packageFilterString = filterString;
+  }
+
+

- Watch out for accidental white-space and other superfluous or unintentional
changes; e.g.

diff -u --exclude '*config.*' --exclude '*Makefile' setup-2.573.2.3/res.rc
setup-new/res.rc
--- setup-2.573.2.3/res.rc	2008-06-19 09:26:20.000000000 +1000
+++ setup-new/res.rc	2009-03-31 21:14:00.390625000 +1100
@@ -316,7 +316,8 @@
 CAPTION "Cygwin Setup - Select Packages"
 FONT 8, "MS Shell Dlg"
 BEGIN
-    CONTROL         "&Keep",IDC_CHOOSE_KEEP,"Button",BS_AUTORADIOBUTTON |
+    EDITTEXT		IDC_CHOOSE_SEARCH_EDIT, 7, 30, 60, 12
+	CONTROL         "&Keep",IDC_CHOOSE_KEEP,"Button",BS_AUTORADIOBUTTON |

here you replaced the spaces before CONTROL with a TAB, or here:

@@ -291,8 +300,9 @@
     case IDC_CHOOSE_HIDE:
       chooser->setObsolete (!IsButtonChecked (id));
       break;
+	
+
     default:
-      // Wasn't recognized or handled.
       return false;
     }

where you added some blank lines and deleted a comment; neither of these two
changes are related to or required for the purpose of your patch in any way.

@@ -283,20 +288,20 @@
     }
   else
     {
-      for (set <std::string, casecompare_lt_op>::const_iterator x
+	  for (set <std::string, casecompare_lt_op>::const_iterator x
 	   = pkg.categories.begin (); x != pkg.categories.end (); ++x)
-        {
+		{
 	  // Special case - yuck
 	  if (casecompare(*x, "All") == 0)
-	    continue;
+		continue;

 	  packagedb db;
 	  PickCategoryLine & catline =
-	    *new PickCategoryLine (*this, *db.categories.find (*x), 1);
+		*new PickCategoryLine (*this, *db.categories.find (*x), 1);
 	  PickLine & line = *new PickPackageLine(*this, pkg);
 	  catline.insert (line);
 	  contents.insert (catline);
-        }
+		}
     }
 }

.. or here where you've just gone and added incorrect indentation without
making any changes at all.  You should always match the existing indentation
style of any code you're working with; if it uses TABs, use TABs, if it uses
spaces, use spaces.  And above all, watch out if your editor is set to
auto-convert between the two!

  The other thing I wanted to mention is that your ChangeLog entry isn't in
the standard format.  Minor: two spaces between the date and your name, and
your name and your email address.  More significant: we don't put introductory
paragraphs in ChangeLogs, it's not what they're for.  Minor again: each line
(apart from the header with date/name/email) should begin with a TAB; you
should only specify the filename once for each file modified, on the first
line; we use the present tense for verbs; and there are some standard ways of
denoting things like the addition of new functions.  A sentence like "Add
definition of function SetPackageFilter to class PickView" contains a lot of
redundancy when you've already given the class and function names in brackets
immediately beforehand; anyone who understands C++ will already know that.

  So I'd rewrite it like so (email protected just for illustration):

2009-03-27  Andrew Punch  <andrew@xxxxxxxxxxxxx.xxx.xx>

	* PickView.h:  Add #include <string>.
	(PickView::SetPackageFilter):  Add new function.
	(PickView::packageFilterString):  Add new string data member.
	* PickView.cc (PickView::setViewMode):  Use it to filter names.
	(PickView::insert_category):  Likewise.
	(PickView::PickView):  Initialise packageFilterString to blank.
	* res.rc (IDC_CHOOSE_SEARCH_EDIT):  Add new edit control.
	* resource.h (IDC_CHOOSE_SEARCH_EDIT):  Add define for control ID.
	* choose.cc (ChooserControlsInfo):  Add IDC_CHOOSE_SEARCH_EDIT
	control to auto-resize list.
	(ChooserPage::OnMessageCmd):  Handle EN_CHANGE event from
	IDC_CHOOSE_SEARCH_EDIT.


  For more info on the style we use, you can browse through the GNU coding
standards, available online at:

    http://www.gnu.org/prep/standards/

  And finally: Thank you again!  This is definitely a really nice feature!

    cheers,
      DaveK