EventServer - Correct unicode handling #1612

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

Tolriq commented Oct 13, 2012

@jmarshallnz / @Montellese

As talked in #1475 a simple patch to correct EventServer so it correctly handles Unicode.

This is the smallest way with 0 impact of doing this.

Owner

Montellese commented Oct 13, 2012

As I stated a few times I don't really know much about all that keyboard handling stuff and I'm sure @jhsrennie would be able to shed some more light on this. But your patch doesn't look to me like "0 impact" because there are quite a few places that rely on the fact and check against keycode being smaller than KEY_VKEY which up until now was always the case but won't be anymore with your change. Furthermore there's at least which does a check on (code & KEY_VKEY) because KEY_VKEY is meant to be used as a flag and not an operator in an arithmetical calculation.

Maybe we just need to change KEY_VKEY from 0xF000 to 0xF0000000 as we need to change from 16bit to 32bit anyway?

Contributor

Tolriq commented Oct 13, 2012

Well it does have 0 impact on existing clients :) and does reproduce the old httpapi way of functioning.

I tried to open a discussion on this many many times. The last answer from @jhsrennie at http://trac.xbmc.org/ticket/12678 was drop EventServer.

Since this have been around for 9 month anticipating http api drop and we are in feature freeze I choose the simplest path to have a chance to not lose something Yatse use and that have no reason to not continue working for Frodo.

Owner

Montellese commented Oct 13, 2012

While it may have 0 impact on existing eventserver clients it may have impact on other input methods (e.g. a keyboard) which are directly handled by XBMC. As I said I'm absolutely no expert in this area, I'm just pointing out that there are places that rely on KEY_VKEY to be used like a flag which is not the case for a keycode coming from EventServer with this patch.

Contributor

Tolriq commented Oct 13, 2012

No not possible :)

I only change things that can only comes from EventClient, no change to anything that can have impact when not coming from EventServer.

Furthermore since EventServer packet parsing can't get more than a ushort there's also no possible way to generate a false VKEY flag for existing clients.

I really choose the simplest path for no impact so normally no reason to refuse this simple correction even if not the best rewrite the eventserver may need for future.

Member

jmarshallnz commented Oct 13, 2012

Where is the button code you pass in converted to something actually useful in the UI? As far as I can tell from a quick scan, CAction/CKey::m_unicode never gets set through the event server path, and that's what we use in the virtual keyboard and edit controls.

It would be useful for those of us not used to this insane maze and don't have an event client handy if you would trace through from the event client -> event server -> key/action -> whatever processes that action and clearly describe exactly how things are supposed to work.

It seems to me that the correct fix is adding a unicode packet to the client + server, then just setting that using the alternate CKey constructor.

Contributor

Tolriq commented Oct 13, 2012

Well as I said there's multiple way to correct this but since I can't have any discussion before submitting a patch I checked how it works and submitted the simplest possible patch. (And I must admit that this is a .... maze )

To send direct keypress we need to send a eventclient packet of type PacketButton.

