-
Notifications
You must be signed in to change notification settings - Fork 109
NSTextAttachment: make double click handling more robust #157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The change itself looks fine, could you please remove the unrelated change from the other merge request so that this could be merged? |
OK, done |
In the meantime Riccardo set up another pull request #160 and claims that this might resolve your issues as well. I am unsure and your code looks fine to me. Could you please give his changes a try and report back? |
No, my problem is still there. For example: TextEdit fork with support for attachments (https://github.com/onflapp/gs-textedit). The delegate will simply not be called because 2 clicks will select text instead. Interestingly enough, Cocoa doesn't do that. Clicks on attachments are not handled as selection at all - I guess they are being tracked separately. But this might be very recent change because I do vaguely remember that attachments were handled as special characters in the past. My suggesting would be to go ahead with this change as it has minimal impact on anything else and it fixes problem that would normally have to be fixed or worked around in the application code. |
As far as I could test with clicks on cocoa regarding attachments and GNUmail, a single click is correct to invoke the delegate for the save action, multiple click make a select, since it is a character that can be selected, after all. On GNUstep we had an issue with attachment finding depending on the click position, I fixed that. I am somehow unsure that a "multiple click" should be handled like a double-click, it would be like eating other clicks away. |
I don't dispute that GNUmail is working correctly. The delegate is not called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a double-click handling issue in NSTextAttachment by making the click count condition more robust. The existing implementation only triggers on exactly 2 clicks, but the click count may exceed 2 due to text selection behavior interfering with the event handling.
- Changed click count condition from
== 2
to>= 2
for more reliable double-click detection - Applied the fix to both NSLeftMouseUp and NSLeftMouseDown event handling paths
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Do you think this PR is worth merging? I can see how someone might click multiple times. |
Last word to @fredkiefer . I think it is a bad PR, since it removes a safety measure in the click number. What if there is an error? I remember seeing that code when debugging this stuff with Fred and he argumented it that way and I agree. |
Agreed. I've thought about it and it seems clumsy. Given that no decision has been made on this in months I am tempted to close it as I really despise PRs remaining open for inordinate amounts of time. |
the existing implementation of doubleClickedOnCell will not be called because the click count is likely to more more than 2 by that point. I suspect it has to do with double click being interpreted as select word first.
I propose to change the condition to work after 2 or more clicks to make it more robust.