Skip to content
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

[UIKit] Add Constructor for UIHoverGestureRecognizer #15837

Merged
merged 11 commits into from
Sep 21, 2022

Conversation

haritha-mohan
Copy link
Member

Fixes #15335

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

Great work, in theory, there are not changes needed if you don't want them. I wonder, if we add at the top of the file:

#nullable enable

What happens? It might as for some changes, maybe to many for the intent of the PR (but good for you to check). Tests for the other actions are missing, but we should write them. We need to add tests under:

tests/monotouch/UIkit

I think we can look at how to test the execution of the callback, and then use a pattern for it.

@@ -319,4 +319,27 @@ public void Activated (UIScreenEdgePanGestureRecognizer sender)
#endif
}

// start code for UIHoverGestureRecognizer Constructor
public partial class UIHoverGestureRecognizer : UIGestureRecognizer {
public UIHoverGestureRecognizer (Action action) : base (action) {}
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is not needed since we already expose a constructor in the generated code:

	[NoWatch, NoTV, iOS (13,0)]
	[BaseType (typeof (UIGestureRecognizer))]
	[DisableDefaultCtor]
	interface UIHoverGestureRecognizer {

		[DesignatedInitializer]
		[Export ("initWithTarget:action:")]
		NativeHandle Constructor (NSObject target, Selector action);

		[DesignatedInitializer]
		[Wrap ("base (action)")]
		NativeHandle Constructor (Action action);
	}

Have your tried removing it? Is it giving you an issue?

Comment on lines 338 to 341
public void Activated (UIHoverGestureRecognizer sender)
{
action (sender);
}
Copy link
Member

Choose a reason for hiding this comment

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

Little trick in the new c# syntax, you can do:

Suggested change
public void Activated (UIHoverGestureRecognizer sender)
{
action (sender);
}
public void Activated (UIHoverGestureRecognizer sender) => action (sender);

Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

This is great 👍

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@haritha-mohan
Copy link
Member Author

Will be coming back to fix this issue. After talking to Manuel, we realized it would be better to add generics and clean up the code to make the bug much easier to fix.

@stephen-hawley
Copy link
Contributor

👀

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

Added generic callbacks- this simplifies the code and makes it much easier to fix the issue (dotnet#15335).
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@haritha-mohan haritha-mohan added the notes-mention Deserves a mention in release notes label Sep 9, 2022
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

.gitignore Outdated
Comment on lines 42 to 44
tests/BundledResources/BundledResources.csproj
tests/EmbeddedResources/EmbeddedResources.csproj
tests/fsharplibrary/fsharplibrary.fsproj
Copy link
Member

Choose a reason for hiding this comment

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

This change is not related to this PR, so it should be removed.

If needed, it should go in a separate PR, explaining the reason for the change (I'm also a bit puzzled about this change, because these files are already checked into git, so they shouldn't be ignored, nor should this actually change any behavior),

tests/.gitignore Outdated
Comment on lines 37 to 39
BundledResources/Info.plist
EmbeddedResources/Info.plist
fsharplibrary/Info.plist
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Member

Choose a reason for hiding this comment

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

This is due to vsmac making changes. I wpuld blame VSMac for modifying files when it should not.

Copy link
Member

@tj-devel709 tj-devel709 left a comment

Choose a reason for hiding this comment

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

After addressing the other issues!

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Looks great!

Except that the compiler complains about the test:

tests/monotouch-test/UIKit/GestureRecognizerTest.cs(98,8): error CS0815: Cannot assign lambda expression to an implicitly-typed variable

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1105.Monterey'
Hash: c0e19cef9fe40d9adc49c87c8e3a5fa4c6cfcb55 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: c0e19cef9fe40d9adc49c87c8e3a5fa4c6cfcb55 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS: vsdrops gist (No breaking changes)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS (no change detected)
.NET (No breaking changes)
  • iOS: vsdrops gist (No breaking changes)
  • tvOS: (empty diff detected)
  • MacCatalyst: vsdrops gist (No breaking changes)
  • macOS: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: c0e19cef9fe40d9adc49c87c8e3a5fa4c6cfcb55 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: simulator tests

0 tests crashed, 1 tests failed, 222 tests passed.

Failures

❌ msbuild tests

1 tests failed, 1 tests passed.
  • MSBuild tests/Integration: Failed (Execution failed with exit code 1)

Html Report (VSDrops) Download

Successes

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

@rolfbjarne
Copy link
Member

Test failure is unrelated (https://github.com/xamarin/maccore/issues/2620), so this is good to go!

@haritha-mohan haritha-mohan merged commit a9c8efa into dotnet:main Sep 21, 2022
@haritha-mohan haritha-mohan deleted the gh15335 branch September 21, 2022 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-mention Deserves a mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a ctor to UIHoverGestureRecognizer that takes Action<UIHoverGestureRecognizer>
7 participants