Nux

Merge lp://staging/~brandontschaefer/nux/xim-tests into lp://staging/nux

Proposed by Brandon Schaefer
Status: Merged
Approved by: Brandon Schaefer
Approved revision: 671
Merged at revision: 727
Proposed branch: lp://staging/~brandontschaefer/nux/xim-tests
Merge into: lp://staging/nux
Prerequisite: lp://staging/~thumper/nux/nux.armel-fixes
Diff against target: 1357 lines (+1051/-12)
16 files modified
Nux/TextEntry.cpp (+2/-0)
NuxGraphics/GraphicsDisplayX11.cpp (+45/-2)
NuxGraphics/GraphicsDisplayX11.h (+7/-1)
NuxGraphics/Makefile.am (+7/-4)
NuxGraphics/XICClient.cpp (+112/-0)
NuxGraphics/XICClient.h (+60/-0)
NuxGraphics/XIMController.cpp (+146/-0)
NuxGraphics/XIMController.h (+62/-0)
NuxGraphics/XInputWindow.cpp (+4/-1)
NuxGraphics/XInputWindow.h (+1/-0)
examples/Makefile.am (+1/-1)
tests/Makefile.am (+11/-2)
tests/nux_automated_test_framework.cpp (+89/-1)
tests/nux_automated_test_framework.h (+2/-0)
tests/xim-test-commands.txt (+52/-0)
tests/xtest-text-entry-xim.cpp (+450/-0)
To merge this branch: bzr merge lp://staging/~brandontschaefer/nux/xim-tests
Reviewer Review Type Date Requested Status
Andrea Azzarone (community) Approve
PS Jenkins bot continuous-integration Pending
Łukasz Zemczak Pending
Jay Taoko Pending
Review via email: mp+132935@code.staging.launchpad.net

This proposal supersedes a proposal from 2012-08-22.

Commit message

XIM Support and Tests.

Description of the change

Fixed this branch to work with Unity: https://code.launchpad.net/~paulliu/nux/nux
To add XIM Support, as well as get some test for it.

This lets users add test for each IM easily in the text file xim-test-commands.txt. Ill probably have to write documentation but there are examples for 3 IMs.

The bases is:
0 = like an init function. It starts the IM with the name you give it.
1 = Key sequences to get the IM in the correct state. ie. ctrl+space
2 = Input you would type into your IM, ie. ninhao
3 = Checks the current text with what it should be.
4 = halt. So we know when to end the current IM test.

To test the three I have in there you'll need: fcitx, hime, and gcin
(sudo apt-get install gcin-chewing fcitx-googlepinyin hime-anthy)

The XIMController is in control of the current XICClient which gets switched based on the current XInputWindow(Dash/Hud. Each window has it's own XICClient).

XIMController gets allocated in GraphicsDisaplyX11 which you can get the XIMController anywhere else through GetGraphicsDisplay()->GetXIMController(). This is how XInputWindow gets a hold of the current XIMController.

***Note*** I still need to figure a good way out to only make XICClients for those windows that accept text....but as of right now any BaseWindow that ->EnableInputWindow() will create an XICClient for that window. (Im thinking I could hijack the bool take_focus which is only true for the Dash/Hud)

To post a comment you must log in.
Revision history for this message
Jay Taoko (jaytaoko) wrote : Posted in a previous version of this proposal

The test is failing on my system. When the program runs, some of the xim test passes while other fails. Anything I am missing on my system? In running on precise.

review: Needs Fixing
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

Resubmitted as a lot has changed now....

Revision history for this message
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal

76 + if (m_xim_controller)
77 + {
78 + delete m_xim_controller;
79 + }

What about using a smart ptr?

95 + void GraphicsDisplay::XICFocus()
96 + {
97 + m_xim_controller->FocusInXIC();
98 + }
99 +
100 + void GraphicsDisplay::XICUnFocus()
101 + {
102 + m_xim_controller->FocusOutXIC();
103 + }

I see that m_xim_controller is not initialized in the ctor. So I think that we should add a null check. Or am I missing something?

+* Copyright 2010-2012 Inalogic® Inc.

I think you don't need 2010 :)

I'll continue the review tomorrow (it's late now...).

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

