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

[TIMOB-26174] Android: add setCancelable method on Ti.UI.AlertDialog #10146

Merged
merged 8 commits into from Jul 18, 2018

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Jul 2, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-26174

Description:
Adds the buttonClickRequired property to Ti.UI.AlertDialog, which controls whether the dialog can be cancelled by clicking the Android system back button. Defaults to true.

Note: No unit tests, because this property requires a user interaction.

Test case:
app.js

var win = Ti.UI.createWindow({layout: 'vertical'}),
	button1 = Ti.UI.createButton({title: 'Cancelable'}),
	button2 = Ti.UI.createButton({title: 'Non-cancelable'}),
	dialog = Ti.UI.createAlertDialog({
		message: 'Message',
		ok: 'OK',
		title: 'Title',
		canceledOnTouchOutside: false,
		persistent: true
	});

button1.addEventListener('click', function () {
	dialog.buttonClickRequired = false;
	dialog.show();
})

button2.addEventListener('click', function () {
	dialog.buttonClickRequired = true;
	dialog.show();
})

win.add([button1, button2]);
win.open();

@ypbnv ypbnv added this to the 7.4.0 milestone Jul 2, 2018
@ypbnv ypbnv requested a review from garymathews July 2, 2018 15:31
@build build added the docs label Jul 2, 2018
@jquick-axway
Copy link
Contributor

I'm thinking that having both a cancel property and a cancelable property might make things confusing from an API standpoint.

The existing cancel property is a zero based index to one of the buttons in property buttonNames that represents the cancel/close button. On Windows, this also tells the system which button to simulator a "click" on when the escape key is pressed.

My point here is if you set cancelable to false, it suggests that the dialog cannot be canceled. However, the cancel button index is still relevant and you can still get a "click" event with property cancel set to true if that button was pressed. This may be confusing to developers. What developers are really after here is that they want to require an end-user to click on one of the buttons... and other forms of dismissal such as pressing the Back key or tapping outside of the dialog should be ignored.

So, instead of calling it cancelable, how about we call it buttonInputRequired or buttonPressRequired?

@hansemannn, @garymathews, @ypbnv, your thoughts?

@hansemannn
Copy link
Collaborator

+1 for buttonPressRequired, unfortunately this is not an API that exists on iOS.

@ypbnv
Copy link
Contributor Author

ypbnv commented Jul 4, 2018

I can see that being misleading, yes. I did not think about it. Updating it to buttonPressRequired.

@@ -289,6 +289,7 @@ public void onCancel(DialogInterface dlg)
dialog = getBuilder().create();
dialog.setCanceledOnTouchOutside(
proxy.getProperties().optBoolean(TiC.PROPERTY_CANCELED_ON_TOUCH_OUTSIDE, true));
dialog.setCancelable(proxy.getProperties().optBoolean(TiC.PROPERTY_BUTTON_PRESS_REQUIRED, true));
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value should be false to retain the old behavior.

You should also ! the property's returned value since buttonPressRequired set to true means it is not cancelable via back key or outside taps.

type: Boolean
default: true on Android
since: "7.4.0"
platforms: [android]
Copy link
Contributor

Choose a reason for hiding this comment

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

Default value should be false to retain the old behavior.

And how about we change the summary to...

Setting this to true requires the end-user to click a dialog button to close the dialog.

And perhaps the following description...

Set to true to prevent the dialog from being dismissed via back navigation or tapping outside of the dialog. This requires the end-user to click on one of the dialog buttons provided by property buttonNames. Note that if the dialog does not have any buttons, then the dialog can only be closed programmatically via the hide() method.

@jquick-axway
Copy link
Contributor

Thanks @ypbnv . Just a bit more feedback and I think we're done. :)

/**
* @module.api
*/
public static final String PROPERTY_BUTTON_PRESS_REQUIRED = "buttonPressRequired";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we name this buttonClickRequired to coincide with the click event, to prevent confusion with the pressed event?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I'm open to different names. Anyone else have an opinion?

Invert default value to keep behavior.
Change the summary and add a description.
@ypbnv
Copy link
Contributor Author

ypbnv commented Jul 11, 2018

Updated with the recent suggestions. BTW should accessing this property before giving it any value return a valid result. Now it is returning undefined. That is the case for properties already in the API like persistent and canceledOnTouchOutside.

@jquick-axway
Copy link
Contributor

Thanks for the recent changes @ypbnv! :)

BTW should accessing this property before giving it any value return a valid result? Now it is returning undefined.

Ideally, it should return the default value we document, but I see what you mean. None of the other properties return a default value either. I'm okay with the way it is for now. We can look into improving things later.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Jul 18, 2018

FR Passed.

The buttonClickRequired property works as expected at creation & later.

Studio Ver: 5.1.0.201807090554
SDK Ver: 7.4.0 local build
OS Ver: 4.2.13
Xcode Ver: 7.0.4
Appc NPM: 1.1.3
Appc CLI: 10.13.5
Daemon Ver: Xcode 9.4.1
Ti CLI Ver: 5.1.1
Alloy Ver: 1.12.0
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.1
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Nexus 6P (Android 8.1.0)
Emulator: Android 4.1.2

@build
Copy link
Contributor

build commented Jul 18, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@lokeshchdhry lokeshchdhry merged commit 1d1932d into tidev:master Jul 18, 2018
@hansemannn hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants