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

Add pressed and released events to Button #446

Merged
merged 34 commits into from Feb 2, 2017

Conversation

Projects
None yet
9 participants
@rookiejava
Contributor

rookiejava commented Oct 11, 2016

Description of Change

With this PR, the following events are available, and works as expected:

public ButtonDemoPage()
{
    //...
    Button button = new Button
    {
         Text = "Click Me!",
         Font = Font.SystemFontOfSize(NamedSize.Large),
         BorderWidth = 1,
         HorizontalOptions = LayoutOptions.Center,
        VerticalOptions = LayoutOptions.CenterAndExpand
    };
    button.Clicked += OnButtonClicked;
    button.Pressed += OnButtonPressed;
    button.Released += OnButtonReleased;
    //...
}

void OnButtonClicked(object sender, EventArgs e)
{
    //Button clicked
}

void OnButtonPressed(object sender, EventArgs e)
{
    //Button pressed
}

void OnButtonReleased(object sender, EventArgs e)
{
    //Button released
}

As I proposed on mailing list thread, events occur in the following order - pressed -> released -> clicked.

Bugs Fixed

  • None

API Changes

Added:

  • event Button.Pressed
  • event Button.Released

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
using static System.String;
namespace Xamarin.Forms.Platform.Android.AppCompat
{
public class ButtonRenderer : ViewRenderer<Button, AppCompatButton>, global::Android.Views.View.IOnAttachStateChangeListener
public class ButtonRenderer : ViewRenderer<Button, AppCompatButton>, AView.IOnAttachStateChangeListener

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

I don't have access to my IDE now, but why make these changes? Were they suggested by Resharper?

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

I don't have access to my IDE now, but why make these changes? Were they suggested by Resharper?

This comment has been minimized.

@rookiejava

rookiejava Oct 11, 2016

Contributor

This is simply because consistency and efficiency.
Do you want to keep "global::Android.Views.View" only here? Or, doens't want to use alias "AView" ?

@rookiejava

rookiejava Oct 11, 2016

Contributor

This is simply because consistency and efficiency.
Do you want to keep "global::Android.Views.View" only here? Or, doens't want to use alias "AView" ?

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

I think you're safe.

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

I think you're safe.

This comment has been minimized.

@rmarinho

rmarinho Oct 12, 2016

Member

This is a normal thing because of namespaces.

@rmarinho

rmarinho Oct 12, 2016

Member

This is a normal thing because of namespaces.

@@ -31,5 +31,37 @@
<remarks>To be added.</remarks>
</Docs>
</Member>
<Member MemberName="SendPressed">
<MemberSignature Language="C#" Value="public void SendPressed ();" />

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

Is there any special meaning in adding space as in SendPressed ();?

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

Is there any special meaning in adding space as in SendPressed ();?

This comment has been minimized.

@rookiejava

rookiejava Oct 11, 2016

Contributor

No. These are auto-generated by monodoc. See also other APIS too. You can easily find many spaces like SendClicked (). :-)

@rookiejava

rookiejava Oct 11, 2016

Contributor

No. These are auto-generated by monodoc. See also other APIS too. You can easily find many spaces like SendClicked (). :-)

@@ -21,6 +21,9 @@
<Interface>
<InterfaceName>Xamarin.Forms.IElementConfiguration&lt;Xamarin.Forms.Button&gt;</InterfaceName>
</Interface>
<Interface>
<InterfaceName>Xamarin.Forms.IFontElement</InterfaceName>
</Interface>

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

Where did these come from? (changes in L2 - L26)

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

Where did these come from? (changes in L2 - L26)

This comment has been minimized.

@rookiejava

rookiejava Oct 11, 2016

Contributor

This changes are automatically generated by 'monodoc' with latest code. I'll update if it is not necessary to change.

Thanks for all your comments. :-)

@rookiejava

rookiejava Oct 11, 2016

Contributor

This changes are automatically generated by 'monodoc' with latest code. I'll update if it is not necessary to change.

Thanks for all your comments. :-)

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

Wait for XF team to comment on this.

@adrianknight89

adrianknight89 Oct 11, 2016

Contributor

Wait for XF team to comment on this.

This comment has been minimized.

@rookiejava

rookiejava Oct 11, 2016

Contributor

OK

@rookiejava

rookiejava Oct 11, 2016

Contributor

OK

This comment has been minimized.

@rmarinho

rmarinho Oct 12, 2016

Member

Yeah it maybe could be missing. Saw this before,

@rmarinho

rmarinho Oct 12, 2016

Member

Yeah it maybe could be missing. Saw this before,

using static System.String;
namespace Xamarin.Forms.Platform.Android.AppCompat
{
public class ButtonRenderer : ViewRenderer<Button, AppCompatButton>, global::Android.Views.View.IOnAttachStateChangeListener
public class ButtonRenderer : ViewRenderer<Button, AppCompatButton>, AView.IOnAttachStateChangeListener

This comment has been minimized.

@rmarinho

rmarinho Oct 12, 2016

Member

This is a normal thing because of namespaces.

@rmarinho

rmarinho Oct 12, 2016

Member

This is a normal thing because of namespaces.

Show outdated Hide outdated Xamarin.Forms.Platform.Android/AppCompat/ButtonRenderer.cs
button.Click += HandleButtonClick;
button.AddHandler(UIElement.TapEvent, new EventHandler<GestureEventArgs>(HandleButtonTap), true);

This comment has been minimized.

@rmarinho

rmarinho Oct 12, 2016

Member

Don't we need to remove the handler?

@rmarinho

rmarinho Oct 12, 2016

Member

Don't we need to remove the handler?

This comment has been minimized.

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes, I think so, Just like we didn't remove Click event handler here. (e,g, button.Click -= HandleButtonClick)

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes, I think so, Just like we didn't remove Click event handler here. (e,g, button.Click -= HandleButtonClick)

@@ -21,6 +21,9 @@
<Interface>
<InterfaceName>Xamarin.Forms.IElementConfiguration&lt;Xamarin.Forms.Button&gt;</InterfaceName>
</Interface>
<Interface>
<InterfaceName>Xamarin.Forms.IFontElement</InterfaceName>
</Interface>

This comment has been minimized.

@rmarinho

rmarinho Oct 12, 2016

Member

Yeah it maybe could be missing. Saw this before,

@rmarinho

rmarinho Oct 12, 2016

Member

Yeah it maybe could be missing. Saw this before,

@@ -98,6 +102,7 @@ protected override void OnElementChanged(ElementChangedEventArgs<Button> e)
AppCompatButton button = CreateNativeControl();
button.SetOnClickListener(ButtonClickListener.Instance.Value);
button.SetOnTouchListener(ButtonTouchListener.Instance.Value);

This comment has been minimized.

@rmarinho

rmarinho Oct 12, 2016

Member

Are we disposing this?

@rmarinho

rmarinho Oct 12, 2016

Member

Are we disposing this?

This comment has been minimized.

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes. It is same as ClickListener. At line 80,

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes. It is same as ClickListener. At line 80,

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Oct 12, 2016

Member

Looks good, Can we also add UItests ?

Member

rmarinho commented Oct 12, 2016

Looks good, Can we also add UItests ?

@rmarinho rmarinho self-assigned this Oct 12, 2016

@rookiejava

I've added inline comments. Thanks for your review.
I'll do a new PR for UITest soon.

@@ -98,6 +102,7 @@ protected override void OnElementChanged(ElementChangedEventArgs<Button> e)
AppCompatButton button = CreateNativeControl();
button.SetOnClickListener(ButtonClickListener.Instance.Value);
button.SetOnTouchListener(ButtonTouchListener.Instance.Value);

This comment has been minimized.

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes. It is same as ClickListener. At line 80,

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes. It is same as ClickListener. At line 80,

button.Click += HandleButtonClick;
button.AddHandler(UIElement.TapEvent, new EventHandler<GestureEventArgs>(HandleButtonTap), true);

This comment has been minimized.

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes, I think so, Just like we didn't remove Click event handler here. (e,g, button.Click -= HandleButtonClick)

@rookiejava

rookiejava Oct 13, 2016

Contributor

Yes, I think so, Just like we didn't remove Click event handler here. (e,g, button.Click -= HandleButtonClick)

Show outdated Hide outdated Xamarin.Forms.Platform.Android/AppCompat/ButtonRenderer.cs

@rmarinho rmarinho requested a review from jassmith Dec 17, 2016

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Dec 17, 2016

Member

Can you rebase this thanks.

Member

rmarinho commented Dec 17, 2016

Can you rebase this thanks.

rmarinho and others added some commits Dec 16, 2016

Add sample HanselForms and TwitterDemo to ControlGallery (#651)
* [Controls] Add Hanselforms sample

* Remove extra twitter sample

* [Controls]Add TwitterDemo sample

* [Controls] Fix build
[iOS] Entry should not pass a newline to the next responder (#397)
* UITextField should not return so that the next field does not get passed a newline

* Added code sample
Xamlc compile data triggers (#648)
* [Xaml] DataTrigger and PropertyCondition no longer use a ServiceProvider

* [XamlC] avoid generating ServiceProvider for unused ProvideValue

* fix tests

hartez and others added some commits Dec 29, 2016

[XamlC] detect duplicate x:Name at compile time (#655)
* [XamlC] detect duplicate x:Name at compile time

* invoking methods with the right arguments produces better results
[iOS] Change keyboard type while keyboard is visible (#443)
* Change keyboard while changing text

* add sample code
[Android] Fix NavigationPage dispose crash when it parents a MasterDe…
…tailPage (#577)

* fix navigation page dispose crash

* changes after review
[iOS] Prevent multiple ListView cells from being swiped simultaneously (
#578)

* disable multiple cell swipe

* add sample code

* refactored

* convert to weakreference

* remove null setting

* change weakreference setting place

* remove if

* revert isopen changes

* add instructions
[WinRT/UWP] Apply BackgroundColor to Stepper buttons (#581)
* [WinRT/UWP] Apply BackgroundColor to Stepper buttons

* Add explanatory text; use nameof

* Move explanatory text to a label
[iOS/Android] Move Map camera to correct region on layout change (#548)
* Move to region on layout change

* remove visibility check
Allow subscriber to be collected if MessagingCenter is the only refer…
…ence to it (#617)

* Repro

* Make messaging center callbacks weak references

* Preserve attribute

* Fix test method name

* Watch for collection of actual delegate target instead of wrapper delegate

* Preserve the original platform instance when changing main page

* Better tests for lambda situations

* Update tests, make callback target a weakreference if it's the subscriber

* Ensure old Platform MessagingCenter subs are gone before creating new Platform
[iOS] Platform specifics for controlling Picker SelectedIndex change …
…behavior (#540)

* picker selected index could change when picker view is dismissed

* use enum
@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Jan 26, 2017

Member

Have you considered the alternate pattern for this, which is using a PressedGestureRecognizer and ReleasedGestureRecognizer. This would allow, without adding a Pressed state information, to change the visual of the element being pressed.

Member

StephaneDelcroix commented Jan 26, 2017

Have you considered the alternate pattern for this, which is using a PressedGestureRecognizer and ReleasedGestureRecognizer. This would allow, without adding a Pressed state information, to change the visual of the element being pressed.

@rookiejava

This comment has been minimized.

Show comment
Hide comment
@rookiejava

rookiejava Jan 31, 2017

Contributor

Please refer to the thread related on it. (6 month ago) - https://lists.dot.net/pipermail/forms-devel/2016-August/000116.html

I could do a PR for

a) Adding the Button.Pressed and Button.Released events. (In this case,
events occur in the following order. pressed -> released -> clicked)

or

b) Adding the Button.ClickMode (Press/Release) property similar to UWP way.

or

c) Adding a new GestureRecognizer (e.g. TouchGestureRecognizer).

What do you prefer? Or, any other suggestions?

As you can see above, I asked the preference of Xamarin Team. And, I've got the feedback from @jassmith.

That's why I choose option a) - Adding the pressed amd released events to the Button. Please let me know again if you all want to change.

Contributor

rookiejava commented Jan 31, 2017

Please refer to the thread related on it. (6 month ago) - https://lists.dot.net/pipermail/forms-devel/2016-August/000116.html

I could do a PR for

a) Adding the Button.Pressed and Button.Released events. (In this case,
events occur in the following order. pressed -> released -> clicked)

or

b) Adding the Button.ClickMode (Press/Release) property similar to UWP way.

or

c) Adding a new GestureRecognizer (e.g. TouchGestureRecognizer).

What do you prefer? Or, any other suggestions?

As you can see above, I asked the preference of Xamarin Team. And, I've got the feedback from @jassmith.

That's why I choose option a) - Adding the pressed amd released events to the Button. Please let me know again if you all want to change.

@rookiejava rookiejava closed this Jan 31, 2017

@rookiejava rookiejava reopened this Jan 31, 2017

@rmarinho rmarinho merged commit 9a6c9a0 into xamarin:master Feb 2, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 351, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3697, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 34…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 344…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 346…
Details

@rookiejava rookiejava deleted the rookiejava:add-pressed-released-events-to-button branch Feb 3, 2017

@samhouts samhouts added D15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment