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

python interface now can use native keyboard (ios currently) if possible. #2101

Merged
merged 6 commits into from Feb 2, 2013

Conversation

ulion
Copy link
Contributor

@ulion ulion commented Jan 22, 2013

I'm glad to see native keyboard feature in Frodo currently, it comes from #1194, but the Keyboard in python interface still wasn't benefit from the feature.
This little commit, just enable use native keyboard feature in python interface if possible.
since the python addon is running in standalone thread other than the main xbmc thread, there is a little adjust need to be set to let the ios keyboard works, which include:

  1. in the input callback in the keyboard factory, use async SendGUIMessage instead of direct call win->OnMessage() or windowsmanager->SendMessage().
  2. addon instance running without an autopool for object-c by default, then an autopool is added in the ioskeyboard main function.
  3. in ApplicationMessenger, the SendGUIMessage method take default window id param and then send 0 to windowsmanager->SendMessage(msg, winid), but the win manager does not handle the winid 0 condition, since SendGUIMessage in the project is not used so many times, I checked and fixed it to redirect to windowsmanager->SendMessage(msg) if winid==0

@jmarshallnz
Copy link
Contributor

Thanks for the patch. Please split the commit up into a single change per commit (i.e. "changed: CApplicationMessenger::SendGUIMessage to handle case where winid == 0", "add autopool if none available" etc.)

git reset HEAD~1, git add -p

@ulion
Copy link
Contributor Author

ulion commented Jan 22, 2013

well, splited, and I also have another non-finished commit I'm working on it for use ios native UITextField control to input, which can input any language chars supported by ios native input method, and support clipboard copy and paste, also will include tap non-input region to cancel feature just like the generic keyboard does.

@Memphiz
Copy link
Member

Memphiz commented Jan 22, 2013

Nice work - does it still work with the common keyboard dialog on non ios?

Beside that i'm really looking forward to your change by using the UITextField on ios - i was not able to figure it out and the current approach just looks a bit ugly on ios :D

Thx for working on that front :)

@ulion
Copy link
Contributor Author

ulion commented Jan 22, 2013

the original great work is from #1194, the ios native keyboard will be used for most cases on ios, except for the python addons. on other platforms it still showing the original generic keyboard.

@Memphiz
Copy link
Member

Memphiz commented Jan 22, 2013

Ahh right - i heard of the guy who did this :D - just wanted to make sure that its still working outside of ios with your patches ;)

@ulion
Copy link
Contributor Author

ulion commented Jan 22, 2013

it's still working on non-ios, certainly, ios keyboard only used on ios, else generic keyboard window instance is used.
this can be checked in KeyboardFactory::ShowAndGetInput method, where is the only place IOSKeyboard instance is created.

@Memphiz
Copy link
Member

Memphiz commented Jan 26, 2013

Since this is part of #2105 i have tested this with the youtube addon and it works like a charm (native keyboard is used in addon settings there). Nice :)

@Memphiz
Copy link
Member

Memphiz commented Jan 26, 2013

This has to go in before #2105

@Memphiz
Copy link
Member

Memphiz commented Jan 30, 2013

@jmarshallnz are you ok with that?

@jmarshallnz
Copy link
Contributor

Overall it looks fine once the inline comments are taken care of.

Does the factory not support autoclose?

@ulion
Copy link
Contributor Author

ulion commented Jan 31, 2013

factory not support autoclose, yet.

@Memphiz
Copy link
Member

Memphiz commented Jan 31, 2013

@ulion could you rebase and adapt to master please? Factory has the autoclose added now ;) - thx for your patience.

@ulion
Copy link
Contributor Author

ulion commented Feb 1, 2013

rebased, and last 3 commits were changed from the first request.

@Memphiz
Copy link
Member

Memphiz commented Feb 1, 2013

nice cleanup - i'm fine with it

@ulion
Copy link
Contributor Author

ulion commented Feb 1, 2013

so far so good, but I found we have to handle the Cancel() from the timer thread at any time, which makes it not so easy to write strong code. I will handle the Cancel() in more thread-safe way, if I can figure it out.

@ulion
Copy link
Contributor Author

ulion commented Feb 2, 2013

finally, I got the Cancel() call thread-safe.

@Memphiz
Copy link
Member

Memphiz commented Feb 2, 2013

looks good - but why that bool pointer - instead just call a function setcanceled in the keyboardview which sets a normal bool member

@ulion
Copy link
Contributor Author

ulion commented Feb 2, 2013

when Cancel() get called, we really don't known whether keyboardView pointer is valid and where the keyboard thread is running to, there could be some race-conditions. so finally I let the Cancel() just set a flag, and let the keyboard thread do the check cancel flag work.
share one common cancel flag make the Cancel() code really simple, and the whole keyboard thread and logic also simple to understand. if we got two cancel flag, it will be more hard to understand how can it safely works.

@Memphiz
Copy link
Member

Memphiz commented Feb 2, 2013

@jmarshallnz since i don't see your inline comments - can you confirm that those are fixed? If so i'm fine enough with it to pull the trigger...

jmarshallnz added a commit that referenced this pull request Feb 2, 2013
python interface now can use native keyboard (ios currently) if possible.
@jmarshallnz jmarshallnz merged commit 3e048f8 into xbmc:master Feb 2, 2013
@ulion ulion deleted the python_use_native_keyboard branch February 17, 2013 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants