Skip to content
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

Show suggestions everywhere except in password fields #562

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

Helium314
Copy link
Contributor

Often suggestions are expected, but not shown, see e.g. #507, #456, #418, #275.
This PR should enable suggestion bar everywhere except in password fields (+ found working in a quick test).

But maybe not all people may like it see e.g. #532, so it might be more sensible to add a setting that is checked somewhere in

final boolean shouldShowSuggestionCandidates =
currentSettingsValues.mInputAttributes.mShouldShowSuggestions
&& currentSettingsValues.isSuggestionsEnabledPerUserSettings();
final boolean shouldShowSuggestionsStripUnlessPassword = currentSettingsValues.mShowsVoiceInputKey
|| currentSettingsValues.mShowsClipboardKey
|| shouldShowSuggestionCandidates
|| currentSettingsValues.isApplicationSpecifiedCompletionsOn();
final boolean shouldShowSuggestionsStrip = shouldShowSuggestionsStripUnlessPassword
&& (!currentSettingsValues.mInputAttributes.mIsPasswordField || currentSettingsValues.mShowsClipboardKey);
mSuggestionStripView.updateVisibility(shouldShowSuggestionsStrip, isFullscreenMode());

I can do this if the change is found acceptable.

@MajeurAndroid
Copy link
Member

MajeurAndroid commented Mar 9, 2022

I was thinking about that. I think we should investigate a bit more on this.
The current behaviour does not seem to be ok, e.g. we are ignoring flagNoSuggestions which is explicitly asking for no suggestions.. And this flag does not seem to be deprecated at all (see here).
I don't think ignoring everything is a good idea, there is some cases where an app does not want to show suggestions to user. If we ignore everything, OpenBoard does not fulfill its contract of behaving as an inputmethod, as expected by android framework and third-party apps.

@Helium314
Copy link
Contributor Author

Looking through the InputType.TYPE_TEXT_FLAG_*:

  • TYPE_TEXT_FLAG_AUTO_COMPLETE: show suggestion bar, but don't show suggestions.
  • TYPE_TEXT_FLAG_AUTO_CORRECT: "does not define whether the IME offers an interface to show suggestions"
  • TYPE_TEXT_FLAG_ENABLE_TEXT_CONVERSION_SUGGESTIONS: show transliteration suggestions
  • TYPE_TEXT_FLAG_NO_SUGGESTIONS: no suggestion bar
  • other flags should not affect suggestions

It looks like TYPE_TEXT_FLAG_AUTO_COMPLETE may be set implicitly sometimes, as this is the only change in this PR, and with this change suggestions are shown for android:inputType="textUri" in a layout xml (which corresponds to TYPE_CLASS_TEXT | TYPE_TEXT_VARIATION_URI). Without the change, suggestions are not shown.
Whether it makes sense or not is a different thing, but at least the rather common Gboard app does show suggestions for Uris (and probably other input types where users in the linked issues expect suggestions).

@MajeurAndroid
Copy link
Member

MajeurAndroid commented Mar 10, 2022

Ok, you convinced me.
For TYPE_TEXT_FLAG_AUTO_COMPLETE, I really think it was a mistake from AOSP to hide suggestion strip and to implement InputMethodSession.displayCompletions() at the same time, which is handling suggestions from editor for this particular purpose.. As you said, I think it might be the source of mentionned issues, it looks like it's set most of the time.

So to keep things nice and clean, can you remove commented lines and keep only password field condition and bring back flagNoSuggestions condition ? We'll merge this after that.

Thanks for your contribution :)

@Helium314
Copy link
Contributor Author

Helium314 commented Mar 10, 2022

So no need to add a setting?

Anyway, I had a closer look at what is going on (made tiny app with all inputTypes in separate edit fields) and noticed that android:inputType="textUri" actually does not stop suggestions from being shown.
I also tried adding android:imeOptions="actionGo", as this is another thing in the xml of an text field that's causing my troubles, but that didn't help (full xml with the non-working uri field, as far as I can tell the autoComplete flag is never set explicitly in xml or in the app).

So probably OpenBoard should still display suggestions if TYPE_TEXT_FLAG_AUTO_COMPLETE is set, and InputMethodSession.displayCompletions() just overrides which suggestions to show.
[Edit: looking at the code, I think this is what is happening anyway]

@MajeurAndroid
Copy link
Member

Yes, I think we have to show suggestions when TYPE_TEXT_FLAG_AUTO_COMPLETE is set, and that hiding it was an issue from AOSP team. That's what I was referring to in my previous comment.

For the setting, I think we can give it a go without one, as we are only removing flagAutoComplete condition.

@MajeurAndroid MajeurAndroid merged commit e9393df into openboard-team:master Mar 15, 2022
@Helium314
Copy link
Contributor Author

I didn't test any of the specifically mentioned apps, but actually the PR might close the first 4 issues mentioned in the initial post.

As for #532, I obviously linked the wrong issue here... and couldn't find which one I actually meant.

@Helium314 Helium314 deleted the patch-1 branch March 16, 2022 08:47
@RHJihan
Copy link
Contributor

RHJihan commented Mar 20, 2022

I am not getting Suggestion Strip on search bar & Google search of Edge Browser, Search bar of Facebook Messenger from this build.
Android version: 11
One UI 3.1

@Helium314
Copy link
Contributor Author

After this PR suggestions show in Lightning browser (and forks) search bar for me. Does it work in Lightning browser for you?
What happens if you enable clipboard key?

@RHJihan
Copy link
Contributor

RHJihan commented Mar 20, 2022

When Clipboard Key is enabled, suggestion strip is showing but no word is suggested.
Same thing happens on Google Play Store Search.

Works fine on Lightning Browser.

@Helium314
Copy link
Contributor Author

This is strange... I tested all input types and flage, and only in password fields and when TYPE_TEXT_FLAG_NO_SUGGESTIONS is set, no suggestions are shown.

Were suggestions in those apps shown before this PR? Because previously suggestions were shown when TYPE_TEXT_FLAG_NO_SUGGESTIONS was set, but are not any more.
Or if you do your own builds, could you try changing

mShouldShowSuggestions = !(mIsPasswordField || flagNoSuggestions);
to mShouldShowSuggestions = !mIsPasswordField;
Then suggestions should be shown really everywhere except in password fields.

@RHJihan
Copy link
Contributor

RHJihan commented Mar 21, 2022

Yes. This works fine. Suggestions are shown everywhere except in password fields.
mShouldShowSuggestions = !mIsPasswordField;

I tested on several apps and found a weird problem with the WhatsApp search.
No word is being suggested for the first word. But it starts suggesting from the second word.

@Helium314
Copy link
Contributor Author

If it works, this means these apps set TYPE_TEXT_FLAG_NO_SUGGESTIONS in places where the user might want suggestions, apparently ignoring "Please avoid using this unless you are certain this is what you want" from documentation.
Interstingly Gboard ignores TYPE_TEXT_FLAG_NO_SUGGESTIONS (i.e. still shows suggestions), presumably because too many apps set the flag when they shouldn't.
@MajeurAndroid what to do here? Ignore it as well, maybe optionally?

Regarding the WhatsApp search I don't know what would cause this. Did the PR or mShouldShowSuggestions = !mIsPasswordField; change behavior here?
Maybe WhatsApp search provides suggestions via InputMethodSession.displayCompletions(). This is the only thing I can currently think of that would have an effect on suggestions...

@RHJihan
Copy link
Contributor

RHJihan commented Mar 22, 2022

Is any decision taken to ignore TYPE_TEXT_FLAG_NO_SUGGESTIONS?

It turns out that the Samsung Keyboard is still showing suggestions where OpenBoard, build from this PR, is not showing.

@Helium314
Copy link
Contributor Author

Is any decision taken to ignore TYPE_TEXT_FLAG_NO_SUGGESTIONS?

I don't think so.
@MajeurAndroid how about adding an option to ignore TYPE_TEXT_FLAG_NO_SUGGESTIONS, with a preference summary text that clarifies that this ignores what is set by some other app?

@MajeurAndroid
Copy link
Member

MajeurAndroid commented Mar 23, 2022

@RHJihan did you tried with the latest build that includes these modifications ?

@RHJihan
Copy link
Contributor

RHJihan commented Mar 23, 2022

@MajeurAndroid I tried the build, which includes the modification mShouldShowSuggestions = !mIsPasswordField;, works fine.
Where the current merged PR fails to show suggestions on several apps i.e. Google Play Store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants