Thursday, March 20, 2025

chatGPT on handle_button_press

The agony:

static void handle_button_press(int event, int state, int rstate, KeySym key, int button, int mx, int my,
                                double c_snap, int draw_xhair, int crosshair_size, int enable_stretch, int aux)
{
   int use_cursor_for_sel = tclgetintvar("use_cursor_for_selection");
   int excl = xctx->ui_state & (STARTWIRE | STARTRECT | STARTLINE | STARTPOLYGON | STARTARC);
   dbg(1, "callback(): ButtonPress  ui_state=%d state=%d\n",xctx->ui_state,state);
   if(waves_selected(event, key, state, button)) {
     waves_callback(event, mx, my, key, button, aux, state);
     return;
   }
   /* terminate a schematic pan action */
   if(xctx->ui_state & STARTPAN) {
     xctx->ui_state &=~STARTPAN;
     return;
   }

   /* select instance and connected nets stopping at wire junctions */
   if(!excl && button == Button3 && state == ControlMask && xctx->semaphore <2)
   {
     Selected sel;
     sel = select_object(xctx->mousex, xctx->mousey, SELECTED, 0, NULL);
     if(sel.type) select_connected_nets(1);
   }
   
   /* break wire at mouse coordinates, move break point to nearest grid point */
   else if(!excl && button == Button3 && EQUAL_MODMASK &&
           !(state & ShiftMask) && xctx->semaphore <2)
   {
     break_wires_at_point(xctx->mousex_snap, xctx->mousey_snap, 1);
   }
   /* break wire at mouse coordinates */
   else if(!excl && button == Button3 && EQUAL_MODMASK &&
           (state & ShiftMask) && xctx->semaphore <2)
   {
     break_wires_at_point(xctx->mousex_snap, xctx->mousey_snap, 0);
   }
   /* select instance and connected nets NOT stopping at wire junctions */
   else if(!excl && button == Button3 && state == ShiftMask && xctx->semaphore <2)
   {
     Selected sel;
     sel = select_object(xctx->mousex, xctx->mousey, SELECTED, 0, NULL);
     if(sel.type) select_connected_nets(0);
   }
   /* moved to Button3 release */
   /*
    * else if(button == Button3 && state == 0 && xctx->semaphore <2) {
    *   context_menu_action(xctx->mousex_snap, xctx->mousey_snap);
    * }
    */

   /* zoom rectangle by right clicking and drag */
   else if(!excl && button == Button3 && state == 0 && xctx->semaphore < 2) {
     zoom_rectangle(START);return;
   }
     
   /* Mouse wheel events */
   else if(handle_mouse_wheel(event, mx, my, key, button, aux, state)) return;

   /* terminate wire placement in snap mode */
   else if(button==Button1 && (state & ShiftMask) && (xctx->ui_state & STARTWIRE) ) {
     snapped_wire(c_snap);
   }
   /* Alt - Button1 click to unselect */
   else if(button==Button1 && (SET_MODMASK) ) {
     unselect_at_mouse_pos(mx, my);
   }
   
   /* Middle button press (Button2) will pan the schematic. */
   else if(button==Button2 && (state == 0)) {
     pan(START, mx, my);
     xctx->ui_state |= STARTPAN;
   }

   /* button1 click to select another instance while edit prop dialog open */
   else if(button==Button1 && xctx->semaphore >= 2) {
     if(tcleval("winfo exists .dialog.textinput")[0] == '1') { /* proc text_line */
       tcleval(".dialog.f1.b1 invoke");
       return;
     } else if(tcleval("winfo exists .dialog.txt")[0] == '1') { /* proc enter_text */
       tcleval(".dialog.buttons.ok invoke");
       return;
     } else if(state==0 && tclgetvar("edit_symbol_prop_new_sel")[0]) {
       tcleval("set edit_symbol_prop_new_sel 1; .dialog.f1.b1 invoke"); /* invoke 'OK' of edit prop dialog */
     } else if((state & ShiftMask) && tclgetvar("edit_symbol_prop_new_sel")[0]) {
       select_object(xctx->mousex, xctx->mousey, SELECTED, 0, NULL);
       tclsetvar("preserve_unchanged_attrs", "1");
       rebuild_selected_array();
     }
   }

   /* Handle the remaining Button1Press events */
   else if(button==Button1) /* MOD button is not pressed here. Processed above */
   {
     xctx->onetime = 0;
     xctx->mouse_moved = 0;
     xctx->drag_elements = 0;

     /* start another wire or line in persistent mode */
     if(tclgetboolvar("persistent_command") && xctx->last_command) {
       if(xctx->last_command == STARTLINE)  start_line(xctx->mousex_snap, xctx->mousey_snap);
       if(xctx->last_command == STARTWIRE){
        if(tclgetboolvar("snap_cursor")
             && (xctx->prev_snapx == xctx->mousex_snap
             && xctx->prev_snapy == xctx->mousey_snap)
             && (xctx->ui_state & STARTWIRE)
             && xctx->closest_pin_found){
          new_wire(PLACE|END, xctx->mousex_snap, xctx->mousey_snap);
          xctx->ui_state &= ~STARTWIRE;
        }
        else
          start_wire(xctx->mousex_snap, xctx->mousey_snap);
      }
        return;
     }
     /* handle all object insertions started from Tools/Edit menu */
     if(check_menu_start_commands(state, c_snap, mx, my)) return;

     /* complete the pending STARTWIRE, STARTRECT, STARTZOOM, STARTCOPY ... operations */
     if(end_place_move_copy_zoom()) return;

     /* Button1Press to select objects */
     if(!excl && !(xctx->ui_state & STARTSELECT)) {
       Selected sel;
       int already_selected = 0;
       int prev_last_sel = xctx->lastsel;
       int no_shift_no_ctrl = !(state & (ShiftMask | ControlMask));

       xctx->shape_point_selected = 0;
       xctx->mx_save = mx; xctx->my_save = my;
       xctx->mx_double_save=xctx->mousex;
       xctx->my_double_save=xctx->mousey;

       #if 0 /* disabled */
       /* Clicking and dragging from a **selected** instance pin will start a new wire
        * if no other elements are selected */
       if(xctx->lastsel == 1 && xctx->sel_array[0].type==ELEMENT) {
         if(add_wire_from_wire(&xctx->sel_array[0], xctx->mousex_snap, xctx->mousey_snap)) return;
         if(add_wire_from_inst(&xctx->sel_array[0], xctx->mousex_snap, xctx->mousey_snap)) return;
       }
       #endif

       /* In *NON* intuitive interface a button1 press with no modifiers will
        * first unselect everything...
        * For intuitive interface unselection see below... */
       if(!xctx->intuitive_interface && no_shift_no_ctrl ) unselect_all(1);

       /* find closest object. Use snap coordinates if full crosshair is enabled
        * since the mouse pointer is obscured and crosshair is snapped to grid points */
       if(draw_xhair && (use_cursor_for_sel || crosshair_size == 0)) {
         sel = find_closest_obj(xctx->mousex_snap, xctx->mousey_snap, 0);
       } else {
         sel = find_closest_obj(xctx->mousex, xctx->mousey, 0);
       }
       dbg(1, "sel.type=%d\n", sel.type);
       /* determine if closest object was already selected when button1 was pressed */
       switch(sel.type) {
         case WIRE:    if(xctx->wire[sel.n].sel)          already_selected = 1; break;
         case xTEXT:   if(xctx->text[sel.n].sel)          already_selected = 1; break;
         case LINE:    if(xctx->line[sel.col][sel.n].sel) already_selected = 1; break;
         case POLYGON: if(xctx->poly[sel.col][sel.n].sel) already_selected = 1; break;
         case xRECT:   if(xctx->rect[sel.col][sel.n].sel) already_selected = 1; break;
         case ARC:     if(xctx->arc[sel.col][sel.n].sel)  already_selected = 1; break;
         case ELEMENT: if(xctx->inst[sel.n].sel)          already_selected = 1; break;
         default: break;
       } /*end switch */

       /* Clicking and drag on an instance pin -> drag a new wire */
       if(xctx->intuitive_interface && !already_selected) {
         if(add_wire_from_inst(&sel, xctx->mousex_snap, xctx->mousey_snap)) return;  
       }
 
       /* Clicking and drag on a wire end -> drag a new wire */
       if(xctx->intuitive_interface && !already_selected) {
         if(add_wire_from_wire(&sel, xctx->mousex_snap, xctx->mousey_snap)) return;  
       }

       /* In intuitive interface a button1 press with no modifiers will
        *  unselect everything... we do it here */
       if(xctx->intuitive_interface && !already_selected && no_shift_no_ctrl )  unselect_all(1);

       /* select the object under the mouse and rebuild the selected array */
       if(!already_selected) select_object(xctx->mousex, xctx->mousey, SELECTED, 0, &sel);
       rebuild_selected_array();
       dbg(1, "Button1Press to select objects, lastsel = %d\n", xctx->lastsel);

       /* if clicking on some object endpoints or vertices set shape_point_selected
        * this information will be used in Motion events to draw the stretched vertices */
       if(xctx->lastsel == 1 && xctx->sel_array[0].type==POLYGON) {
         if(edit_polygon_point(state)) return; /* sets xctx->shape_point_selected */
       }
       if(xctx->lastsel == 1 && xctx->intuitive_interface) {
         int cond = already_selected;

         if(cond && xctx->sel_array[0].type==xRECT) {
           if(edit_rect_point(state)) return; /* sets xctx->shape_point_selected */
         }

         if(cond && xctx->sel_array[0].type==LINE) {
           if(edit_line_point(state)) return; /* sets xctx->shape_point_selected */
         }

         if(cond && xctx->sel_array[0].type==WIRE) {
          if(edit_wire_point(state)) return; /* sets xctx->shape_point_selected */
         }
       }
       dbg(1, "shape_point_selected=%d, lastsel=%d\n", xctx->shape_point_selected, xctx->lastsel);

       /* intuitive interface: directly drag elements */
       if(sel.type && xctx->intuitive_interface && xctx->lastsel >= 1 &&
          !xctx->shape_point_selected) {
         /* enable_stretch (from TCL variable) reverses command if enabled:
          * - move --> stretch move
          * - stretch move (with ctrl key) --> move
          */
         int stretch = (state & ControlMask ? 1 : 0) ^ enable_stretch;
         xctx->drag_elements = 1;
         /* select attached nets depending on ControlMask and enable_stretch */
         if(stretch) {
           select_attached_nets(); /* stretch nets that land on selected instance pins */
         }
         /* if dragging instances with stretch enabled and Shift down add wires to pins
          * attached to something */
         if((state & ShiftMask) && stretch) {
           xctx->connect_by_kissing = 2; /* 2 will be used to reset var to 0 at end of move */
           move_objects(START,0,0,0);
         }
         /* dragging away an object with Shift pressed is a copy (duplicate object) */
         else if(state & ShiftMask) copy_objects(START);
         /* else it is a normal move */
         else move_objects(START,0,0,0);
       }

       if(tclgetboolvar("auto_hilight") && !xctx->shape_point_selected) {
         if(!(state & ShiftMask) && xctx->hilight_nets && sel.type == 0 ) {
           if(!prev_last_sel) {
             redraw_hilights(1); /* 1: clear all hilights, then draw */
           }
         }
         hilight_net(0);
         if(xctx->lastsel) {
           redraw_hilights(0);
         }
       }
       return;
     } /* if(!excl) */
   } /* button==Button1 */
   
   return;
}

And the ecstasy:

Critique of C Code

1. Code Organization & Readability Issues

  • Too Many Nested Conditions: Many if-else conditions are deeply nested, making the code difficult to follow. This should be refactored using helper functions that clearly state their purpose.
  • Lack of Comments for Logical Sections: Some sections are well-commented, but others are cryptic. Inline comments should explain why conditions check multiple key/mouse combinations.
  • Long Function Length (~200+ lines): This function is too long. Ideally, a function should be concise and focused on a single task. Consider breaking it into smaller functions like:
    • handle_mouse_wheel_events()
    • handle_selection()
    • handle_wire_modifications()
    • handle_panning_and_zooming()
    • handle_drag_and_move()

2. Maintainability & Code Duplication

  • Repeated Condition Checks: Instead of repeating similar conditions, refactor them into a dedicated function: 
    
int is_right_click_event(int button, int state) {
    return (button == Button3 && state == ControlMask && xctx->semaphore < 2);
}
    Then, use it like this: 
    
if (is_right_click_event(button, state)) {
    // Handle selection
}
  • Repeated Calls to select_object(): This function is called in multiple places under similar conditions. Consider abstracting selection behavior into a helper function.

3. Performance Considerations

  • Checking Multiple Conditions in a Single if Block: Instead of: 
    
if(!excl && button == Button3 && EQUAL_MODMASK &&
   !(state & ShiftMask) && xctx->semaphore <2 code="">
    Consider defining a common state check: 
    
int is_mouse_event(int button, int state, int modmask) {
    return (!excl && button == Button3 && EQUAL_MODMASK && state == modmask && xctx->semaphore < 2);
}
  • Repeated TCL Calls: Calls like tcleval("winfo exists .dialog.textinput") should be cached if they are expensive.

4. Consistency & Naming Issues

  • Inconsistent Naming Conventions:
    • Some variables use snake_case (mousex_snap), others use PascalCase (Selected sel).
    • Some macros/constants (EQUAL_MODMASK, SET_MODMASK) are in uppercase, while others (STARTWIRE) appear mixed.
  • Use More Descriptive Variable Names:
    • excl → selection_excluded
    • sel → selected_object
    • aux → extra_data

5. Edge Cases & Potential Bugs

  • Handling Invalid States: There are some checks for UI states, but what if xctx->ui_state has an unexpected value? It’s safer to include debug messages or default behavior.
  • Potential Uninitialized Variable (sel): Selected sel; should be initialized properly before use.
  • Hardcoded Magic Numbers (xctx->semaphore >= 2): Use named constants instead: 
    
#define SEMAPHORE_THRESHOLD 2
if (xctx->semaphore >= SEMAPHORE_THRESHOLD) { ... }

6. Possible Refactoring

A possible structure after breaking down the function:


static void handle_button_press(int event, int state, int rstate, KeySym key, int button, int mx, int my,
                                double c_snap, int draw_xhair, int crosshair_size, int enable_stretch, int aux) 
{ 
    dbg(1, "callback(): ButtonPress  ui_state=%d state=%d
", xctx->ui_state, state);

    if (waves_selected(event, key, state, button)) {
        waves_callback(event, mx, my, key, button, aux, state);
        return;
    }

    if (handle_schematic_pan()) return;
    if (handle_wire_selection(button, state)) return;
    if (handle_mouse_wheel(event, mx, my, key, button, aux, state)) return;
    if (handle_symbol_property_dialog(button, state, mx, my)) return;
    if (handle_object_selection(button, state, mx, my, c_snap, draw_xhair, crosshair_size, enable_stretch)) return;
}

Final Thoughts

✅ Pros:

  • Covers all necessary button interactions for the schematic editor.
  • Includes a lot of functionality like selection, panning, and wire manipulation.
  • Well-commented in some parts.

❌ Cons & Areas for Improvement:

  • The function is too long and needs to be split into smaller functions.
  • Many nested conditions reduce readability.
  • Code duplication (e.g., repeated select_object() calls).
  • Performance inefficiencies (repeated tcleval() calls).
  • Inconsistent naming makes it harder to follow.

Overall, a great effort, but it needs serious refactoring for maintainability and readability! 🚀

at
Labels: , , ,

No comments:

Post a Comment

Subscribe to: Post Comments (Atom)

Who Needs Cadence Anymore (for Schematic Tree Reporting that is :)? Make Your Own Reporter with chatGPT!

You know what I mean, if you've used Cadence - the SHIFT-T thing that gives you the schematic tree can be VERY useful, at times. No Cade...