-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
auto toggle toolbar when suggestions are available #674
Conversation
I was about to open a feature request for exactly that! Thanks for the PR, I will look through it/test it and give feedback in the next days! 👍🏻 |
no problem glad you liked it 👍 |
Could you also add a separate settings for just auto-hiding the toolbar when suggestions are available? When you have pinned keys, they will flash when you type e.g. |
When there are default suggestions on opening an input field (with next-word-suggestions on), the toolbar shortly shows up, and then disappears once the suggestions arrive. |
@Helium314 thanks for the feedback and sorry about the delay
I have divided the toggle setting to 2 separate settings (show/hide),
That should help those who use pinned navigation toolbar keys, it was a bit too intrusive to show the toolbar after each word.
Could u please check if this is still an issue, my device may render it too fast for me to notice the flicker |
There is really no need for that, here I'm much worse...
Yes, it still happens |
Do you happen to know whether this piece of code is necessary? |
It was added in e337f74. |
Ok so I modified the condition in hideWindow() so the visibility of the toolbar is set to I believe the flickering should be mostly gone (hopefully) with 92d7827, would appreciate a feedback to know if it's still present. |
Flickering looks mostly good now. There are a few more cases where it happens, like when the toolbar was showing, you close the keyboard, and then tap inside some text field where toolbar should not show. But that's more obscure, and not necessary to fix. One think I noticed when re-reading: boolean suppressResume = currentSettingsValues.mAutoShowToolbar
&& mInputLogic.mConnection.getTextAfterCursor(1, 0).length() == 0
&& mInputLogic.mConnection.getCodePointBeforeCursor() == Constants.NOT_A_CODE;
if (!suppressResume)
mHandler.postResumeSuggestions(true /* shouldDelay */); right above the first |
@Helium314 thanks a lot for the feedback
Yeah I've noticed that too, I just wasn't convinced that the toolbar should be displayed in that case. Note that the suggestions are shown only when the "Personalized suggestions" setting is enabled because they are actually personalized predictions. So if that setting is turned off, the main toolbar will be shown. I can add the provided code, but then personalized predictions won't be suggested when the input starts. Also, upon reading the comment above the first |
I think it makes sense to also show it in this case, because (at least for me) those initial suggestions are rarely something I actually want to type.
The code above essentially checks whether the text field is empty, which does not depend on the cursor position. (maybe that could be checked more efficiently) |
A summary when the toolbar shows automatically, depending on other settings (auto-show and auto-hide enabled)
|
Thanks for the detailed summary. I assume it is an observation of the current behavior? because I don't think that all of it is intended. In my opinion the toolbar should be auto shown when:
The toolbar should be auto-hidden when actual suggestions are available (including inline suggestions) If suggestions are disabled, I think the "auto show toolbar" setting is still useful & should not be affected (the suggestion strip view is of no use in that case, and should probably be disabled, but that is another issue) On a side note, it seems like a waste of screen estate that setNeutralSuggestions() sets empty suggestions if next-word suggestions are disabled. Do you think it would be possible to show the top 3 dictionary words with highest frequency instead? |
I wasn't sure whether everything was working as you intended, that's why I wrote the summary.
👍
Not sure about the "even if the input field is not empty" part, but at least some uses will want this.
This seems to be a very specific and rare situation. It should only be considered if it doesn't introduce much complexity.
👍
I think hiding words properly already, didn't test inline suggestions though.
Possible yes, but it might not be particularly useful (and then the app is showing next-word suggestions despite the setting being disabled). |
thanks, it was helpful!
it's a relatively simple check, currently that's not a common situation but it may be useful in the future when dismissing clipboard suggestions
I personally find it quite useful to have an accessible toolbar when I start writing in an input field and then need to copy-paste some text from somewhere else.
My bad, what I meant was that empty suggestions are shown when next-word suggestions setting is enabled. |
@@ -1144,7 +1154,7 @@ public void onExtractedCursorMovement(final int dx, final int dy) { | |||
|
|||
@Override | |||
public void hideWindow() { | |||
if (mSuggestionStripView != null) | |||
if (mSuggestionStripView != null && !mSettings.getCurrent().mAutoShowToolbar) |
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.
Could you add some comment to the code on why hiding the toolbar depends on the auto-show setting? When reading just the code here, and not the entire PR one could easily assume it's a mistake.
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.
Actually it does not seem like a good idea to hide the toolbar only if the auto show setting is disabled if the reason for hiding the toolbar is #185.
Maybe it would be better to test whether it is possible to not hide the toolbar at all in hideWindow()?
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.
Why did you remove it? I just would have liked a comment in the code to avoid confusion, but now I am confused with this change...
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.
because I don't seem to replicate #185 which was the reasoning for adding it, was it not?
if you can, please check whether the bug is still there. I don't think that line should have been added in the first place..
it causes flickering on input start when auto showing the toolbar, and it does not make sense not to execute a bug prevention code just because some setting isn't disabled.
// Auto hide the toolbar if dictionary suggestions are available | ||
if (currentSettingsValues.mAutoHideToolbar | ||
&& !noSuggestionsFromDictionaries | ||
&& !suggestedWords.isPrediction()) { |
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.
Why don't hide if it's a prediction?
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.
because it would conflict with auto show on input start.
I could change it to this so predictions will auto hide the toolbar if the auto show setting is disabled:
if (currentSettingsValues.mAutoHideToolbar
&& !noSuggestionsFromDictionaries
&& (!suggestedWords.isPrediction() || !currentSettingsValues.mAutoShowToolbar)) {
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 just wanted to know why you added it like this, it was not a criticism or request for change.
Now you changed it to (!suggestedWords.isPrediction() || !currentSettingsValues.mAutoShowToolbar)) {
, but I'm still not sure I understand why. (and really, this is not a request to change it, just for clarification)
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.
Actually never mind, it seems predictions are not actually generated on input start, not sure why I thought they did.
I can simplify the condition to
if (currentSettingsValues.mAutoHideToolbar && !noSuggestionsFromDictionaries)
which should auto hide the toolbar when predictions are available
app/src/main/java/helium314/keyboard/latin/settings/CorrectionSettingsFragment.java
Outdated
Show resolved
Hide resolved
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.
@Helium314 feee2f2 is 100% going to cause a flicker on input start every time..
if you really think this is needed then you might to implement this function:
@Override
public void showWindow(boolean showInput) {
if (mSuggestionStripView != null && mSettings.getCurrent().mAutoShowToolbar)
mSuggestionStripView.setToolbarVisibility(true);
super.showWindow(showInput);
}
so that the toolbar visibility will be set ASAP, likely before the someone could notice it
@@ -256,7 +256,7 @@ private void updateKeys() { | |||
? km.isDeviceLocked() | |||
: km.isKeyguardLocked(); | |||
mToolbarExpandKey.setOnClickListener(hideToolbarKeys ? null : this); | |||
mPinnedKeys.setVisibility(hideToolbarKeys ? GONE : VISIBLE); | |||
mPinnedKeys.setVisibility(hideToolbarKeys ? GONE : mSuggestionsStrip.getVisibility()); |
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.
Selecting text by long pressing on text and moving the marker, the pinned keys flash on every move
Nice catch. I believe this change should fix it ^
} | ||
} else if (currentSettingsValues.mAutoShowToolbar) { | ||
final int codePoint = mInputLogic.mConnection.getCodePointBeforeCursor(); | ||
if (mInputLogic.mLastComposedWord == LastComposedWord.NOT_A_COMPOSED_WORD |
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.
It's a little tricky to auto show the toolbar on input start without causing a flicker, so I converted that requirement to this check every time an empty suggestion is about to be shown.
From my observation, the side effect is that now the main toolbar may also be shown when the cursor is moved to a position in which the previous word is not a valid word
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 think this condition is problematic so it's going to be removed:
if (mInputLogic.mLastComposedWord == LastComposedWord.NOT_A_COMPOSED_WORD
it does not seem to work as expected for my use case, which is to show the toolbar if the user hasn't typed anything yet in the input field.
It's because whenever a word is deleted, the last composed word is also affected as a result, which wasn't my intention.
// Auto hide the toolbar if dictionary suggestions are available | ||
if (currentSettingsValues.mAutoHideToolbar | ||
&& !noSuggestionsFromDictionaries | ||
&& !suggestedWords.isPrediction()) { |
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.
because it would conflict with auto show on input start.
I could change it to this so predictions will auto hide the toolbar if the auto show setting is disabled:
if (currentSettingsValues.mAutoHideToolbar
&& !noSuggestionsFromDictionaries
&& (!suggestedWords.isPrediction() || !currentSettingsValues.mAutoShowToolbar)) {
@@ -1144,7 +1154,7 @@ public void onExtractedCursorMovement(final int dx, final int dy) { | |||
|
|||
@Override | |||
public void hideWindow() { | |||
if (mSuggestionStripView != null) | |||
if (mSuggestionStripView != null && !mSettings.getCurrent().mAutoShowToolbar) |
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.
Actually it does not seem like a good idea to hide the toolbar only if the auto show setting is disabled if the reason for hiding the toolbar is #185.
Maybe it would be better to test whether it is possible to not hide the toolbar at all in hideWindow()?
@@ -1144,7 +1154,7 @@ public void onExtractedCursorMovement(final int dx, final int dy) { | |||
|
|||
@Override | |||
public void hideWindow() { | |||
if (mSuggestionStripView != null) | |||
if (mSuggestionStripView != null && !mSettings.getCurrent().mAutoShowToolbar) |
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.
because I don't seem to replicate #185 which was the reasoning for adding it, was it not?
if you can, please check whether the bug is still there. I don't think that line should have been added in the first place..
it causes flickering on input start when auto showing the toolbar, and it does not make sense not to execute a bug prevention code just because some setting isn't disabled.
// Auto hide the toolbar if dictionary suggestions are available | ||
if (currentSettingsValues.mAutoHideToolbar | ||
&& !noSuggestionsFromDictionaries | ||
&& !suggestedWords.isPrediction()) { |
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.
Actually never mind, it seems predictions are not actually generated on input start, not sure why I thought they did.
I can simplify the condition to
if (currentSettingsValues.mAutoHideToolbar && !noSuggestionsFromDictionaries)
which should auto hide the toolbar when predictions are available
Wouldn't it be better to have these settings in the new "Toolbar" page? 🤔 |
Wow. Sorry @codokie, GitHub didn't show me any of your comments from the past 2 days, only the pushes. So I was really confused first and then annoyed as it looked like you just pushed changes instead of answering my questions.
The toolbar settings don't exist in https://github.com/codokie/HeliBoard/tree/autotoggle-toolbar, but the plan to move the settings should be somewhere in here. |
Under which conditions? I tried a quite a few different things, but never managed to get that flicker.. |
No worries I had a feeling Github notifications may be broken. Thanks for merging my PR
You know what, never mind, I think most people won't be able to notice it unless their phone lags. |
I added a new setting that makes the suggestions appear automatically as they become available without having to touch the expand toolbar key. If there is no character before the cursor (when starting input or after a newline), the main toolbar will be shown. This functionality is present in many other mainstream keyboards. I believe this fixes #449
Here is a short demo
toolbar.mp4