X-Recipient: archive-cygwin AT delorie DOT com
X-Spam-Check-By: sourceware.org
To: cygwin AT cygwin DOT com
From: Lewis Hyatt <lhyatt AT princeton DOT edu>
Subject:  Re: setup.exe suggestion + patch
Date:  Thu, 13 Sep 2007 14:48:41 -0400
Lines: 438
Message-ID: <fcc0m0$lcd$1@sea.gmane.org>
References:  <fbpk5f$7v7$1 AT sea DOT gmane DOT org> <Pine DOT GSO DOT 4 DOT 63 DOT 0709131418150 DOT 8875 AT access1 DOT cims DOT nyu DOT edu>
Mime-Version:  1.0
Content-Type:  multipart/mixed;  boundary="------------000708010204010504070101"
User-Agent: Thunderbird 2.0.0.6 (Windows/20070728)
In-Reply-To: <Pine.GSO.4.63.0709131418150.8875@access1.cims.nyu.edu>
X-IsSubscribed: yes
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Precedence: bulk
List-Id: <cygwin.cygwin.com>
List-Unsubscribe: <mailto:cygwin-unsubscribe-archive-cygwin=delorie DOT com AT cygwin DOT com>
List-Subscribe: <mailto:cygwin-subscribe AT cygwin DOT com>
List-Archive: <http://sourceware.org/ml/cygwin/>
List-Post: <mailto:cygwin AT cygwin DOT com>
List-Help: <mailto:cygwin-help AT cygwin DOT com>, <http://sourceware.org/ml/#faqs>
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
Delivered-To: mailing list cygwin AT cygwin DOT com

--------------000708010204010504070101
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Igor Peshansky wrote:


> Third, I have to apologize -- I've had a partial reply to your message
> sitting in my drafts since the day you sent it, but got bogged down.

OK, I understand of course, I just wanted to check. Thanks for getting 
back to me Igor and Dave!

> A few comments on the patch:
> 1) It would be great if you used "diff -up" -- unified diffs are so much
> easier to read.

I attached the output of cvs diff -up. I also attached the two new files 
PopupMenu.{cc,h}, which I didn't realize were not included in the diff 
already.

> 2) Is there a reason you use popup menus, rather than pull-down lists?

I think pull-down lists would probably be nicer and more intuitive. 
Adding this is a much larger change, though, because you have to change 
the PickLine classes to paint the controls, and you have to modify the 
window procedures, etc. I can do all that, but wanted to gauge interest 
first. The drop-down menu way only takes a couple lines, and doesn't 
require dealing with the window procedure, so I thought it was a good 
way to start anyway. Note that my implementation of 
packagemeta::select_action(), which builds the menu, is also admittedly 
pretty quick and dirty. It's clear how to improve it, but it works fine 
the way it is and re-uses the already-debugged packagemeta::set_action() 
code.

-Lewis

--------------000708010204010504070101
Content-Type: text/plain;
 name="PopupMenu.cc"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="PopupMenu.cc"

/*
 * Copyright (c) 2002 Lewis Hyatt.
 *
 *     This program is free software; you can redistribute it and/or modify
 *     it under the terms of the GNU General Public License as published by
 *     the Free Software Foundation; either version 2 of the License, or
 *     (at your option) any later version.
 *
 *     A copy of the GNU General Public License can be found at
 *     http://www.gnu.org/
 *
 * Written by Lewis Hyatt <lhyatt AT princeton DOT edu>
 *
 */

#include "PopupMenu.h"
#include "Exception.h"
#include "resource.h"
#include "String++.h"
using namespace std;

void PopupMenu::init_menu(char const* const* const items, size_t const num_items)
{
	hMenu=0;
	static size_t const max_items = IDM_POPUP_LAST - IDM_POPUP_FIRST + 1;
	if(num_items > max_items) throw new Exception(TOSTRING (__LINE__) " " __FILE__, "Too many popup menu items");
	
	hMenu = CreatePopupMenu();
	for(size_t i=0; i!=num_items; ++i)
	  	AppendMenu(hMenu, MF_STRING, IDM_POPUP_FIRST+i, items[i]);
}

void PopupMenu::init_menu(string const* const items, size_t const num_items)
{
	vector<char const*> items_c(num_items);
	for(size_t i=0; i!=num_items; ++i) items_c[i] = items[i].c_str();
	init_menu(&items_c[0], num_items);
}

int PopupMenu::get_selection(int x, int y) const {
	if(x==position_undefined || y==position_undefined)	{
		POINT point;
		GetCursorPos(&point);
		if(x==position_undefined) x=point.x;
		if(y==position_undefined) y=point.y;
	}
	
	int const result = TrackPopupMenuEx(
		hMenu,
		TPM_CENTERALIGN | TPM_TOPALIGN | TPM_NONOTIFY | TPM_RETURNCMD, 
		x, y,
		GetActiveWindow(),
		0
	);
	
	return result==0 ? -1 : result - IDM_POPUP_FIRST;
}

--------------000708010204010504070101
Content-Type: text/plain;
 name="PopupMenu.h"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="PopupMenu.h"

/*
 * Copyright (c) 2007 Lewis Hyatt.
 *
 *     This program is free software; you can redistribute it and/or modify
 *     it under the terms of the GNU General Public License as published by
 *     the Free Software Foundation; either version 2 of the License, or
 *     (at your option) any later version.
 *
 *     A copy of the GNU General Public License can be found at
 *     http://www.gnu.org/
 *
 * Written by Lewis Hyatt <lhyatt AT princeton DOT edu>
 *
 */
 
//the purpose of this class is to create a popup menu on the fly from a list of strings.
//the menu is displayed and then the index of the selected string is returned, or -1 if
//no string was selected. this class needs a block of IDs reserved for the menu items,
//which is defined by IDM_POPUP_FIRST and IDM_POPUP_LAST in resource.h.
 

#ifndef SETUP_POPUPMENU_H
#define SETUP_POPUPMENU_H

#include "win32.h"
#include <string>
#include <vector>

class PopupMenu	{
	HMENU hMenu;
	
	//make non-copyable for simplicity
	PopupMenu(PopupMenu const&);
	PopupMenu& operator=(PopupMenu const*);
	
	//the init function is private because it is only called from the constructor
	void init_menu(char const* const* items, size_t num_items);
	void init_menu(std::string const* items, size_t num_items);
public:

	//constructor creates the menu but does not show it. there are a few overloads so you can pass an
	//array of char*, an array of string, or a vector of string.
	
	PopupMenu(char const* const* items, size_t num_items)
	{
		init_menu(items, num_items);
	}
	
	PopupMenu(std::string const* items, size_t num_items)
	{
		init_menu(items, num_items);
	}
	
	explicit PopupMenu(std::vector<std::string> const& items)
	{
		init_menu(&items[0], items.size());
	}
	
	//get_selection shows the menu and returns the index of the selected item.
	//the x and y parameters let you pick where to show the menu; leave them
	//undefined to show at the current cursor location.
	
	enum {position_undefined = -1};
	int get_selection(int x=position_undefined, int y=position_undefined) const;
	
	//destructor frees the menu
	~PopupMenu()
	{
		DestroyMenu(hMenu);
	}
};

#endif /* SETUP_POPUPMENU_H */

--------------000708010204010504070101
Content-Type: text/plain;
 name="diff_up.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="diff_up.txt"

? setup/.deps
? setup/.libs
? setup/Makefile
? setup/PopupMenu.cc
? setup/PopupMenu.h
? setup/config.cache
? setup/config.log
? setup/config.status
? setup/inilex.cc
? setup/iniparse.cc
? setup/iniparse.h
? setup/libtool
? setup/setup_version.c
? setup/csu_util/.deps
? setup/csu_util/.dirstamp
? setup/libgetopt++/.libs
? setup/libgetopt++/Makefile
? setup/libgetopt++/config.log
? setup/libgetopt++/config.status
? setup/libgetopt++/libgetopt++.la
? setup/libgetopt++/libtool
? setup/libgetopt++/include/autoconf.h
? setup/libgetopt++/include/stamp-h1
? setup/libgetopt++/src/.deps
? setup/libgetopt++/src/.dirstamp
? setup/libgetopt++/src/BoolOption.lo
? setup/libgetopt++/src/GetOption.lo
? setup/libgetopt++/src/Option.lo
? setup/libgetopt++/src/OptionSet.lo
? setup/libgetopt++/src/StringOption.lo
? setup/libgetopt++/tests/.deps
? setup/libmd5-rfc/.deps
? setup/libmd5-rfc/.dirstamp
? setup/tests/.deps
? setup/tests/Makefile
Index: setup/Makefile.am
===================================================================
RCS file: /cvs/cygwin-apps/setup/Makefile.am,v
retrieving revision 2.68
diff -u -p -r2.68 Makefile.am
--- setup/Makefile.am	30 Jul 2007 22:55:49 -0000	2.68
+++ setup/Makefile.am	13 Sep 2007 18:37:32 -0000
@@ -228,6 +228,8 @@ setup_SOURCES = \
 	PickPackageLine.h \
 	PickView.cc \
 	PickView.h \
+	PopupMenu.h \
+	PopupMenu.cc \
 	postinstall.cc \
 	prereq.cc \
 	prereq.h \
Index: setup/PickCategoryLine.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/PickCategoryLine.cc,v
retrieving revision 2.10
diff -u -p -r2.10 PickCategoryLine.cc
--- setup/PickCategoryLine.cc	24 May 2006 13:01:34 -0000	2.10
+++ setup/PickCategoryLine.cc	13 Sep 2007 18:37:32 -0000
@@ -16,6 +16,7 @@
 #include "PickCategoryLine.h"
 #include "package_db.h"
 #include "PickView.h"
+#include "PopupMenu.h"
 
 void
 PickCategoryLine::empty (void)
@@ -110,11 +111,12 @@ PickCategoryLine::click (int const myrow
     {
       if ((size_t) x >= spin_x)
 	{
-	  ++current_default;
-	  
-          packagedb().markUnVisited();
-
-	  return set_action (current_default);
+	  //create a popup menu for the user to select the desired action
+	  PopupMenu menu(packagemeta::_actions::captions, packagemeta::_actions::num_actions);
+	  int const selection = menu.get_selection();
+	  if(selection<0) return 0;	  
+	  packagedb().markUnVisited();
+	  return set_action(selection);
 	}
       else
 	{
Index: setup/PickPackageLine.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/PickPackageLine.cc,v
retrieving revision 2.21
diff -u -p -r2.21 PickPackageLine.cc
--- setup/PickPackageLine.cc	24 May 2006 13:01:34 -0000	2.21
+++ setup/PickPackageLine.cc	13 Sep 2007 18:37:32 -0000
@@ -134,7 +134,10 @@ PickPackageLine::click (int const myrow,
 
   if (x >= theView.headers[theView.new_col].x - HMARGIN / 2
       && x <= theView.headers[theView.new_col + 1].x - HMARGIN / 2)
-    pkg.set_action (pkg.trustp(theView.deftrust));
+  {
+    	//pkg.set_action (pkg.trustp(theView.deftrust));
+    	pkg.select_action(pkg.trustp(theView.deftrust));
+  }
 
   packagedb().markUnVisited();
   /* Add any packages that are needed by this package */
Index: setup/package_meta.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_meta.cc,v
retrieving revision 2.52
diff -u -p -r2.52 package_meta.cc
--- setup/package_meta.cc	17 Apr 2006 16:13:17 -0000	2.52
+++ setup/package_meta.cc	13 Sep 2007 18:37:33 -0000
@@ -18,6 +18,7 @@ static const char *cvsid = "\n%%% $Id: p
 #endif
 
 #include "package_meta.h"
+#include "PopupMenu.h"
 
 #include <string>
 #include <set>
@@ -65,23 +66,14 @@ const
   packagemeta::_actions
 packagemeta::Uninstall_action (3);
 
-char const *
-packagemeta::_actions::caption ()
-{
-  switch (_value)
-    {
-    case 0:
-      return "Default";
-    case 1:
-      return "Install";
-    case 2:
-      return "Reinstall";
-    case 3:
-      return "Uninstall";
-    }
-  // Pacify GCC: (all case options are checked above)
-  return 0;
-}
+//action caption definitions
+//TODO: this could be grabbed from the resource file instead
+char const* const packagemeta::_actions::captions[packagemeta::_actions::num_actions] = {
+	"Default",
+	"Install",
+	"Reinstall",
+	"Uninstall"
+};
 
 packagemeta::packagemeta (packagemeta const &rhs) :
   name (rhs.name), key (rhs.name), installed_from (), 
@@ -99,7 +91,7 @@ packagemeta::packagemeta (packagemeta co
 packagemeta::_actions & packagemeta::_actions::operator++ ()
 {
   ++_value;
-  if (_value > 3)
+  if (_value >= num_actions)
     _value = 0;
   return *this;
 }
@@ -410,6 +402,30 @@ packagemeta::set_action (packageversion 
     }
 }
 
+void packagemeta::select_action(packageversion const& default_version)	{
+	vector<string> actions;
+	
+	//in order to avoid duplicating all of the logic in set_action() above, we will instead just call set_action repeatedly
+	//to see what it would have done, and then put all of that in the menu.
+	//this is a little silly, but the assumption is that set_action() is fast, so it isn't such a waste to just call it once for
+	//all actions, and then call it again for the one the user actually wanted.
+	//this is quick and dirty, a better solution would be to get rid of set_action, and just reformulate its logic into this function.
+	//I am willing to do that, if there is agreement that this is an improvement over the old way of just cycling through. -Lewis
+	
+	string next_action=action_caption();
+	do	{
+		actions.push_back(next_action);
+		set_action(default_version);
+		next_action = action_caption();
+	} while(next_action != actions.front());
+	
+	PopupMenu menu(actions);
+	int const selection = menu.get_selection();
+	
+	//again we use a silly inefficient for loop so we don't have to think about what all set_action was actually doing.
+	for(int i=0; i<selection; ++i) set_action(default_version); //note case i<0 (no selection) is handled correctly.
+}
+
 int
 packagemeta::set_requirements (trusts deftrust, size_t depth)
 {
Index: setup/package_meta.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_meta.h,v
retrieving revision 2.36
diff -u -p -r2.36 package_meta.h
--- setup/package_meta.h	17 Apr 2006 16:13:17 -0000	2.36
+++ setup/package_meta.h	13 Sep 2007 18:37:33 -0000
@@ -60,16 +60,19 @@ public:
   class _actions
   {
   public:
+  	enum {num_actions = 4};
+  	static char const* const captions[num_actions];
     _actions ():_value (0) {};
     _actions (int aInt) {
-    _value = aInt;
-    if (_value < 0 ||  _value > 3)
+    	_value = aInt;
+       if (_value < 0 ||  _value >= num_actions)
       _value = 0;
     }
     _actions & operator ++ ();
     bool operator == (_actions const &rhs) { return _value == rhs._value; }
     bool operator != (_actions const &rhs) { return _value != rhs._value; }
-    const char *caption ();
+    const char *caption() const {return caption(_value);}
+    static const char* caption(int const value) {return captions[value];}
   private:
     int _value;
   };
@@ -78,6 +81,7 @@ public:
   static const _actions Reinstall_action;
   static const _actions Uninstall_action;
   void set_action (packageversion const &default_version);
+  void select_action(packageversion const &default_version);
   void set_action (_actions, packageversion const & default_version);
   void uninstall ();
   int set_requirements (trusts deftrust, size_t depth);
Index: setup/resource.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/resource.h,v
retrieving revision 2.35
diff -u -p -r2.35 resource.h
--- setup/resource.h	4 May 2007 21:56:53 -0000	2.35
+++ setup/resource.h	13 Sep 2007 18:37:35 -0000
@@ -159,3 +159,9 @@
 #define IDC_STATUS_HEADER                 582
 #define IDC_STATUS                        583
 #define IDC_STATIC_HEADER                 584
+
+//popup menus
+
+//note: numbers 600-699 are reserved for popup menus; see PopupMenu.h
+#define IDM_POPUP_FIRST			600
+#define IDM_POPUP_LAST			699


--------------000708010204010504070101
Content-Type: text/plain; charset=us-ascii

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/
--------------000708010204010504070101--