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-26284] Android: Cannot add child-views to views extending TiUIFragment #10245

Merged
merged 16 commits into from Oct 1, 2018

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Aug 10, 2018

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

Optional Description:
Children added before the fragment transaction of the component are left under the fragment. These additions handle them and draws them once the fragment transaction has been committed.

Note: Currently this takes care of only add() method. Dealing with positions for insertAt() will be added once we clear if it needs reworking.

Do we want any special treatment for TiUIFragment implementations?

Test case:
This requires a Ti.Map module built against a SDK with these changes merged.
app.js

var Map = require('ti.map');
var win = Titanium.UI.createWindow();
var mapview = Map.createView();
mapview.add(Ti.UI.createView({ width: 50, height: 50, backgroundColor: 'red'}));
win.add(mapview);
win.open();

I have to think a bit for a possible unit test.

@ypbnv ypbnv added this to the 7.4.0 milestone Aug 10, 2018
@build
Copy link
Contributor

build commented Aug 10, 2018

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.

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn hansemannn changed the title [TIMOB-26284] Android: Cannot add child-views to views extending UIFragment [TIMOB-26284] Android: Cannot add child-views to views extending TiUIFragment Aug 10, 2018

public void realizeFragmentViews()
{
for (TiUIView tiUIView : childrenToRealize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could we rename tiUIView into child ?

childrenToRealize.add(child);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@ypbnv, we also need to override the insertAt(TiUIView, int) and remove(TiUIView) methods. It's an unlikely edge-case, but it should be easy to add support, no?

For example, see our TiUIScrollView code here...
https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/ui/src/java/ti/modules/titanium/ui/widget/TiUIScrollView.java#L1065

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought about insertAt(), but what I found out took the focus at that moment. I have described it in here:
https://jira.appcelerator.org/browse/TIMOB-26283
Let me know what Gary and you think about that. If insertAt() needs a fix it will be better to do it for the source before overriding it here.

I will override remove() , too.

@hansemannn hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
@ypbnv
Copy link
Contributor Author

ypbnv commented Sep 5, 2018

@jquick-axway I tried a few approaches about insertAt() and this update seems to not break anything else and consist of at least overriding as I was able to do. For remove() I was not able to time a test properly to break an application without the override, but the way it is now it should prevent drawing views that are not supposed to be there.

I have also updated the test case.

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

FR Passed.

add & insertAt methods works as expected & the child views are properly displayed.

Studio Ver: 5.1.1.201809051655
SDK Ver: 7.5.0 local build
OS Ver: 10.13.5
Xcode Ver: Xcode 9.4.1
Appc NPM: 4.2.13
Appc CLI: 7.0.6
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.2
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Nexus 6P (Android 8.1.0)
Emulator: ⇨ Android 4.1.2

@ypbnv
Copy link
Contributor Author

ypbnv commented Sep 21, 2018

@jquick-axway , @garymathews I couldn't come up with a proper unit test for that. On the JS everything looks fine so I wasn't able to write a test that is failing before these changes. Do you have any ideas?

@jquick-axway
Copy link
Contributor

@ypbnv, perhaps we should keep it simple and do a unit test like the ScrollView "add-insert-remove" one shown below. It doesn't wait for the window to open. It just verifies that you can add/remove child views.
https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.ui.scrollview.test.js#L165

@sgtcoolguy
Copy link
Contributor

FWIW, I tried writing a test like the one Josh pointed out, but it doesn't fail without the changes in this PR. I couldn't come up with a great way to test this either, short of an Appium UI test in our integration suite. I think we're ok to merge as-is.

@sgtcoolguy sgtcoolguy merged commit 544f322 into tidev:master Oct 1, 2018
build pushed a commit to hansemannn/titanium_mobile that referenced this pull request Oct 1, 2018
…Fragment (tidev#10245)

* Handle child-views added before the fragment transaction commit.

* Fix formatting.

* Override insertAt() and fix realizeViews for multiple views.

* Ensure childred are removed from childrenToRealize on remove() call.

* Fix formatting.
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