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-7673] "replaceAt", "insertAt", and "getAt" view-like UI components #5109

Merged
merged 14 commits into from Apr 17, 2014

Conversation

pec1985
Copy link
Contributor

@pec1985 pec1985 commented Dec 10, 2013

Details in Jira ticket
Sample code on comment

@@ -556,6 +566,106 @@ public void add(TiViewProxy child)
//TODO zOrder
}

@SuppressWarnings("unchecked")
@Kroll.method
public void replaceAt(Object params)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you know that the input is going to be of type HashMap<String,Object> why not use KrollDict directly instead of adding SuppressWarnings markers?

@vishalduggal
Copy link
Contributor

There is no actual need for the getAt method since we expose the chilren property. You should only implement replaceAt and insertAt

@vishalduggal
Copy link
Contributor

This would also be a good time to refactor the iOS code to not use two arrays (chilren and pendingAdds) and instead use a single array like iOS.

@vishalduggal
Copy link
Contributor

Documentation missing. This ticket is also marked as TiAPI. So please create parity tickets for BB and MW

@vishalduggal
Copy link
Contributor

Code Reviewed. Please address comments

public void insertAt(TiUIView child, int position)
{
add(child, position);
if(children.contains(child)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The if block is not required. Instead refactor the add method to insert child at correct index

@vishalduggal
Copy link
Contributor

Code looks ok. One minor fix. Will continue with FR once addressed.

@@ -224,60 +245,21 @@ -(void)remove:(id)arg
ENSURE_UI_THREAD_1_ARG(arg);

pthread_rwlock_wrlock(&childrenLock);
if ([children containsObject:arg])
{
NSMutableArray* childrenCopy = [children mutableCopy];
Copy link
Contributor

Choose a reason for hiding this comment

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

This array got leaked since it is never released. Either use [self children] outside the locks or release this object at end of method

@vishalduggal
Copy link
Contributor

Code reviewed. 2 memory leaks. Otherwise ok. Starting functional.

@vishalduggal
Copy link
Contributor

FR Passed. Ran test case and KS. Will merge once memory leaks are addressed.

vishalduggal added a commit that referenced this pull request Apr 17, 2014
[TIMOB-7673] "replaceAt", "insertAt", and "getAt" view-like UI components
@vishalduggal vishalduggal merged commit 9acd6b2 into tidev:master Apr 17, 2014
farfromrefug pushed a commit to Akylas/titanium_mobile that referenced this pull request Aug 15, 2014
[TIMOB-7673] "replaceAt", "insertAt", and "getAt" view-like UI components
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants