keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

notany() and context() do not work well together in KeymanWeb #917

Closed mcdurdin closed 3 years ago

mcdurdin commented 5 years ago

The notany() statement and the context() statement do not cooperate in generated KeymanWeb code. Currently, a context statement that references the notany() in the context will return a compile error. This dramatically limits the usefulness of notany() ... because there is no way to capture the character that was matched.

For example:

store(&TARGETS) 'web'

begin Unicode > use(main)

group(main) using keys

store(vow) 'aeiou'
notany(vow) 'b' + 'c' > context(1) 'BC'

I did some investigation into possible solutions. This is a partial fix:

diff --git a/windows/src/developer/TIKE/compile/CompileKeymanWeb.pas b/windows/src/developer/TIKE/compile/CompileKeymanWeb.pas
index d45c8ca3..d923ee5c 100644
--- a/windows/src/developer/TIKE/compile/CompileKeymanWeb.pas
+++ b/windows/src/developer/TIKE/compile/CompileKeymanWeb.pas
@@ -746,6 +746,7 @@ var
   pwszcontext,pwsz: PWideChar;
   Index: Integer;   // I3910
   nlt: string;
+  ContextLen: Integer;

   function AdjustIndex(pwszContext: PWideChar; Index: Integer): Integer;   // I3910
   var
@@ -785,6 +786,11 @@ var
           end;
         CODE_DEADKEY:
           Result := Result + nlt + Format('k.KDO(%d,t,%d);', [len, recContext.Deadkey.Deadkey]);   // I4611
+        CODE_NOTANY:
+          begin
+            Index := ContextLen - ContextIndex + 1; // Need to get context index from cached context, based on length of context
+            Result := Result + nlt + Format('k.KO(%d,t,k.KC(%d,1,t));', [len, Index]);
+          end;
         else
         begin
           ReportError(fkp.Line, CERR_NotSupportedInKeymanWebContext, Format('Statement %s is not currently supported in CODE_CONTEXT match', [GetCodeName(recContext.Code)]));  // I1971   // I4061
@@ -815,6 +821,8 @@ begin

   if Assigned(fkp) then
   begin
+    ContextLen := xstrlen(fkp.dpContext);
+
     if IsKeyboardVersion10OrLater
     // KMW >=10.0 use the full, sentinel-based length for context deletions.
     then

However, it does not cope with start-of-context checks (although a naive perusal seems to indicate that it should). It also has not been tested against the 10.0 code paths. A complete spec needs to be written to validate the requirements and compatibility.

mcdurdin commented 5 years ago

See also https://community.software.sil.org/t/notany-issue/905

MayuraVerma commented 5 years ago

Any updates on this? This is a very important feature to keep the keyboard layout consistent across all platform.

mcdurdin commented 5 years ago

No progress to report at this time. We welcome pull requests from the community!

MayuraVerma commented 5 years ago

Please target this for at least 11.0

mcdurdin commented 5 years ago

@jahorton what do you think about this issue? Is it something that is feasible for 11.0?

MayuraVerma commented 5 years ago

Hello @mcdurdin

Is this targeted for 11.0?

mcdurdin commented 5 years ago

No, this won't make it into 11.0.

MayuraVerma commented 5 years ago

@mcdurdin

This issue is killing the keyboard. is there any workaround?

mcdurdin commented 5 years ago

One workaround is to do, instead of notany(x), any(y), where the store(y) contains the inverse set of expected characters. With this pattern, you'd put only the characters you expect, such as punctuation, English letters, etc, and perhaps U+000D U+000A U+0009 (enter and tab characters), for example:

store(y) U+0009 U+000A U+000D U+0020 'abcdefghijklmnopqrstuvwxyzABC...0123...!@#...'

any(y) + 'x' > context(1) beep c or whatever you want it do to!

This is not 100% perfect because you have to specify a larger set of characters, and will miss situations where a character that is not referenced in the store arises, but those are edge cases that I am guessing will not arise all that often.

MayuraVerma commented 5 years ago

thanks,

how can we handle the beginning of document situation?

mcdurdin commented 5 years ago

how can we handle the beginning of document situation?

Use the nul keyword:

nul + 'x' > 'y' c ... nul means no starting context (i.e. start of document)
mcdurdin commented 5 years ago

