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-23341] Exception on iPhone: no row found for index. #7992

Closed
wants to merge 1 commit into from

Conversation

TiBaharroth
Copy link
Contributor

@TiBaharroth TiBaharroth commented May 6, 2016

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

Fix the bug : insert in the insertRowBefore and insertRowAfter

if (![NSThread isMainThread]) {
        TiThreadPerformOnMainThread(^{[self insertRowBefore:args];},YES); // YES = wait for the operation to complete on the main thread !!!
        return;
}

Explaination :
'The problem here is actually very simple, but it took me few hours to find this out. JS engine itself runs in async context, which means that if one is inserting multiple rows in JavaScript (e.g. in a loop) using insertRowBefore then the obj-c code behind this does the work in async (non-UI) thread. I think the reason the ENSURE_UI_THREAD(insertRowBefore,args); line was commented is becase the actual action happens in UI thread anyway - see how [table dispatchAction:action] works (TiUITableViewProxy.m around line 535).

The code that checks the row indexes and verifies against existing list still runs asynchronously and thus if you run inserting rows in loop this can have unpredictable results because the list might not yet be updated, etc.. So the solution here is to synchronize the insertion and wait for it’s completion. Finally the commented code mentioned by Dave Cadwallader should be replaced with this:'

From https://archive.appcelerator.com/question/129708/obj-c-fix-for-error-on-tableview-insertrowbefore--insertrowafter

…iewProxy insertRowAfter

JIRA: https://jira.appcelerator.org/browse/AC-2289

Fix the bug : insert in the insertRowBefore and insertRowAfter
if (![NSThread isMainThread]) {
        TiThreadPerformOnMainThread(^{[self insertRowBefore:args];},YES); // YES = wait for the operation to complete on the main thread !!!
        return;
}

Explaination : 
'The problem here is actually very simple, but it took me few hours to find this out. JS engine itself runs in async context, which means that if one is inserting multiple rows in JavaScript (e.g. in a loop) using insertRowBefore then the obj-c code behind this does the work in async (non-UI) thread. I think the reason the ENSURE_UI_THREAD(insertRowBefore,args); line was commented is becase the actual action happens in UI thread anyway - see how [table dispatchAction:action] works (TiUITableViewProxy.m around line 535).

The code that checks the row indexes and verifies against existing list still runs asynchronously and thus if you run inserting rows in loop this can have unpredictable results because the list might not yet be updated, etc.. So the solution here is to synchronize the insertion and wait for it’s completion. Finally the commented code mentioned by Dave Cadwallader should be replaced with this:'

From https://archive.appcelerator.com/question/129708/obj-c-fix-for-error-on-tableview-insertrowbefore--insertrowafter
@hansemannn
Copy link
Collaborator

Should be obsolete in 5.4.0 and greater, since we use the main thread execution by default. Can you test the issue with 5.4.0 and verify it works without the fix?

@hansemannn hansemannn changed the title [AC-2289] Exception on iPhone: no row found for index. [TIMOB-23341] Exception on iPhone: no row found for index. May 8, 2016
@cheekiatng
Copy link
Contributor

in 5.4.0, although the main thread is set by default for new projects, devs can still enable it on/off, so this error must not happen in both scenarios. (main thread on / off)

@TiBaharroth
Copy link
Contributor Author

Sorry, i didn't try with SDK 5.4.0, i will try asap but i think it will fix with this version

@hansemannn
Copy link
Collaborator

Thank you @TiBaharroth, looking forward to that. We will schedule it for 6.0.0.

@build
Copy link
Contributor

build commented Jun 16, 2016

Can one of the admins verify this patch?

@hansemannn
Copy link
Collaborator

@TiBaharroth Can you confirm that it works on 5.4.0 and 6.0.0? I would appreciate!

@TiBaharroth
Copy link
Contributor Author

Can't reproduce the issue with 5.4.0

So i think it's OK :)

@hansemannn
Copy link
Collaborator

Thanks! Closing for now.

@hansemannn hansemannn closed this Jul 25, 2016
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

4 participants