Opened 2 years ago

Closed 2 years ago

#75 closed defect (fixed)

Grabbing a brush doesn't display the XOR box

Reported by: yrizoud@… Owned by: PulkoMandy
Priority: minor Milestone: 2.6
Component: GrafX2 Version: 2.5
Keywords: Cc:

Description

Press B to start grabbing, click and hold the mouse button : Instead of having a "xor" rectangle following the mouse, there is one '+' cursor staying at the starting position, and one '+' cursor following the mouse.

On mouse button release, the brush is grabbed, however sometimes the first + mouse curor is not cleaned properly : You have to move the mouse over it to clear it.

IIRC this is the kind of visual artifact that you get when you change the mouse cursor type (global variable) while the cursor is already displayed.

Tested on Windows 2.5.1949 (portable build)

Change History (13)

comment:1 Changed 2 years ago by PulkoMandy

Milestone: 2.52.6

comment:2 Changed 2 years ago by Philip Linde

This problem was introduced with commit f2d579695cf78bf3e538383bd5843347be0c6acf, which refers to issue #54.

comment:4 Changed 2 years ago by Philip Linde

On a closer look, it seems like it might be related to CURSOR_FOR_OPERATION which is indexed by the OPERATIONS enum value, but it was not updated with the commit that added new circle operations. This array needs to be exhaustive and the elements need to be in the same order as the enum values for it to work, or Start_operation_stack will happily pick a cursor that is associated with another operation.

For now I will simply update the table and submit a pull request, but in the long run it should probably be replaced by a big switch (which the compiler can check is exhaustive and isn't dependent on ordering) or included in the Operations struct and initialized along with all the other operation properties in Init_operation.

comment:5 Changed 2 years ago by Philip Linde

I have submitted merge request 67 to address this issue. It seems to work fine here.

comment:6 Changed 2 years ago by Philip Linde

Note that the patch sets the XOR cursor for all the circles. I am not sure this is correct or what the preferred cursor mode is, but it currently looks OK and I thought this issue more pressing. I may have some time tomorrow evening to amend the patch if you have any changes in mind.

comment:7 Changed 2 years ago by PulkoMandy

Merged as-is for now. We can review the exact cursors used for each op when we put them back into the operation table (not sure why they ended being separate).

comment:8 Changed 2 years ago by Thomas Bernard

Thank you for spotting the problem !

comment:9 Changed 2 years ago by Philip Linde

Thanks for accepting the request so quickly!

I think the reason for keeping the cursors separate from the Operation table may have been that the cursors are only defined per operation, while the Operation table is per operation-per mouse action-per stack size, so keeping the info in the Operation table would mean a lot of redundant fields. But that still seems preferable to a separate table that's so easy to miss.

An alternative may be to have the CURSOR_FOR_OPERATION table filled with some default value from the beginning (maybe CURSOR_SHAPE_ARROW), and to have init.c set the cursors per operation along with the Init_operation calls. Then this initialization is all in the same place, and operations won't get assigned the wrong cursor because they are ordered the wrong way in case someone forgets setting the cursor for the operation.

comment:10 Changed 2 years ago by PulkoMandy

Well, some operations do change cursor in different steps. For example for a grad-rect, you first draw the rectangle using XOR lines, and then define the gradient with a cross-cursor IIRC?

comment:11 Changed 2 years ago by Philip Linde

That's right, I think they do that currently by changing the cursor within the operation handlers. So better to keep it in the Operation table.

comment:12 Changed 2 years ago by PulkoMandy

Owner: changed from pulkomandy to PulkoMandy
Status: newassigned

comment:13 Changed 2 years ago by PulkoMandy

Resolution: fixed
Status: assignedclosed

Closing as the merge request has been included.

Note: See TracTickets for help on using tickets.