[Bf-committers] Select menu patch, code review
Ton Roosendaal
ton at blender.org
Mon Nov 1 12:13:54 CET 2004
Hi,
This all was on my todo for long...
Patch was submitted by Campbell Barton (included below) for drawing a
popup menu on ALT+Select to show all objects that are under the cursor.
Although the patch seemingly works OK there are a number of issues with
it which are intersting to discuss.
Most of all, reviewing a patch is very hard without a good tool. Would
be nice if we can have a way to view patches with nice coloring & info,
like cvsweb does!
Review:
- first you can notice is that the code for a menu was mixed within the
normal selection code, using same code in duplicate and confusing the
total readabilty of the function. Instead I've created a separate
function for it. This is a good convention especially when adding
changes in code being maintained by others, that way it is much easier
to review patches.
- by inserting menu code in the normal selection code, the menu
displayed each time different ordering of Objects. This because the
normal selection cycles through the list of Objects, making it possible
to select sequentally. The separate function will display the list of
objects identical on each click
- the code to construct a menu item was confusing and used far too many
calls for the simple task. Probably because the developer didn't know
the trick to printf a '%' character :)
- the coder used a char array of 2 bytes, and printed a number in it
(up to 2 chars). Char arrays always need 1 extra byte for the trailing
zero!
- the coder changed the 'dist' value for CTRL select to 1000 (why!)
which would result in a CTRL-click select of objects being 1000 pixels
away from the cursor. I only discovered that while recoding it and
doing a new diff with cvs.
- the code called a function (pupmenu) that was not declared in an
include file. When developing for Blender, always check on the printed
warnings!
For those still interested, compare patch below with the cvs diff:
http://projects.blender.org/viewcvs/viewcvs.cgi/blender/source/blender/
src/editview.c.diff?r1=1.41&r2=1.42&cvsroot=bf-blender
Please note that the cvs diff shows more changes than actually made,
mostly because I first applied campbells patch and then cleaned it up.
:)
-Ton-
Index: source/blender/src/editview.c
===================================================================
RCS file: /cvsroot/bf-blender/blender/source/blender/src/editview.c,v
retrieving revision 1.38
diff -u -r1.38 editview.c
--- source/blender/src/editview.c 1 Oct 2004 14:48:12 -0000 1.38
+++ source/blender/src/editview.c 3 Oct 2004 12:45:53 -0000
@@ -853,19 +853,34 @@
}
}
+/* The max number of menu items in an object select menu */
+#define SEL_MENU_SIZE 22
+
void mouse_select(void)
{
Base *base, *startbase=NULL, *basact=NULL, *oldbasact=NULL;
+
+ /* Vars for object selection menu */
+ Base *baseList[SEL_MENU_SIZE]; /*baseList is used to store all
possible bases to bring up a menu */
+ short baseCount = 0;
+ /* menuText should be the initial size of text (18 atm) + (
SEL_MENU_SIZE * 26)
+ 26 being the longest possible length for a menu item */
+ char menuText[512] = "Select Object%t";
+ char menuIndexChar[2];
+ /*end select menu */
+
unsigned int buffer[MAXPICKBUF];
- int temp, a, dist=100;
+ int temp, a, dist=1000;
short hits, mval[2];
+
/* always start list from basact */
startbase= FIRSTBASE;
if(BASACT && BASACT->next) startbase= BASACT->next;
getmouseco_areawin(mval);
-
+
+ /* This block uses the controle key to make the object selected by
its centre point rather then its contentse*/
if(G.obedit==0 && (G.qual & LR_CTRLKEY)) {
base= startbase;
@@ -878,8 +893,28 @@
temp= abs(base->sx -mval[0]) + abs(base->sy -mval[1]);
if(base==BASACT) temp+=10;
if(temp<dist ) {
- basact= base;
+
dist= temp;
+
+
+ /* Generate menu text */
+ if (G.qual & LR_ALTKEY) {
+ if (baseCount < SEL_MENU_SIZE) {
+ baseList[baseCount] = base;
+ strcat(menuText, "|"); /* Seperate menu items */
+ sprintf(menuIndexChar, "%d", baseCount+1); /* Turn Base count
into a string */
+ strcat(menuText, base->object->id.name+2); /*Object name*/
+ strcat(menuText, "%x");
+ strcat(menuText, menuIndexChar);
+
+ baseCount++;
+ } /* end generating menu*/
+ } else { /* no altkey, no menu */
+ basact= base; /* No menu so we need to do this now */
+ }
+
+
+
}
}
base= base->next;
@@ -887,9 +922,27 @@
if(base==0) base= FIRSTBASE;
if(base==startbase) break;
}
-
+
+ if (G.qual & LR_ALTKEY) {
+ /* USE MENU IF MORE THEN 1 ITEM ARE UNDER THE MOUSE*/
+ if (baseCount == 1) { /* Only 1 item, dont bother with a menu */
+ base = baseList[0];
+ basact = base;
+ } else { /* We have more then 1 item under the mouse cursor */
+
+ baseCount = pupmenu(menuText);
+
+ if (baseCount != -1) { /* If nothing is selected then dont do
anything */
+ base = baseList[baseCount-1];
+ basact = base;
+ } else {
+ return; /* No menu item selected so give up now */
+ }
+ }
+ } /*end altkey/menu option*/
+
/* complete redraw when: */
- if(G.f & (G_VERTEXPAINT+G_FACESELECT+G_TEXTUREPAINT+G_WEIGHTPAINT))
allqueue(REDRAWVIEW3D, 1);
+ if(G.f & (G_VERTEXPAINT+G_FACESELECT+G_TEXTUREPAINT+G_WEIGHTPAINT))
allqueue(REDRAWVIEW3D, 0);
}
else {
@@ -903,15 +956,59 @@
if(base->lay & G.vd->lay) {
for(a=0; a<hits; a++) {
/* index was converted */
- if(base->selcol==buffer[ (4 * a) + 3 ]) basact= base;
+ if(base->selcol==buffer[ (4 * a) + 3 ]) {
+
+
+
+ /* Generate menu text */
+ if (G.qual & LR_ALTKEY) {
+ if (baseCount < SEL_MENU_SIZE) {
+ baseList[baseCount] = base;
+ strcat(menuText, "|"); /* Seperate menu items */
+ sprintf(menuIndexChar, "%d", baseCount+1); /* Turn Base count
into a string */
+ strcat(menuText, base->object->id.name+2); /*Object name*/
+ strcat(menuText, "%x");
+ strcat(menuText, menuIndexChar);
+ baseCount++;
+ } /* end generating menu*/
+ } else { /* no altkey, no menu */
+ basact= base; /* No menu so we need to do this now */
+ }
+
+
+ }
}
}
- if(basact) break;
+
+ /* Act normaly if Menu isnt being used */
+ if (!(G.qual & LR_ALTKEY)) {
+ if(basact) break; /* No menu so we need to do this now */
+ }
base= base->next;
if(base==0) base= FIRSTBASE;
if(base==startbase) break;
}
+
+
+ /* Menu only if the altkey is pressed */
+ if (G.qual & LR_ALTKEY) {
+ /* USE MENU IF MORE THEN 1 ITEM ARE UNDER THE MOUSE*/
+ if (baseCount == 1) { /* Only 1 item, dont bother with a menu */
+ base = baseList[0];
+ basact = base;
+ } else { /* We have more then 1 item under the mouse cursor */
+ baseCount = pupmenu(menuText);
+
+ if (baseCount != -1) { /* If nothing is selected then dont do
anything */
+ base = baseList[baseCount-1];
+ basact = base;
+ } else {
+ return; /* No menu item selected so give up now */
+ }
+ }
+ } /*end altkey/menu option*/
+
}
}
------------------------------------------------------------------------
--
Ton Roosendaal Blender Foundation ton at blender.org
http://www.blender.org
More information about the Bf-committers
mailing list