To have it processed correctly in Application::onKey we need to pass the FromKeyboard that check that m_ButtonCode is > KEY_VKEY. (And will then use the m_unicode.

Remark 1 : the onKey check if the fromService flag is set and handle special case for those.

The onKey is called from Application::ProcessEventServer for EventServer.
This one fetch ButtonCode from que ES queue then create the correct CKey.

Since this comes from a service the cKey::SetFromService is called this is the one that check the 0xF100 flag and set the m_unicode. (It set it from the m_Button code - the flag).

The ES queue is filled with CEventButtonState that does not handle unicode or such so only the bcode with the flag set or not.

The queue is filled by OnPacketBUTTON that can have a special flag PTB_VKEY that will then generate the buttoncode by adding the flag.

This is more or less how EventServer works for key-presses. (I just changed the | flag to a + flag to not loose data since it's the way it was working for old HTTPApi).
The only drawback is that of course unicode value added to 0xF000 is bigger than a unsigned short so needs to change to uint.

The solution you propose is the way I proposed 10 month ago, just adding a new flag for Button Packet then adding a flag to CEventStateButton and finally call the correct CKey constructor.

But this include new feature and since I fight for long time for this I was pretty sure that it would have been postponed to after Frodo killing one feature of Yatse.

Edit : If you agree to possibly add the better way to Frodo I can update this PR to this correct way without any problems :)

Member

jmarshallnz commented Oct 13, 2012

SetFromService(true) sets m_unicode to the buttoncode - 0xf100 as long as buttoncode & 0xf100 is true.

However, you seem to be setting buttoncode + 0xf000 in the event client, which won't guarantee that 0xf100 is true at all?

Where does 0xf100 get OR'd onto your buttoncode?

Exactly what buttoncode do you send from your event client?

Contributor

Tolriq commented Oct 13, 2012

i ve just change the | to a + not the rules :(

the 0x100 is set from the client since there's no flag to set it in eventserver actually.

Member

jmarshallnz commented Oct 13, 2012

So what you do, is take (unicode | 0x100) and pass that with PTB_VKEY set. The EventClient code then takes that and or's with 0xf000 to give (unicode | 0xf100) which is then (unicode | KEY_ASCII) and everything works.

However, with your change, this breaks: (unicode | 0x100) + 0xf000 does not necessarily mean that the bits 0xf100 are set. From what I can see, you should be able to send any "unicode" that doesn't set those 5 bits already.

What I suggest is defining a new mask that you set (0x80000000 seems reasonable) when PTB_VKEY is set. Then make the button code a uint32_t inside the EventClient and pass that through to the event server. Then, in CApp::ProcessEventServer, check for that mask and if set, pass the key directly to the CKey constructor (4th one in the header) that takes in the unicode value. i.e. exactly how the keyboard does it. You won't have the ascii code or modifiers etc. (just set them to 0) but that shouldn't be used anyway for direct keyboard input.

Contributor

Tolriq commented Oct 14, 2012

Updated to PR to the solution I suggested long ago, I added a new flag instead of using the existing PTB_VKEY to not break compatibility with old clients that adds the 0x100 on their side.

I hope this won't be considered a new feature and can still be merged for Frodo :)

Member

jmarshallnz commented Oct 14, 2012

  1. Don't put the define in Key.h - XBMC shouldn't need it, only the eventclient/Application.h need it. Just define it inside the event client header should do.
  2. Remove the FromService stuff and just call the correct CKey() constructor from CApplication::ProcessEventServer() when you have that flag present.
  3. Fixup the whitespace + coding convention (braces on separate lines).

Otherwise, IMO this looks fine.

Contributor

Tolriq commented Oct 14, 2012

Update PR.

No other choice than moving the if apart since the SetFromService would kill the value and I can't remove totally the call to this to avoid break of old clients.

Member

jmarshallnz commented Oct 14, 2012

Looks good. 2 more things and we're done :)

  1. Name the define KEY_UNICODE - it's not related to virtual keys as it's an actual unicode character, right?
  2. Squash down ready for merge.
Contributor

Tolriq commented Oct 14, 2012

Hum sorry new to git no idea what you mean about 2 :)

Edit : and forgot but I named it like because KEY_UNICODE is already defined :(
#define KEY_UNICODE 0xF200

Since in the final it's only a flag why not FLAG_UNICODE ?

Member

jmarshallnz commented Oct 14, 2012

Sure - FLAG_UNICODE is fine. You can also prefix it by ES for event server if you want to save it clashing.

For squashing, in your branch do the following:

git rebase -i HEAD~8

Then follow the instructions in your text editor - use s for squashing the commit into the previous one. Once the rebase is done, you'll get to edit the commit message. Note: The HEAD~8 here is just "rebase the last 8 commits from the top (HEAD)".

Contributor

Tolriq commented Oct 14, 2012

Hope I did good :)

Did not prefix since the FLAG is defined in EventClient Namespace.

@ghost ghost assigned jmarshallnz Oct 14, 2012

Member

jmarshallnz commented Oct 14, 2012

If it's a define then it's global. Either make it an static const unsigned int (whereby you query it using EVENTCLIENT::<name_of_member>) or prefix it please.

The squash looks perfect. Now you can learn something new: To amend the top-most commit, make your changes, git add the files then git commit --amend.

Contributor

Tolriq commented Oct 14, 2012

This one I know :p

Git is not very easy for basic SVN users :p but really powerful :)

xbmc/Application.cpp
@@ -3210,6 +3210,12 @@ bool CApplication::ProcessEventServer(float frameTime)
else
{
CKey key;
+ if(wKeyID & ES_FLAG_UNICODE)
+ {
+ key = CKey((uint8_t)false, wKeyID|ES_FLAG_UNICODE, KEY_ASCII, 0, 0);
@jmarshallnz

jmarshallnz Oct 14, 2012

Member

The KEY_ASCII should be 0 I think - you're not passing an ascii value in, and this is supposed to take a char anyway (KEY_ASCII is 0xf100 so all you're doing here is setting it to 0...)

@jmarshallnz

jmarshallnz Oct 14, 2012

Member

Also, the false should be 0, and you can drop the cast to unit8_t

@Tolriq

Tolriq Oct 14, 2012

Contributor

KEY_ASCII is needed because later on the code does a | KEY_ASCII that will generate bad key data :(
(if (key.GetUnicode())
action = CAction(key.GetAscii() | KEY_ASCII, key.GetUnicode());) in Application::OnKey

I can change the false to 0 but if I remove the cast then the compiler does not know witch constructor to use, so no choice I'm aware off (I'm not good at C )

@jmarshallnz

jmarshallnz Oct 14, 2012

Member

Ok, you can leave the cast in, but make the false 0.

You definitely don't need KEY_ASCII though - it's being passed to a char (8bits), and it's not completely obvious what the result will be (I think it'll be 0, but not 100). Thus, when it's OR'd with KEY_ASCII in the code you mention, you still get >= KEY_ASCII anyway.

Almost there :)

xbmc/Application.cpp
@@ -3210,6 +3210,12 @@ bool CApplication::ProcessEventServer(float frameTime)
else
{
CKey key;
+ if(wKeyID & ES_FLAG_UNICODE)
+ {
+ key = CKey((uint8_t)0, wKeyID|ES_FLAG_UNICODE, KEY_ASCII, 0, 0);
@Tolriq

Tolriq Oct 14, 2012

Contributor

Your message is dropped but I don't know why and how but if I don't put KEY_ASCII here it does not work :(
So there's definitively something that is passed to the CAction later that cause problem.

@jmarshallnz

jmarshallnz Oct 14, 2012

Member

You don't need to | with ES_FLAG_UNICODE either (it's already there).

It's OK to leave the KEY_ASCII there - I'll figure out what's happening once it's in and clean it up.

BTW: If you're on IRC, this would be way faster (#xbmc on freenode), plus, we wouldn't be spamming 100+ people with every little note :)

Member

jmarshallnz commented Oct 15, 2012

Pushed in slighty modified in 4a54dd0

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