Bug 91641

Created attachment 116069 [details]
definition file for kmfl - defines mapping for ";n"

patch submitted to avoid the assert:  https://gerrit.libreoffice.org/15960

This is basically a stab in the dark.  It works and is used by several other IM functions, but the concept of SolarMutex guards is a black box to me.  The assert avoidance is only needed for development builds anyway, so not a critical patch for resolving this bug.

patch submitted to resolve the incorrect placement of the composed character: https://gerrit.libreoffice.org/15961

There is still an edge case that doesn't work properly.  The very first cell that is edited with a call to IMDeleteSurrounding does not work properly.  That is because it is not recognized as having the focus.

Step1.  Type 123456 in a cell.
Step2.  In the input line, position the cursor between 34
Step3.  Enter a kmfl sequence, like ;;~a  (for ä̃).  the result is 123;;~ä̃456.

This is a different problem from the reported problem.  The cursor position is OK (because deleteText was not called), but the composing keystrokes are not swallowed up (also becase deleteText was not called).

I have a patch submitted with resolves this: https://gerrit.libreoffice.org/15962 but I am not sure that this is the best way to fix it.

Justin Luth committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=8bf66416a834a503309d4d0f3298d65f321f360e

tdf#91641 EditLine should have focus on accessibleText init.

It will be available in 5.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.

Ho hum =) So - we can take a SolarMutex in these IM methods - if we are sure that ibus is not doing the right thing and there is no hope of fixing it there - but ... that's far less than ideal.

Ideally the ibus input method plugin should be calling:

GDK_THREADS_ENTER();

do_stuff();

GDK_THREADS_LEAVE();

in the right places. Where 'right' means - almost certainly in response to any dbus message - which will happen in an input handler from the main-loop. And 'wrong' means - you mustn't take this non-recursive mutex anywhere you're called by gtk+ itself (which clearly holds the toolkit lock while it is executing).

Yes - there is some 'deprecated' nonsense about these methods - but there is basically no other way to achieve thread-safety for LibreOffice cf.

/**
 * GDK_THREADS_ENTER:
 *
 * This macro marks the beginning of a critical section in which GDK and
 * GTK+ functions can be called safely and without causing race
 * conditions.  Only one thread at a time can be in such a critial
 * section. The macro expands to a no-op if #G_THREADS_ENABLED has not
 * been defined. Typically gdk_threads_enter() should be used instead of
 * this macro.
 *
 * Deprecated:3.6: Use g_main_context_invoke(), g_idle_add() and related
 *     functions if you need to schedule GTK+ calls from other threads.
 */
#define GDK_THREADS_ENTER() gdk_threads_enter()

So - please do use those.

For now - if we insist on banging our head futiley against the wall in an attempt to work around whatever bit of the brokenness that we happen to be able to see ;-) (which is only part of it - LibreOffice does use gtk+ from multiple threads so the only safe way is as above).

Then yes we can add SolarMutexGuard aMutex; to each of the input-method methods. Lets have a typedef:

IBusThreadingNeedsFixGuard aGuard;

or something and a link to this bug next to the typedef; my hope would be to remove this later =) I'd like to see the complete set of input-methods instrumented if that's possible ?

Does that sound sensible ? it's -really- awesome to have you working on this - many thanks for raising the issue, and submitting patches too =)

(In reply to Michael Meeks from comment #6)
If the SolarMutex solution is not ideal, it can be ignored, since it only affects testing with the development builds.  (I'm still just a newbie LibreOffice-only programmer trying to tackle bugs that specifically affect us, so I didn't really understand much of what you said)-:

Justin Luth committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6c76a0a84785b48c29687e6d31b8229a230563d8

tdf#91641 adjust cursor when deleting preceding characters

It will be available in 5.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.

Justin Luth committed a patch related to this issue.
It has been pushed to "libreoffice-5-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=227e7f903fee64d63b48bd31707887ba28a019bc&h=libreoffice-5-0

tdf#91641 adjust cursor when deleting preceding characters

It will be available in 5.0.0.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.

Justin Luth committed a patch related to this issue.
It has been pushed to "libreoffice-4-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6f4c31eb757cd19311b23ce06fdbbe9268f9cd16&h=libreoffice-4-4

tdf#91641 adjust cursor when deleting preceding characters

It will be available in 4.4.5.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.