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-18500] Android: event.cancel not set properly for optionsDialog #9476

Merged
merged 7 commits into from Nov 13, 2017

Conversation

maggieaxway
Copy link
Contributor

@maggieaxway maggieaxway commented Sep 25, 2017

Update TiUIDialog.java

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

To test by clicking every option and button. Click outside dialog or back for cancel.

Test case:

var win = Ti.UI.createWindow({
    backgroundColor: 'white',
    exitOnClose: true,
});

var opts = {
    title: 'Delete File?'
};

var isAndroid = Ti.Platform.osname == 'android';

if(isAndroid){
    opts.options = ['Confirm', 'Cancel'];
    opts.buttonNames = ['Help'];
    //opts.Help = 0;
} else {
    opts.options = ['Confirm', 'Help', 'Cancel'];
}

var dialog;
win.addEventListener('click',function(){
    dialog = Ti.UI.createOptionDialog(opts);
    dialog.show();
    dialog.addEventListener('click', onSelectDialog);
    dialog.addEventListener('cancel', function(event){
        alert("Dialog canceled " + " event.cancel = " + event.cancel+ " event.index = " +event.index);
    })
});

function onSelectDialog(event){
    if(isAndroid){

        if(event.button === false && event.index === 0) {
            console.log(event.index + "fired");
            alert("Confirm option selected event.index= " + event.index);
        }

        if(event.button === false && event.index === 1) {
            console.log(event.index + "fired");
            alert("Cancel option selected event.index= " + event.index);
        }

        if(event.button === true && event.index === 0) {
            console.log(event.index + "fired event.index = " + event.index);
            alert("Help Button clicked event.index= " + event.index);

        }
    }

}

win.open();

@maggieaxway maggieaxway added this to the 6.3.0 milestone Sep 25, 2017
@build
Copy link
Contributor

build commented Sep 25, 2017

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

🚫

Tests have failed, see below for more information.

Tests:

Classname Name Time Error
android.Titanium.Network.HTTPClient clearCookieUnaffectedCheck 60.061 Error: timeout of 60000ms exceeded

Generated by 🚫 dangerJS

}
data.put(TiC.EVENT_PROPERTY_INDEX, id);
data.put(TiC.PROPERTY_CANCEL, id == cancelIndex);
fireEvent(TiC.EVENT_CLICK, data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do this to prevent duplicate code:

boolean isCancel = id == cancelIndex;
...
data.put(TiC.PROPERTY_CANCEL, isCancel);
if (isCancel) {
    fireEvent(TiC.EVENT_CANCEL, data);
} else {
    fireEvent(TiC.EVENT_CLICK, data);
}

@garymathews
Copy link
Contributor

Also, remember to put the ticket number in the title of the PR and the commit message. 👍

@garymathews garymathews changed the title Fix Android: event.cancel not set properly for optionsDialog [TIMOB-18500] Android: event.cancel not set properly for optionsDialog Sep 25, 2017
@hansemannn hansemannn modified the milestones: 6.3.0, 7.0.0 Sep 26, 2017
@hansemannn
Copy link
Collaborator

Changed fix-version from 6.3.0 to 7.0.0. Please do a backport to 6_3_X once this PR is approved and label that with 6.3.0, so we have all versions aligned. Thanks!

@lokeshchdhry
Copy link
Contributor

FR Passed.

Right event.index is returned for every option.

Studio Ver: 4.10.0.201709271713
SDK Ver: 7.0.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.10
Appc CLI: 6.3.0
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.7
Node Ver: 7.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 6P --- Android 8.0.0
⇨ google Nexus 5 --- Android 6.0.1

@lokeshchdhry
Copy link
Contributor

@maggieaxway , can you please take a look at the unit test failures so that merge can be enabled.

@eric34 eric34 merged commit 1cc2aa1 into tidev:master Nov 13, 2017
@mukherjee2
Copy link
Contributor

@lokeshchdhry @eric34 , I'm closing tickets, and noticed that there's no CR approval for this PR.

@mukherjee2
Copy link
Contributor

@garymathews would you be able to do a CR?

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

7 participants