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:  setup.exe suggestion + patch
Date:  Thu, 06 Sep 2007 15:24:34 -0400
Lines: 410
Message-ID: <fbpk5f$7v7$1@sea.gmane.org>
Mime-Version:  1.0
Content-Type:  multipart/mixed;  boundary="------------000105000800090107080106"
User-Agent: Thunderbird 2.0.0.6 (Windows/20070728)
X-IsSubscribed: yes
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
List-Id: <cygwin.cygwin.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

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

Hello-

Firstly, thanks to everyone who has worked on setup.exe, it's really a 
very convenient program! There is just one thing that has always 
bothered me, which is that you have to click repeatedly on the package 
or category to cycle through all the available actions to find the one 
you want. The main problem is that each click causes the dependencies to 
be recalculated, which can cause annoying slowdowns if you're trying to 
do something like uninstall all packages in a large category. There is 
also the following situation which occurs often, especially when you are 
playing around with installing and uninstalling new packages:

-package A requires package B
-package A has two available versions
-package B appears before package A in the list

Now suppose A and B are both installed, and you want to uninstall them. 
Since B appears first, you click through to uninstall, no problem. Now 
you scroll down, maybe several pages away, and try to uninstall package 
A. The first time you click, though, you end up on the Prev version, 
which then calculates that it needs package A and goes back and sets 
package A to Install again. The only way to uninstall both of them is to 
uninstall B first, and then A. When there are multiple dependencies 
involved, it can quickly get impossible to get setup to do what you want.

The simplest way I could think of to correct this would be to change the 
behavior so that when you click on a Category or a Package, instead of 
simply cycling through, you get a little popup menu that asks you what 
you want to do instead. This way, you can go directly to Uninstall 
without dealing with the intervening options. This also lets you see all 
available versions at once, and avoids calculating dependencies 
unnecessarily.

I wrote a simple patch that implements this suggestion. Attached are the 
outputs of cvs diff (in diff.txt) and cvs diff -n (in diff_n.txt). (I'm 
sorry, I don't know much about CVS, is this the preferred way to submit 
a patch?). Here is a summary of the changes:

-Created new class PopupMenu in PopupMenu.{h,cc}, which makes a popup 
menu at the mouse cursor location and returns the selected item.

-Added #define to resource.h for use by PopupMenu. For now, it just 
reserves 100 IDs, supporting arbitrary popup menus with up to 100 
entries. (The number 100 is easily configurable in resource.h.)

-Modified PickCategoryLine to open the menu instead of cycling.

-Added new function select_action() to the packagemeta class, which 
implements the menu selection. For now, this is done in an extremely 
quick and dirty way that simple calls set_action() repeatedly to figure 
out which options would have been cycled through. I would be willing to 
re-do this in a more efficient way if this patch is deemed useful, but I 
don't even think that's necessary, I think it's fine to do it this way 
which reuses the already bug-tested code in set_action().

-Modified PickPackageLine to call select_action() instead of 
set_action() when the line is clicked.

-Made some minor changes to packagemeta::_action to expose the category 
strings as part of the public interface, so they could be reused in the 
popup menu.

Anyway I hope this is useful, if this patch isn't acceptable please let 
me know and I can fix it or change it. I wasn't sure about conventions 
with tabs, line endings, line lengths, etc., for one thing. In general, 
I think the problem I have described requires fixing. If you don't think 
this solution is an improvement, I can look into fixing it a different 
way also.

-Lewis

--------------000105000800090107080106
Content-Type: text/plain;
 name="diff.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="diff.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 -r2.68 Makefile.am
230a231,232
> 	PopupMenu.h \
> 	PopupMenu.cc \
Index: setup/PickCategoryLine.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/PickCategoryLine.cc,v
retrieving revision 2.10
diff -r2.10 PickCategoryLine.cc
18a19
> #include "PopupMenu.h"
113,117c114,120
< 	  ++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);
Index: setup/PickPackageLine.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/PickPackageLine.cc,v
retrieving revision 2.21
diff -r2.21 PickPackageLine.cc
137c137,140
<     pkg.set_action (pkg.trustp(theView.deftrust));
---
>   {
>     	//pkg.set_action (pkg.trustp(theView.deftrust));
>     	pkg.select_action(pkg.trustp(theView.deftrust));
>   }
Index: setup/package_meta.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_meta.cc,v
retrieving revision 2.52
diff -r2.52 package_meta.cc
20a21
> #include "PopupMenu.h"
68,84c69,76
< 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"
> };
102c94
<   if (_value > 3)
---
>   if (_value >= num_actions)
412a405,428
> 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.
> }
> 
Index: setup/package_meta.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_meta.h,v
retrieving revision 2.36
diff -r2.36 package_meta.h
62a63,64
>   	enum {num_actions = 4};
>   	static char const* const captions[num_actions];
65,66c67,68
<     _value = aInt;
<     if (_value < 0 ||  _value > 3)
---
>     	_value = aInt;
>        if (_value < 0 ||  _value >= num_actions)
72c74,75
<     const char *caption ();
---
>     const char *caption() const {return caption(_value);}
>     static const char* caption(int const value) {return captions[value];}
80a84
>   void select_action(packageversion const &default_version);
Index: setup/resource.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/resource.h,v
retrieving revision 2.35
diff -r2.35 resource.h
161a162,167
> 
> //popup menus
> 
> //note: numbers 600-699 are reserved for popup menus; see PopupMenu.h
> #define IDM_POPUP_FIRST			600
> #define IDM_POPUP_LAST			699

--------------000105000800090107080106
Content-Type: text/plain;
 name="diff_n.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="diff_n.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 -n -r2.68 Makefile.am
a230 2
	PopupMenu.h \
	PopupMenu.cc \
Index: setup/PickCategoryLine.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/PickCategoryLine.cc,v
retrieving revision 2.10
diff -n -r2.10 PickCategoryLine.cc
a18 1
#include "PopupMenu.h"
d113 5
a117 7
	
	  //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);
Index: setup/PickPackageLine.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/PickPackageLine.cc,v
retrieving revision 2.21
diff -n -r2.21 PickPackageLine.cc
d137 1
a137 4
  {
    	//pkg.set_action (pkg.trustp(theView.deftrust));
    	pkg.select_action(pkg.trustp(theView.deftrust));
  }
Index: setup/package_meta.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_meta.cc,v
retrieving revision 2.52
diff -n -r2.52 package_meta.cc
a20 1
#include "PopupMenu.h"
d68 17
a84 8
//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"
};
d102 1
a102 1
  if (_value >= num_actions)
a412 24
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.
}

Index: setup/package_meta.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_meta.h,v
retrieving revision 2.36
diff -n -r2.36 package_meta.h
a62 2
  	enum {num_actions = 4};
  	static char const* const captions[num_actions];
d65 2
a66 2
    	_value = aInt;
       if (_value < 0 ||  _value >= num_actions)
d72 1
a72 2
    const char *caption() const {return caption(_value);}
    static const char* caption(int const value) {return captions[value];}
a80 1
  void select_action(packageversion const &default_version);
Index: setup/resource.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/resource.h,v
retrieving revision 2.35
diff -n -r2.35 resource.h
a161 6

//popup menus

//note: numbers 600-699 are reserved for popup menus; see PopupMenu.h
#define IDM_POPUP_FIRST			600
#define IDM_POPUP_LAST			699


--------------000105000800090107080106
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/
--------------000105000800090107080106--