-
-
Notifications
You must be signed in to change notification settings - Fork 230
exposed the mouse button event "clicks" property #3712
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: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdd a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/reST/ref/event.rst`:
- Around line 75-76: Add a short description for the new clicks attribute in the
MOUSEBUTTONDOWN and MOUSEBUTTONUP attribute list explaining it reports the
number of rapid consecutive clicks (as provided by the system/SDL double-click
detection) and add a version directive (e.g., .. versionadded:: 2.5.x) after the
existing .. versionchangedold:: 2.0.1 note stating that the clicks attribute was
added to MOUSEBUTTONDOWN and MOUSEBUTTONUP; reference the attribute name
"clicks" and the event names "MOUSEBUTTONDOWN"/"MOUSEBUTTONUP" so the change is
discoverable.
|
Hello, first of all, thank you for contributing! I agree that event attributes are generally undocumented, but after the list of event types there are a few paragraphs clarifying some event attributes, for example, if you scroll down you can read:
I think this page of the docs is extremely messy, there's version added/version changed tags in random places (they should probably be at the end) and the paragraphs aren't perfectly placed. I think this section should be improved, and I might even try to do that myself. That aside, the same way the touch attribute is "documented", I think a small paragraph near it should be added clarifying the attribute. For example, I can see someone wondering whether a double click will fire a single event with clicks=2 or two events, the first having clicks=1 and the second clicks=2, therefore I think it should be explained. |
|
Thank you! I will take these points into consideration and work on them tomorrow. With that I have a question. What version should I write down? Should it be the version It's nice to give contributing a try, even if it's what I already do at my job haha |
|
@JReesW :) You could put 2.5.7 for now but it would probably need to be updated as I think 2.5.7 won't take long to drop and likely won't receive any new API, tho it's not guaranteed (also we still need the opinion of senior reviewers, but I don't have anything against the addition myself) |
|
@damusss I have added some more clarity to the documentation regarding the |
|
@JReesW thank you, it's looking good! If I could propose a final improvement to the explanation, as mentioned in my original message, I'd add a sentence along the lines of |
|
@damusss done! |
damusss
left a comment
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.
Looking good to me, it's useful information that SDL easily gives us. Thank you :)
|
Awesome, glad I could be of service, and thanks for looking it over for me! :) |
|
@JReesW No problem! I'm pretty sure the contributor role is given after the pull request is merged, for a pull request to be merged it requires at least two approvals, one of which has to come from a senior reviewer or an admin, which I am not, so we'll have to wait and see what their opinions are :) |
|
haha fair enough, we will see! |
|
(if you want the failing check to pass you have to run |
|
done! |
Why?
I am adding this because I wanted a simple mechanism to detect double-clicks inside my game. I found that the
SDL_MouseButtonEventactually has a property calledclicksthat counts the amount of rapid clicks in a row based on the system-defined double click timing. Pygame-CE just didn't expose this functionality. This PR amends this by simply exposing theclicksproperty of the SDL_MouseButtonEvent to the Pygame mouse button events. It is my belief that Pygame should expose this as it is an existing functionality of SDL that Pygame seems to have forgotten to implement, and it offers an easy solution to a commonly tackled problem of how to add double-click detection to Pygame (or any n-clicks detection for that matter).Test program
Below is a simple test program that opens a window that can detect mouse clicks. It prints the button ID and the
clicksproperty whenever a mouse button is clicked. Additionally pressingqexits the program.As can be seen when running the program, if you rapidly click on the window then the
clicksvalue (the second number) increases, showing the number of rapid, consecutive clicks. Pausing for a short second and then clicking shows that theclicksvalue has gone back to1, meaning the pause was enough for the clicks to no longer be considered rapid and consecutive.Tests
No test has been added, as Pygame-CE's tests don't seem to cover testing the individual properties of events. If it should be put in a test, let me know and I'll get on it.
Documentation
The
clicksproperty has been added to the list of properties for both the MOUSEBUTTONUP and MOUSEBUTTONDOWN events. No additional documentation has been added as no individual properties are discussed in depth elsewhere either.