Code review comment for lp:~wengxt/nux/fcitx-support

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Hello, sorry for be so late to review this....

There is something wrong with restarting IMs in the XIM part. ie. When you restart hime the text entry does not re initialize the xim again which leaves no im working (Only for XIM). Though that is an edge case. In the XIM destructor a XDestroyIC (xic) and XCloseIM(xim) should be there...

Also note this XIM support will not work in Unity. This is because the window getting set for the XIC is the root window for compiz. So when events come through for say the Dash window and get sent through XFilterEvent it will not match the compiz window which the XIC is setup with and fail.

I am also not sure if the XIM support will fit in this way for unity. As we will need an XIC for each window (Dash/Hud). So a possible fix for the XIM part is get the window related to the TextEntry and send that through when focusing the TextEntry...

Seems to be the way to get the window relating to a focused TextEntry.
nux::BaseWindow* window = static_cast<nux::BaseWindow*>(GetTopLevelViewWindow()))

I want to add this Fcitx support in as it communicates directly with fcitx as opposed to going through X. I think the more IMs that get added the harder it will be to maintain. I am really considering dropping all friend classes for the IM stuff and adding public methods into TextEntry to handle anything the IM stuff needs. I don't want to ruin encapsulation for a few protected variables. This will also reduce code in OnCommit for both ibus/fcitx and should for other things. Though this is my fault for programming it like this in the first place...

So right now...we need test for the fcitx support. Preferably in both Unity and Nux, but getting test in for Nux should be fine. Take a look at xtest-text-entry.cpp, this is where we have test for ibus. (It should be renamed to xtest-text-entry-ibus.cpp)

I also wanted to apologize for forcing IBus in Unity. It was not something I wanted to do and I really want to make sure everyone can use any IM they wish.

One last thing...we are in a FF (Feature Freeze) for 12.10. This being said I am not 100% if we can get a FF exception for this branch. If we cannot then this well get held back until 13.04.

Thank you again for this branch :)

review: Needs Fixing

« Back to merge proposal