It's not completely trivial to implement because of the way context() is implemented in KeymanWeb -- it assumes the context character is known on the basis of the rule, but in the case of notany(), we need to query the context from the document in order to retrieve the character. To add extra complexity, we need to do that query before we start the output, and cache it, because once we start output, the context may be lost. This is mostly fine except when the character is a deadkey.

I am going to assume KM10+ for this because otherwise we are in a world of pain anyway with deadkeys... It looks like we don't have a good clean way of retrieving the deadkey and caching it keyboard-code-side at present.

I am modeling the keyboard as follows, at present:

store(vow) 'aeiou'
notany(vow) 'b' + 'c' > context(1) 'BC'

Compiles as:

if(k.KKM(e, modCodes.VIRTUAL_KEY /* 0x4000 */, keyCodes.K_C /* 0x43 */)&&k.KFCM(2,t,[{t:'a',a:this.s_vow,n:1},'b'])) {   // Line 9
      r=m=1;
      var kc1 = k.KC(2,1,t);
      k.KDC(2,t);
      k.KO(-1,t,kc1);
      k.KO(-1,t,"BC");
    }

This works great for everything except deadkeys.

@jahorton notes:

Well, there's the cachedContextEx cache that should still have data from the combined cache until the next rule executes; the data still exists, as the deadkey's identifier should be retrievable there. The issue is that we don't have a keyboard API function designed to actually access that for use by keyboard code.

... there's already _BuildExtendedContext - that already exists. It's not exposed for use in keyboards, but it's used internally by KFCM/fullContextMatch already.

It returns type (string|number)[]. Note that output doesn't accept this format. Yet.

I'd probably implement outputExtended to iterate over the provided array, calling output for string entries and deadkeyOutput for numerical (deadkey) entries.

Add in a simple check for index == 0 to specify the needed dn value for only that iteration and it should be fairly straightforward.

mcdurdin commented 5 years ago

After discussion with Joshua, we concluded that this requires backward-incompatible changes to the compiled keyboard, so this will need to land in 13.0 (not enough time to get it finished and tested for 12.0).

MayuraVerma commented 5 years ago

@mcdurdin is it possible to implement without deadkeys for now? and support deadkeys later?

I am willing support the testing of keyboards.

mcdurdin commented 4 years ago

Sadly, even without deadkeys, I'm not sure if this can be done without backward-incompatible changes to the keyboard compiler. I'm short on time to dig deeper at present. So it will need to wait until the next release.

MayuraVerma commented 4 years ago

OK. Please let me know when you need help to test.

MayuraVerma commented 3 years ago

Can this be resolved in version 14?

mcdurdin commented 3 years ago

We don't have the resources to tackle this for 14.0.

mcdurdin commented 3 years ago

Internal discussion here: we are going to see if we can shuffle priorities and get this into 14.0.

MayuraVerma commented 3 years ago

Internal discussion here: we are going to see if we can shuffle priorities and get this into 14.0.

Please let me know, if I can help in running any tests

mcdurdin commented 3 years ago

Capturing for posterity.

https://community.software.sil.org/t/problem-with-caps-lock-output/3718/17 also relates.

@jahorton writes:

@mcdurdin writes:

It's been too long so I don't remember the details of what we need to do.

Same. It feels like you're more the expert on the issue than I, given the amount of contribution each of us has given toward the issue in the past.

One key thing I noticed in our previous documentation on the issue:

After discussion with Joshua, we concluded that this requires backward-incompatible changes to the compiled keyboard, so this will need to land in 13.0 (not enough time to get it finished and tested for 12.0).

At a glance now, without digging too deeply... use of notany should result in a 14.0+ only Web keyboard as a result, right? I don't see too much reason that this should interfere too much with compilation of Web keyboards that don't use notany, so in terms of Developer, it may just be a matter of supporting an extra compiler target for the new API functions?

Of course, there's still the Web-side stuff as well, including proper unit tests - and designing those will require stronger familiarization with the issue to write.. For a rough estimate, I'm guessing 2 or 3 days of work in total if the assumptions above are correct.

mcdurdin commented 3 years ago

Okay, we have a solution; please check the alpha build in a couple of days to test that it is working correctly -- it should land in 14.0.177.

MayuraVerma commented 3 years ago

@mcdurdin Thank you. This is resolved in iOS version. I assume this is resolved in Andriod and web as well.

mcdurdin commented 3 years ago

Yes will be resolved on all three platforms.