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

fix(android): fix reusing a dialog with a new "parent" window #11073

Merged
merged 3 commits into from Aug 20, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Jul 23, 2019

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

Description:
Reset the Builder instance once the Dialog has been shown in order to allow reusing the proxy with different contexts.
Checking if we can have a reliable unit test for that.

Test case:
app.js

var dialog = Ti.UI.createAlertDialog({
    title: "Alert Dialog",
    message: "This is the alert message.",
    buttonNames: ["OK", "Cancel"],
    cancel: 1,
});

var window = Ti.UI.createWindow();
var button = Ti.UI.createButton({ title: "Show Alert" });
button.addEventListener("click", function(e) {
    var childWindow = Ti.UI.createWindow({ title: "Child Window" });
    childWindow.addEventListener("open", function() {
        function dialogEventHandler() {
            dialog.removeEventListener("click", dialogEventHandler);
            dialog.removeEventListener("cancel", dialogEventHandler);
            childWindow.close();
        }
        dialog.addEventListener("click", dialogEventHandler);
        dialog.addEventListener("cancel", dialogEventHandler);
        dialog.show();
    });
    childWindow.open();
});
window.add(button);
window.open();

@ypbnv ypbnv added this to the 8.2.0 milestone Jul 23, 2019
@ypbnv ypbnv requested a review from jquick-axway July 23, 2019 08:19
@build build requested a review from a team July 23, 2019 08:45
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

@jquick-axway
Copy link
Contributor

Ahh... it was a variable scope issue. Thanks @ypbnv.

Issues like this is exactly why devs prefer to prefix member variables with m_* or m* or this.*. It avoids scoping bugs just like this. (sigh)

@ssekhri
Copy link

ssekhri commented Jul 30, 2019

FR Passed. Dialog re-use works successfully.
Verified on:
Mac OS 10.14.3
Ti SDK: 8.2.0.v20190723012209
Appc CLI: 7.1.0-24
Node: 8.16.0
JDK: 10.0.2
Xcode: 10.2.1
Studio: 5.1.3.201907112159

@ssekhri
Copy link

ssekhri commented Jul 30, 2019

@ypbnv kindly back-port to 8_1_X branch

@build
Copy link
Contributor

build commented Aug 19, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 4344 tests are passing.
(There are 472 tests skipped)

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against 23aa55e

@sgtcoolguy sgtcoolguy merged commit 0492a24 into tidev:master Aug 20, 2019
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

5 participants