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

Call idle timer changes on main thread #1331

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jonnyijapan
Contributor

jonnyijapan commented Sep 13, 2018

@digitall

This comment has been minimized.

Member

digitall commented Sep 14, 2018

These proposed code changes look fairly self-explanatory and reasonable given the described bugs and are limited to the IOS port so this looks good.

However, the commit messages do not conform to our http://wiki.scummvm.org/index.php/Commit_Guidelines so I don't want to merge this as-is. Also, the extra merge commit is not required.

I will cherry pick these commits, amend the commit messages and rebase them onto the master branch shortly to close this out.

@jonnyijapan : Thanks for providing this code fix.

@digitall

This comment has been minimized.

Member

digitall commented Sep 14, 2018

Ah, just realised that the launch fix commit has already been applied as PR #1328 , though the commit message was not fixed. Not amending history with force-push so will leave that as-is, but amend the newer idle timer fix.

@jonnyijapan : Suggest a clean branch and cherry-pick / rebase for PR if possible in future.

@digitall

This comment has been minimized.

Member

digitall commented Sep 14, 2018

Committed as 58f3aac ... Closing.

@digitall digitall requested a review from criezy Sep 14, 2018

@digitall digitall closed this Sep 14, 2018

@criezy

This comment has been minimized.

Member

criezy commented Sep 14, 2018

@digitall You didn't leave me much time to review before merging :-P
Or was asking for my review a misclick?
In any case the change seems fine to me and I see you fixed the commit message when cherry-picking. And buildbot seems happy with it as well. So it''s all good.

@digitall

This comment has been minimized.

Member

digitall commented Sep 14, 2018

@criezy : Yes, sorry about that. It was intentional. The code change was logical given the described issue so I was happy to merge and close, but since I don't have any IOS devices to test with and not the OSX/IOS porter, I thought that you should take a look for "second opinion" to make sure I hadn't just broken older IOS builds or something. :) Anyway, will put a comment to explain in future :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment