X-Recipient: archive-cygwin AT delorie DOT com X-Spam-Check-By: sourceware.org To: cygwin AT cygwin DOT com From: Lewis Hyatt Subject: setup.exe suggestion + patch Date: Thu, 06 Sep 2007 15:24:34 -0400 Lines: 410 Message-ID: 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: List-Subscribe: List-Archive: List-Post: List-Help: , 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 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 } > 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 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= 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--