Well we want to avoid using the new C++ standard for now in Nux. Pointers aren't that bad ;).

The reason I didn't do a NULL check is the m_xim_controller gets initialized when the OpenGL window is created.

In GLWindowManager.cpp

112 GraphicsDisplay *glwindow = new GraphicsDisplay();
113 glwindow->CreateOpenGLWindow(WindowTitle, WindowWidth, WindowHeight, Style, GLWindow, FullscreenFlag, create_rendering_data);

or this function:

128 GraphicsDisplay *glwindow = new GraphicsDisplay();
129 glwindow->CreateFromOpenGLWindow(WindowHandle, WindowDCHandle, OpenGLRenderingContext);

Right after the GrapichsDisplay is created the window is made which allocates the controller.

For sanity reason I can check for NULL...but it doesn't seem necessary, because if the m_xim_controller is NULL then so is the m_X11Display and m_X11Window which I don't see those checked for NULL else where in there...let me know what you think.

And yes...it is no longer 2010 haha :)

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

Or are you talking about nux::ObjectPtr?

Revision history for this message
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal

> Well we want to avoid using the new C++ standard for now in Nux. Pointers
> aren't that bad ;).

I think that the 'old" C++ standard has smart ptrs too :) Boost can help here too.

>
> The reason I didn't do a NULL check is the m_xim_controller gets initialized
> when the OpenGL window is created.
>
> In GLWindowManager.cpp
>
> 112 GraphicsDisplay *glwindow = new GraphicsDisplay();
> 113 glwindow->CreateOpenGLWindow(WindowTitle, WindowWidth, WindowHeight,
> Style, GLWindow, FullscreenFlag, create_rendering_data);
>
> or this function:
>
> 128 GraphicsDisplay *glwindow = new GraphicsDisplay();
> 129 glwindow->CreateFromOpenGLWindow(WindowHandle, WindowDCHandle,
> OpenGLRenderingContext);
>
> Right after the GrapichsDisplay is created the window is made which allocates
> the controller.
>
> For sanity reason I can check for NULL...but it doesn't seem necessary,
> because if the m_xim_controller is NULL then so is the m_X11Display and
> m_X11Window which I don't see those checked for NULL else where in there...let
> me know what you think.

Perfect. Yeah it's not necessary. Never mind my comment ;)

>
> And yes...it is no longer 2010 haha :)

Revision history for this message
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal

> Or are you talking about nux::ObjectPtr?

No, don't use nux::ObjectPtr. boost::shared_ptr should be good.

Revision history for this message
Andrea Azzarone (azzar1) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote : Posted in a previous version of this proposal

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-nux/184/console reported an error when processing this lp:~brandontschaefer/nux/xim-tests branch.
Not merging it.

Revision history for this message
Łukasz Zemczak (sil2100) wrote : Posted in a previous version of this proposal

Ok, first of all - I approve of this code and think it's very useful to have, but.

After consulting with the release team, we have decided to postpone this feature to 13.04. So sadly, here I need to disapprove of it so that it does not accidentally land in 12.10 lp:nux trunk.

But what I would like to do is (what Iain proposed) creating a PPA with unity and nux versions including this feature, making it public for people to test and comment (through the 2 bugs that are mentioned). So that we have it fully tested for 13.04.

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

Tims branch has landed for the new X11 macros, so now this branch is ready to merge :).

Revision history for this message
Andrea Azzarone (azzar1) wrote :

282 +bool XICClient::HasXIC() const
283 +{
284 + if (xic_ == NULL)
285 + return false;
286 + return true;
287 +}

Mabye return xic_ != null(ptr)?

You're not using nux namespace. Why?

472 + const char *xmodifier;
473 + /* don't do anything if we are using ibus */
474 + xmodifier = getenv("XMODIFIERS");

/*...*/
Maybe const char* xmo... = getenv...

I prefer this style but it's up to you :)

+void XIMController::SetupXIMClientCallback(Display *dpy, XPointer client_data, XPointer call_data)

The * goes with the type.

Revision history for this message
Andrea Azzarone (azzar1) :
review: Approve
671. By Brandon Schaefer

* Merged trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches