GNOME Bugzilla – Bug 398188
Selection outline flips and rotates
Last modified: 2007-07-08 22:17:26 UTC
Please describe the problem: The selection outline will rotate or flip horizontally/vertically or something... Steps to reproduce: 1. New Image, pick the rectangle selection tool 2. Click the mouse, press CTRL+SHIFT to start a centered square selection, and start to drag... 3. Now, during the drag, release the SHIFT button only, and drag the selection to a rectangular shape... 4. Press the SHIFT button again and continue dragging... Actual results: At this point it really goes crazy... If you drag slow it seems like the selection outline is flipping or rotating... (if you have long width/short height selection you can really see it) Expected results: Pressing shift should just square up the selection outline... Does this happen every time? Yes Other information: Xubuntu 6.10
Should be looked into before 2.4.
Created attachment 88434 [details] [review] intermediate rough patch, still with debugging printout This patch seems to help a bit for the specific sequence of shift/controll pressing/releasing described here. But still using slightly different sequences of pressing and releasing shift and control one gets into the mode where the rectangle should be a square but isn't, and toggles back and forth between two aspects ratios for the mouse motion events... The patch includes a bunch of g_print() statements, sorry. I am kinda stunned by how hard fixing this has turned out to be. Maybe even it would be best to just rewrite the logic in gimp_rectangle_tool_motion() from scratch... Another thought that comes to mind while working on this is that maybe too much internal state is kept? Isn't it so that if one keeps state variables that can be calculated from other state variables as separate, there is always the risk that they get into an inconsistent state.
(In reply to comment #2) > I am kinda stunned by how hard fixing this has turned out to be. Maybe even it > would be best to just rewrite the logic in gimp_rectangle_tool_motion() from > scratch... The code is indeed quite a mess, but I am convinced the right thing to do is to refactor and clean it up instead of rewriting it. Wasn't the current code a rewrite to being with? :) > Another thought that comes to mind while working on this is that maybe too much > internal state is kept? Isn't it so that if one keeps state variables that can > be calculated from other state variables as separate, there is always the risk > that they get into an inconsistent state. I think that's a good point. What I think is OK though is to have many state variables that are setup only in one place (_button_press) and then used as read only values.
Created attachment 90989 [details] [review] Gimp-Rectangle-Tool-Motion-Rewrite-draft-8.patch It was pointless trying to refactor that mess, you were right Tor, a rewrite is more or less the only way of getting control of that code. I am working on a rewrite and I have come quite far, I now have a base good enough for making it enjoyable to build upon. Since it is of little use to commit code known to be incomplete, I will aim for fixing this bug, bug #353936 and bug #452787 in one commit. This patch is compilable and already works pretty well when not using both fixed_center and fixed_aspect at the same time. I don't expect me to need more than two weekends to polish this up. Also, keeping it here is a nice backup, it would suck to lose this code in a harddisk crash :)
Created attachment 91127 [details] [review] Gimp-Rectangle-Tool-Motion-Rewrite-draft-13.patch This patch fixes this bug, bug #353936 and bug #417168, but has some other flaws left, as well as some missing functionality when updating the tool options etc, but this should be easy and the final patch is hopefully fixed in some days. (This patch is also easier to read "raw" than the other one.)
Created attachment 91432 [details] [review] Gimp-Rectangle-Tool-Motion-Rewrite-commitable-1.patch This patch fixes above mentioned bugs, as well as makes adjusting the rectangle with the keys or tool options obeying current tool options.
Created attachment 91436 [details] [review] Gimp-Rectangle-Tool-Motion-Rewrite-commitable-2.patch Made LEFT, RIGHT etc into enums.
Created attachment 91445 [details] [review] Gimp-Rectangle-Tool-Motion-Rewrite-commitable-3.patch Removed gimp_rectangle_tool_update_and_redraw, and refactored gimp_rectangle_tool_active_modifier_key accordinlgy. Also removed gimp_rectangle_tool_get_fixed_center_coords.
Commited slighly modified patch to trunk, revision 22903. 2007-07-08 Martin Nordholts <martinn@svn.gnome.org> Completely rewrote logic in gimp_rectangle_tool_motion, in effect also fixing bug #353936 and bug #398188. The general strategy now is to have specialized functions doing one thing, and one thing only. The patch also makes adjusting the rectangle through the keyboard and through the rectangle tool options follow tool options. * app/tools/gimprectangletool.c (gimp_rectangle_tool_motion): Completely refactored. (gimp_rectangle_tool_active_modifier_key): (gimp_rectangle_tool_update_options): Submit to tool options. (gimp_rectangle_tool_apply_coord) (gimp_rectangle_tool_clamp) (gimp_rectangle_tool_clamp_width) (gimp_rectangle_tool_clamp_height) (gimp_rectangle_tool_keep_inside) (gimp_rectangle_tool_keep_inside_horizontally) (gimp_rectangle_tool_keep_inside_vertically) (gimp_rectangle_tool_apply_fixed_width) (gimp_rectangle_tool_apply_fixed_height) (gimp_rectangle_tool_apply_aspect) (gimp_rectangle_tool_update_with_coord) (gimp_rectangle_tool_get_constraints) (gimp_rectangle_tool_handle_general_clamping): The new specialized functions.