Skip to content

Update window_cocoa.mm#26662

Merged
asmorkalov merged 4 commits intoopencv:4.xfrom
RoshniUG:4.x
Feb 8, 2025
Merged

Update window_cocoa.mm#26662
asmorkalov merged 4 commits intoopencv:4.xfrom
RoshniUG:4.x

Conversation

@RoshniUG
Copy link
Copy Markdown
Contributor

@RoshniUG RoshniUG commented Dec 23, 2024 •
edited
Loading

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake
  • [x ] Added reference to the original bug report (Cocoa/highgui: Incorrectly maps ctrl-click as EVENT_LBUTTONDOWN/EVENT_LBUTTONUP #26661).
  • [x]Updated the code as per the reviewer's suggestion to use a ternary operator.
  • Verified that the feature is properly documented and can be built with CMake.

@asmorkalov asmorkalov added this to the 4.11.0 milestone Dec 23, 2024
@NekoAsakura
Copy link
Copy Markdown
Contributor

NekoAsakura commented Dec 23, 2024

Please reference the original bug report (issues#26661) in PR to help the reviewer quickly locate and reproduce the issue.

Additionally, I believe using ternary expression is better than nested if-else loops in this case.

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Dec 23, 2024

sure , i'll try to modify it using ternary operator

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Dec 24, 2024

I've updated the code to use the ternary operator as suggested. Let me know if further changes are needed!

@NekoAsakura
Copy link
Copy Markdown
Contributor

NekoAsakura commented Dec 24, 2024

Have you read https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request ?

  1. Try not to include "oops" commits - ones that just fix an error in the previous commit. If you have those, then before submitting squash those fixes directly into the commits where they belong.

That is, if you add fixup commits to your pull request, you should typically squash them using git rebase -i.

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Dec 24, 2024

Thank you for your feedback . I have read the guidelines on making-a-good-pull-request , and also i squashed the fixup commits directly into the original commit using git rebase -i .
The commit history should now be cleaner and more organized.
Please let me know if there is any further changes needed

@opencv-alalek
Copy link
Copy Markdown
Contributor

opencv-alalek commented Dec 24, 2024

Files changed 19

You need to exclude unrelated files from PR

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Dec 24, 2024

i have excluded the unrelated files from my pull request , please let me know if there are any further changes

@opencv-alalek
Copy link
Copy Markdown
Contributor

opencv-alalek commented Dec 25, 2024

i have excluded the unrelated files from my pull request , please let me know if there are any further changes

Wrong.

excluded != deleted.

Please learn git basics.

PR should have 1 commits and 1 changes file.

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Dec 25, 2024 via email

@asmorkalov asmorkalov modified the milestones: 4.11.0, 4.12.0 Dec 27, 2024
@seanm
Copy link
Copy Markdown
Contributor

seanm commented Jan 8, 2025

Once you have the commits all cleaned up, ping me and I can review. I know Cocoa.

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Jan 9, 2025 via email

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jan 24, 2025

@RoshniUG Friendly reminder.

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Jan 24, 2025 via email

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jan 31, 2025

@RoshniUG Friendly reminder.

@RoshniUG RoshniUG closed this Jan 31, 2025
@RoshniUG RoshniUG reopened this Jan 31, 2025
@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Jan 31, 2025 via email

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 5, 2025 •
edited
Loading

@RoshniUG please fix CI warnings:

 modules/highgui/src/window_cocoa.mm:879: trailing whitespace.
+    [self cvSendMouseEvent:event 
modules/highgui/src/window_cocoa.mm:880: trailing whitespace.
+                      type:([event modifierFlags] & NSControlKeyMask) ? CV_EVENT_RBUTTONDOWN : CV_EVENT_LBUTTONDOWN 
modules/highgui/src/window_cocoa.mm:885: trailing whitespace.
+    [self cvSendMouseEvent:event 
modules/highgui/src/window_cocoa.mm:886: trailing whitespace.
+                      type:([event modifierFlags] & NSControlKeyMask) ? CV_EVENT_RBUTTONUP : CV_EVENT_LBUTTONUP

@RoshniUG
Copy link
Copy Markdown
Contributor Author

RoshniUG commented Feb 5, 2025

Sure sir I'll look into it

seanm
seanm approved these changes Feb 5, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@seanm seanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm conceptually.

The indentation seems messed up though.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 6, 2025

@RoshniUG Friendly reminder on CI warnings.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 6, 2025

There is merge conflict still.

@asmorkalov asmorkalov merged commit b323780 into opencv:4.x Feb 8, 2025
27 of 28 checks passed
@asmorkalov asmorkalov mentioned this pull request Feb 19, 2025
Merged
NanQin555 pushed a commit to NanQin555/opencv that referenced this pull request Feb 24, 2025
@RoshniUG @NanQin555
Merge pull request opencv#26662 from RoshniUG:4.x
b272866 
Update window_cocoa.mm opencv#26662

### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
- [x ] Added reference to the original bug report (opencv#26661).
- [x]Updated the code as per the reviewer's suggestion to use a ternary operator.
- [x] Verified that the feature is properly documented and can be built with CMake.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VadimLevin VadimLevin VadimLevin approved these changes

+1 more reviewer

@seanm seanm seanm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@VadimLevin VadimLevin

Labels

platform: ios/osx

Projects

None yet

Milestone

4.12.0

Development

Successfully merging this pull request may close these issues.

6 participants

@RoshniUG @NekoAsakura @opencv-alalek @seanm @asmorkalov @VadimLevin