-
-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(android): Control when system keyboard Keyboard Picker clears activity stack #13313
base: beta
Are you sure you want to change the base?
Conversation
User Test ResultsTest specification and instructions Test Artifacts |
dcd70c5
to
118e15b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any issues without how this code will execute, but I had trouble understanding the naming, and I had to re-read it carefully to not be confused.
Part of the problem is that I didn't know this: "A task is a collection of activities that users interact with when trying to do something in your app." This is confusing to me because I don't think of activities as smaller than tasks. I think of a task as something that is discrete and an activity as something broader. You can't do much about the Android naming, but it is possible to clarify this relationship with your code and naming.
Beyond that, I think I would still be confused about what is being enabled and disabled. You are not clearing immediately but setting the flag to clear later, predicated on some other event. But that event is not mentioned in the name of the flag. I would rather see it included in the name of the flag (even a long name) than being required to read other code or comments to understand the purpose of the flag.
One other confusing thing is that the actual flag is named canClearActivityTask
when it seems the flag is forcing the clear to happen. By prefixing it with 'can' I expect that the underlying action is available but optional. The difference in the flag name from the names of the functions that set the flag cause me to doubt that I understand the code that is applying the flag.
Yeah, I find the naming quite clunky.
Maybe something like this?
|
Test ResultsI tested this issue with the attached Keyman"18.0.199-beta-test-13313" build(24/02/2025) on Android 14(Physical device) and Android 12(Emulator). Here I am sharing my observation.
|
That is a lot better, but can it also specify when the existing activity is cleared? How do you best describe what actually triggers the clearing of the activity (provided this flag has been set)? |
i.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TASK); | ||
if (KeyboardPickerActivity.getClearActivityTask()) { | ||
// Keyboard Picker Activity becomes root activity and clears Keyman app | ||
i.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TASK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a lot better, but can it also specify when the existing activity is cleared? How do you best describe what actually triggers the clearing of the activity (provided this flag has been set)?
This is the scenario:
When the Keyman system keyboard (as opposed to in-app keyboard) is launching the Keyboard Picker,
we generally want to dismiss the Keyman app if it's running.
The exception is when using the Keyman system keyboard within the Keyman app (e.g. doing a keyboard search).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just looked at this again. I think what would help would be to associate your functions directly with showing the keyboard picker. The code reads like it is generic to clearing activity task (closing Keyman), but when you call the related functions, you include a comment that shows that this is only relevant to the keyboard picker.
So instead of clearExistingActivity
, you could give the context in the name and say clearExistingActivityOnShowKeyboardPicker
. That is, assuming you only ever plan to read and apply the flag when showing the keyboard picker. (That makes the comments pretty much redundant, but we'd always rather just read the function if we can instead of needing a comment to help us interpret the code.)
If the 'ExistingActivity' is always going to be Keyman -- it is the thing that is getting cleared or closed, then could it actually be closeKeymanOnShowKeyboardPicker
? Or is that going too far?
Sorry to belabor this, but it seems like we are doing something concrete and discrete, but the terminology is referencing things that are generic and of wider scope. So my lack of familiarity with the Android architecture made it hard to comprehend. I think the code could be in more concrete domain/Keyman terms where the reference and mapping to the Android API only happens when you actually set Intent.FLAG_ACTIVITY_CLEAR_TASK
.
I still have a problem with the variable canClearActivityTask
which seems to really mean forceClearActivityTask
or forceCloseKeyman
(if closing Keyman is the only possible ActivityTask that we will be clearing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, latest round of renames
I called the flag canCloseParentAppOnShowKeyboardPicker
because there may be other app developers using Keyman Engine for Android (besides Keyman).
And then went with
- dontCloseParentAppOnShowKeyboardPicker()
- closeParentAppOnShowKeyboardPicker()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense -- I wasn't giving any thought to the API boundary.
Fixes #12991
For as long as Keyman for Android 2.8, the system keyboard KeyboardPickerActivity has been setting the
Intent.FLAG_ACTIVITY_CLEAR_TASK
flag which becomes the root activity and clears previous tasks (Keyman app).When using Keyman as a system keyboard within the Keyman app, we don't want this to occur.
TODO:
Change
Add call to disable/enable when the system keyboard Keyboard Picker clears previous tasks.
User Testing
Setup - Install PR build of Keyman for Android
From "Get Started", set Keyman as the default system keyboard.
Close Keyman if it's running
Launch Chrome browser and select the text area and select Keyman keyboard if need be
With the Keyman system keyboard OSK displayed, hold the globe key to launch the Keyboard Picker
Dismiss the keyboard picker
Verify Keyman is not launched in the background