remove shouldFinish to allow outer loop to handle cleanup

[johnathan-becker]

Remove shouldFinish assignment on mouse-up in inner event loop

In the inner do/while loop that drains NSAppKitDefined events after a mouse-up is received, there was a redundant shouldFinish = YES assignment immediately before a break. We didn't need this because shouldFinish is already set to yes in the outer loop whenever a mouse-up event is seen and setting it in the inner loop actually caused the outer loop exit prematurely before it does its cleanup.

[fredkiefer]

I have no issue with removing this specific line, but wouldn't it be better, if we removed the whole if statement? I would prefer to clean up this whole thing once and for all and not have some left over, that doesn't hurt as much as the current code, but is still not a clean solution.

[johnathan-becker]

I am not opposed to that. It will break us but we can come back and find a different solution when we have more time and/or just patch libs-gui when we build GNUstep.

[probonopd]

Thank you very much for looking into this.

On a semi-related note, it would be nice if regular menus and context menus (right-click) would behave consistently in this regard. Thanks!

[pkgdemon]

@probonopd You are right I missed checking context menus during my testing. However reverting 82717ee entirely does not fix this issue for context menus. That would be some other separate fix I think we would need to make and I am not sure if it would even relate to this code at this point.

[probonopd]

@pkgdemon context menus were not working in a consistent way before, so I agree they can be handled in a separate PR.

[johnathan-becker]

@fredkiefer What is your final decision on this MR? I know it is affecting others so I am fine with whatever decision you make. I do need to get something scheduled on our backlog though if we are going to just revert my change (which again I am fine with).

[fredkiefer]

I still prefer the full revert over just removing this one line. Why? When we remove this line there is only a break left that will exit the inner do/while loop. But that would happen anyway as the condition for the loop is not met. This means we have just four extra lines in the code left over without any behavioural change. And I prefer to rather have less code than more.
Given that analysis, I really don't understand how these two changes could end up in different results for you. So either my analysis is wrong or it should just be the same for you.