Bug 137620 - kmfl keyboard entry jumps around to other textboxes or beginning of line in editeng
Summary: kmfl keyboard entry jumps around to other textboxes or beginning of line in e...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Draw (show other bugs)
Version:
(earliest affected)
6.2.0.3 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.1.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2020-10-20 12:30 UTC by Justin L
Modified: 2022-03-09 09:49 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
tableKMFL.odg: helper document to demonstrate the problem. (12.09 KB, application/vnd.oasis.opendocument.graphics)
2020-10-20 12:30 UTC, Justin L
Details
kmfl_bad.mp4: screen capture showing the problem (359.65 KB, video/mp4)
2020-10-20 13:02 UTC, Justin L
Details
kmfl_prebacktrace.txt: gdb results before applying patch (10.72 KB, text/plain)
2020-10-22 05:46 UTC, Justin L
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin L 2020-10-20 12:30:51 UTC
Created attachment 166541 [details]
tableKMFL.odg: helper document to demonstrate the problem.

KMFL is an ibux linux keyboarding solution that converts keyboard entry (like ;;~a into ä̃ ) by using accessibility calls to delete the previous characters. This has stopped working in Draw in LO 6.2 - now the cursor frequently jumps around when GTK calls to delete/replace the characters.

Information on installing kmfl can be found in bug 91641 comment 0 and comment 1.

Steps:
1.) With ibus kmfl configured with GE.kmn, open the attached document
2.) Click in the first text box and enter "simple" there.
3.) Click in the yellow table square, and type ;;~a

Result:
the ;;~ composing characters are left in the yellow square, and the composed ä̃ is the first character in the textbox.

bisected to bibisec–linux-64-6.2 commit 863e44897a39bc73f1b118d1273bf8552488f816
author	Paul Trojahn on 2018-09-04 09:41:28 +0200
commit 6b669c9d9ee61c5c37c384c3a546467a048f5636
    tdf#118967 Batch all a11y notifications

I imagine this might have implications for CJK keyboarding solutions based on ibus as well, but have no proof of that.
Comment 1 Justin L 2020-10-20 13:02:59 UTC
Created attachment 166545 [details]
kmfl_bad.mp4: screen capture showing the problem
Comment 2 Justin L 2020-10-21 12:44:45 UTC
Likely no one will want a simple revert done, but that is fairly easily possible and is staged at https://gerrit.libreoffice.org/c/core/+/104614.

The author has not been actively developing for the last two years - this was one of his last commits.  https://cgit.freedesktop.org/libreoffice/core/log/?qt=author&q=Paul+Trojahn.

I tried (without understanding) to just undo parts of this, but was unsuccessful.
Comment 3 Noel Grandin 2020-10-21 12:57:08 UTC
Sorry, no idea how to fix this - might try asking caolan. Reverting that commit will likely cause other problems.
Comment 4 Caolán McNamara 2020-10-21 13:57:21 UTC
Just to be clear, does KMFL directly use accessibility calls or does it use "delete-surrounding" and "retrieve-surrounding" GTK-IM signals and LibreOffice uses its accessibility stuff to try and fulfill those (the lcl_GetxText hack) ?
Comment 5 Justin L 2020-10-21 14:13:47 UTC
(In reply to Caolán McNamara from comment #4)
> does KMFL directly use accessibility calls or does it use
> "delete-surrounding" and "retrieve-surrounding" GTK-IM signals

delete-surrounding.

Here is a quote I found in my email:
"KMFL (and other inputs methods) need to be able to delete text that has been previously entered. There are couple of ways to do this, one being to use a system call like "delete surrounding text". Another method is to simulate the backspace key. The former method is the preferred method because it gives better control over what is deleted.  However applications have to be written to support "surrounding text". Apps are supposed to report to ibus whether they support "surrounding text" calls and if they don't,  then kmfl falls back to simulating the backspace key.
Comment 6 Justin L 2020-10-21 14:22:12 UTC
connecting to bug 85912 which has good info about kmfl. This was one of the bugs that launched me into LibreOffice programming.
Comment 7 Caolán McNamara 2020-10-21 15:18:18 UTC
Using the a11y layer for "delete-surrounding" and "retrieve-surrounding" was one of those bright ideas I probably should never have had, it's always been a bit wrong in some way or another.

I wonder if the alternative thing I added for the "editengine hosted in a GtkDrawingArea" case works better or just fails horribly in some other way. To test that, in calc, format->page->header->edit and try out your IME in those editengines. Maybe if that works, or can be improved to work, then the stuff used to implement that could be adapted to handle the normal case of an "editengine hosted in a vcl::Window" and sidestep the use of the a11y stuff entirely
Comment 8 Justin L 2020-10-21 16:05:45 UTC
calc's format->page->header->edit  does nothing on a build from Sept 12. (I just get the characters I typed, but no deletions, and no composed end result.  ;;~a == ;;~a)

On another machine with a build from Oct 16, I get an assert as soon as I type the first semi-colon (only when the IME keyboard is chosen).

vcl/source/app/dbggui.cxx:35: void ImplDbgTestSolarMutex(): Assertion `ImplGetSVData()->mpDefInst->GetYieldMutex()->IsCurrentThread() && "SolarMutex not owned!"' failed.
Comment 9 Caolán McNamara 2020-10-21 20:00:44 UTC
(In reply to Justin L from comment #8)
> calc's format->page->header->edit  does nothing on a build from Sept 12.

Yeah, its quite new.
 
> On another machine with a build from Oct 16, I get an assert as soon as I
> type the first semi-colon (only when the IME keyboard is chosen).
> 
> vcl/source/app/dbggui.cxx:35: void ImplDbgTestSolarMutex(): Assertion
> `ImplGetSVData()->mpDefInst->GetYieldMutex()->IsCurrentThread() &&
> "SolarMutex not owned!"' failed.

That build surely has it :-) I'm clearly missing a SolarMutexGuard somewhere, should be fairly easy to spot from a backtrace. (https://gerrit.libreoffice.org/c/core/+/104644 is possibly sufficient) A quick "dnf search kmfl" doesn't show me something I could use to quickly reproduce locally under fedora.
Comment 10 Justin L 2020-10-22 05:20:06 UTC
(In reply to Caolán McNamara from comment #9)
> A quick "dnf search kmfl" doesn't show me something I could use to quickly
> reproduce locally under fedora.

KMFL is now an official release of http://keyman.com/linux.  
Q. What Linux distros will Keyman work with?

A. Keyman is built for amd64 architecture and runs on Debian, Ubuntu, Wasta Linux. It can be compiled to run from source in most distributions.
Comment 11 Justin L 2020-10-22 05:46:12 UTC
Created attachment 166609 [details]
kmfl_prebacktrace.txt: gdb results before applying patch

(In reply to Caolán McNamara from comment #9)
> I'm clearly missing a SolarMutexGuard
> somewhere, should be fairly easy to spot from a backtrace.

Your patch does get rid of the assert. The composed character is created, but only the last character is replaced.  So ";;~a" turned into ;;~ä̃".
Comment 12 Justin L 2020-10-22 06:00:08 UTC
For anyone needing a workaround for this bug, start LibreOffice with

 XMODIFIERS=@im=ibus GTK_IM_MODULE=xim  soffice

This can be automated in Ubuntu / Wasta with
sudo cat > /etc/profile.d/libreoffice   << EOF
XMODIFIERS=@im=ibus GTK_IM_MODULE=xim 
EOF
Comment 13 Caolán McNamara 2020-10-22 13:23:40 UTC
alright, how about with https://gerrit.libreoffice.org/c/core/+/104659 then. That seems to work better wrt the editengine-in-GtkDrawingArea case
Comment 14 Justin L 2020-10-22 19:31:23 UTC
(In reply to Caolán McNamara from comment #13)
> alright, how about with https://gerrit.libreoffice.org/c/core/+/104659 then.

Nifty. That worked well for the left side. It took a bit of effort to get it to work in the middle or right side. Either using the cursor to go back and forth a few times, or pressing enter - all seemed to help, but still a bit hit or miss. When it doesn't work, it just does nothing at all - no deletes and no forming a composed character.
Comment 15 Caolán McNamara 2020-10-23 12:10:29 UTC
I think comment #14 is a focus thing and https://gerrit.libreoffice.org/c/core/+/104721 seems to be the problem there.
Comment 16 Justin L 2020-10-23 14:04:11 UTC
(In reply to Caolán McNamara from comment #15)
> I think comment #14 is a focus thing and
> https://gerrit.libreoffice.org/c/core/+/104721 seems to be the problem there.

That works in terms of always creating the composing text, and deleting characters. So now I can easily enter characters in left, middle, and right.

However, in the middle of a word it deletes the wrong characters.
bc|def -> bc;;~adef == bc;;fä

That sounds like https://cgit.freedesktop.org/libreoffice/core/commit/?id=6c76a0a84785b48c29687e6d31b8229a230563d8
Comment 17 Caolán McNamara 2020-10-23 15:03:44 UTC
should we just "select" and "delete" the range from delete-surrounding-text, leaving the cursor at that deletion point ? like https://gerrit.libreoffice.org/c/core/+/104735 does, or should we copy the original cursor pos and just adjust it by the num of chars removed and set a new cursor ?
Comment 18 Justin L 2020-10-23 16:39:48 UTC
(In reply to Caolán McNamara from comment #17)
> should we just "select" and "delete" the range or should we copy the
> original cursor pos and just adjust it by the num of chars removed?

Well, in this case the select and delete method works for me.

At this point everything seems to be working well.  I was able to get an assert doing undo once, but it is not necessarily related since undo is known to be flaky and I could not reproduce it. In general undo and redo seem to work. (Arguably in our use case it would be nice to just add/remove the composed character and skip all of the intermediary keypresses, but that is way beyond the scope of this bug report and probably not generically desirable in any case.)
Comment 19 Caolán McNamara 2020-10-23 19:13:44 UTC
well, that's somewhat encouraging and possibly gives us a chunk of something we can reuse to escape out of this problem without having to use the a11y apis.
Comment 20 Commit Notification 2020-10-25 21:02:25 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/41df2d06cc13a1c60a4ea9671535afb8bd55b344

Related: tdf#137620 factor out working IMDeleteSurrounding hunk

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Commit Notification 2020-10-25 21:03:38 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ce5e41ab99af350ca8f4b9fef3017d53f3526f83

Related: tdf#137620 use existing SalEvent::SurroundingTextRequest

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 22 Commit Notification 2020-10-27 16:02:06 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/b9405fbc4e19901c78d136895c5ab0437d8450ac

Resolves: tdf#137620 add DeleteSurroundingText at vcl::Window level

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 23 Justin L 2020-10-27 18:50:48 UTC
(In reply to Commit Notification from comment #22)
> Resolves: tdf#137620 add DeleteSurroundingText at vcl::Window level

Neat - this looks like it fixed the problem in Writer tables as well.

However, I just noticed one more problem. It ONLY seems to work on the first document. If I close the writer document (down to the start center) and then open up my draw test, then nothing IME happens at all.  No composed character and no deleting characters.  (If I exit the program completely, then it works again for the first document.)

It works fine if I open three documents from File - open. I can even close and restart them as long as one if left running. It is only when dropping to the start center that IME stuff stops.
Comment 24 Commit Notification 2020-10-28 09:05:31 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/8dcbbea3802670004c3e78a1ff1ec56b23df674c

tdf#137620 add explicit SurroundingText support to starmath Edit Window

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 25 Caolán McNamara 2020-10-28 12:32:52 UTC
comment #23 might be a focus issue and https://gerrit.libreoffice.org/c/core/+/104932 might help there
Comment 26 Commit Notification 2020-10-28 19:38:21 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/a25acac2a6c7770bb07ced4c29b3b9009ad7ff14

Related: tdf#137620 focus-in IMHandler on grab_focus

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 27 Justin L 2020-10-29 16:53:33 UTC
Calc's cells are not deleting the composing characters. (It works in the editing area, but not when the cursor is in the cell itself.) It was working earlier. Missing focus?

Open Calc. Start typing:
;;~a
vcl/unx/gtk3/gtk3gtkframe.cxx:4448: SignalIMDeleteSurrounding will CalcDeleteSurrounding selection from text[] start[0] offset[-1] chars[1]
vcl/unx/gtk3/gtk3gtkframe.cxx:4413: IM delete-surrounding, unable to move to offset: -1
vcl/unx/gtk3/gtk3gtkframe.cxx:4448: SignalIMDeleteSurrounding will CalcDeleteSurrounding selection from text[] start[0] offset[-1] chars[1]
vcl/unx/gtk3/gtk3gtkframe.cxx:4413: IM delete-surrounding, unable to move to offset: -1
vcl/unx/gtk3/gtk3gtkframe.cxx:4448: SignalIMDeleteSurrounding will CalcDeleteSurrounding selection from text[] start[0] offset[-1] chars[1]
vcl/unx/gtk3/gtk3gtkframe.cxx:4413: IM delete-surrounding, unable to move to offset: -1
Comment 28 Justin L 2020-10-29 17:25:53 UTC
(In reply to Justin L from comment #27)
>  Missing focus?
Probably not so simple. Needs something like comment 24? Looks like it came from
commit	e087e25f05e689091cbf1c4f91b6e93878ac17e
  weld InputBar
Comment 29 Caolán McNamara 2020-10-30 15:38:16 UTC
yeah, probably, let me have an attempt at that I think I see where it should go
Comment 30 Commit Notification 2020-10-30 19:45:00 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/c946abb704c9f72c1fdc696ac72c6a9381d95f16

tdf#137620 add explicit SurroundingText support to ScGridWindow

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 31 Justin L 2020-10-31 07:00:33 UTC
Comment 30's fix seems to work good for calc.

I have just two more places that I have found.
The first is Impress Outline view (view menu - Outline) doesn't delete composing characters (the same problem response as comment 27). Bibisect points to comment 22's Resolves: tdf#137620 add DeleteSurroundingText at vcl::Window level
Comment 32 Caolán McNamara 2020-10-31 19:48:50 UTC
sd/source/ui/view/sdwindow.cxx has (pre-existing) GetSurroundingText/GetSurroundingTextSelection and they have special cased "ViewShell::ST_OUTLINE" to do nothing. It might be as simple as removing those cases.
Comment 33 Commit Notification 2020-11-01 19:36:59 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/c99e66cb5c75efb933694bfbb8edd7117c3d655f

tdf#137620 support surrounding text for impress outline view

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 34 Justin L 2020-11-02 04:26:15 UTC
The last instance I found was calc comments. They were working until 6.2 as mentioned in comment 0, when again they stopped deleting the composing chars.
I tried to fix this one myself, but couldn't even find the window/control that handled the comment...
Comment 35 Commit Notification 2020-11-02 11:41:29 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ff69401d8c941e5adaa662da81c6f02d9fe71955

tdf#137620 support surrounding text for calc comments

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 36 Justin L 2020-11-02 12:59:29 UTC
OK, I think that this report has enough comments and commits associated with it - lets mark this as fixed. If there are any other situations, create a separate bug report for it.

Thanks a million for all your work on this Caolán.
Comment 37 Kevin Suo 2020-11-07 02:42:06 UTC Comment hidden (obsolete)
Comment 38 Justin L 2020-11-07 04:41:56 UTC Comment hidden (obsolete)
Comment 39 Kevin Suo 2020-11-07 05:57:32 UTC Comment hidden (obsolete)
Comment 40 Kevin Suo 2020-11-07 05:58:48 UTC Comment hidden (obsolete)
Comment 41 Kevin Suo 2020-11-07 07:46:48 UTC Comment hidden (obsolete)