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

Clipboard suggestions #647

Merged
merged 47 commits into from
Jul 5, 2024
Merged

Conversation

codokie
Copy link
Contributor

@codokie codokie commented Apr 5, 2024

I made the primary clipboard appear in the suggestion strip view as a suggested word whenever neutral suggestions are to be shown like in a beginning of sentence (fixes #167).
Here is an example of the feature:
example

In android 7.0+, the API allows detection of sensitive clipboard content, and when that happens the content is redacted like this:
password

Suggestions will disappear if they are picked or 3 minutes have passed.
There is also new setting in Suggestions settings that allows to toggle the suggestions.
There will be a suggestion for number input fields only if the clipboard content is a number.

P.S. You can long press the clipboard suggestion and then press the X icon to dismiss it

@RHJihan
Copy link
Contributor

RHJihan commented Apr 7, 2024

Nice work. ❤
One question: Is there a time limit for how long old clipboard content will be suggested?
The clipboard icon from the toolbar can be used for consistency, perhaps?

@Helium314
Copy link
Owner

Helium314 commented Apr 7, 2024

As usual, the PR is already 2 days old but I didn't even have time to go through the code...

I haven't tested, but "whenever neutral suggestions are to be shown" might not be quite the right thing.
HeliBoard might start showing suggestions right away when opening an input field, but when this is right after copying, the clipboard suggestion might still be wanted. Though the clipboard suggestion could be added to the list of normal suggestions in such a case.
On the other hand seeing the clipboard suggestion often for a long time might be seen as annoying / unnecessary.
I'm thinking about when it should not be shown any more. I'll ask in the issue to reach a few more users.

And maybe the icon should be the normal clipboard icon instead? I can imagine the icon looking a bit out of place depending on the theme / colors.

Maybe also the input field type should be considered, because e.g. clipboard text content should not be shown for a number field.

@RHJihan
Copy link
Contributor

RHJihan commented Apr 8, 2024

I'm thinking about when it should not be shown any more. I'll ask in the issue to reach a few more users.

My opinion is up to 90 seconds right after copying any text. Samsung keyboard does this for about 120 seconds.

setting now toggles suggestions.
@codokie
Copy link
Contributor Author

codokie commented Apr 9, 2024

In 7ce9d9f I made the following changes:

  • The unicode clipboard icon has been replaced with the default clipboard icon.

  • Suggestions will disappear if they are picked or if they are older than max history retention time.

  • The setting now completely toggles the clipboard suggestions instead of adjusting the visibility of its content

  • There will be a suggestion for number input fields only if the clipboard content is a number.

  • Adjusted postResumeSuggestions() performUpdateSuggestionStripSync() to resume suggestions if there is a recent clipboard item available. This should make the clipboard suggestions update more consistently even if Suggest correction text is off or the input type has no suggestions flag

Thanks for the feedback, and let me know what you think

@RHJihan
Copy link
Contributor

RHJihan commented Apr 9, 2024

Suggestions will disappear if they are picked or if they are older than max history retention time.

I am a little bit confused. What if someone toggles off the clipboard history (the default is 10 min) or sets the retention time to a bigger one? Or set that to No limit? Is it convenient to match the history retention time or to set some specific time, i.e., 90 seconds or 120 seconds?

There will be a suggestion for number input fields only if the clipboard content is a number.

It's impressive. Loved it.

The unicode clipboard icon has been replaced with the default clipboard icon.

It's looking good.

@codokie
Copy link
Contributor Author

codokie commented Apr 9, 2024

I am a little bit confused. What if someone toggles off the clipboard history (the default is 10 min) or sets the retention time to a bigger one? Or set that to No limit? Is it convenient to match the history retention time or to set some specific time, i.e., 90 seconds or 120 seconds?

I can easily set it to a constant time but personally feel that even 120 seconds is too short. If I copied something and something has caught my attention for 2 minutes (may happen frequently), I still want to be suggested the copied content afterwards.

If retention is set to "No limit", then the content will be suggested until the user has picked the suggestion or deleted it (by long pressing and pressing the bin button, very easy).
It will also stop appearing after the primary clipboard has been cleared (either manually or hourly in Android 13+).

It is possible to add a slider for setting the time but I haven't seen a keyboard who actually has such a setting.

Glad you liked the other additions :)

@RHJihan
Copy link
Contributor

RHJihan commented Apr 9, 2024

I can easily set it to a constant time but personally feel that even 120 seconds is too short. If I copied something and something has caught my attention for 2 minutes (may happen frequently), I still want to be suggested the copied content afterwards.

I will suggest seeing what other keyboards are doing in this situation, i.e., Gboard, Swiftkey, or Samsung Keyboard. I guess it will bring a better user experience.

@codokie
Copy link
Contributor Author

codokie commented Apr 9, 2024

Is the time limit specified somewhere? I'm not 100% sure but In Gboard at least the suggestion doesn't seem to disappear after 120 seconds

@RHJihan
Copy link
Contributor

RHJihan commented Apr 9, 2024

Is the time limit specified somewhere? I'm not 100% sure but In Gboard at least the suggestion doesn't seem to disappear after 120 seconds

on my phone's Gboard the clipboard suggestion disappears after about 120 seconds. Is this more than 120 seconds on your phone?

@codokie
Copy link
Contributor Author

codokie commented Apr 9, 2024

@RHJihan I've checked this again and I think you may be right. So I've set the suggestion to expire every 2 minutes. Actually it is inconsistent, should I revert it back?

If you have the time, could you please review the code or test this PR?

Also, I've noticed that in many cases when there are no (clipboard) suggestions and we are not in the middle of composing text, the suggestions are empty.
I think it would make more sense to show the main toolbar instead of the "neutral" suggestions. That's how it works in GBoard etc.

So in ba3f49b I've added that functionality, but if that's problematic it can be moved to a separate PR (will cause a merge conflict though).

@RHJihan
Copy link
Contributor

RHJihan commented Apr 10, 2024

Actually it is inconsistent, should I revert it back?

I am confused here, too. But I think clipboard suggestions shouldn't be very long. Anyone needing clipboard content after a specific time can always have it on the clipboard history view.

I tested the PR. Here are my thoughts:
If the history retention time is less than 120 seconds, I think clipboard suggestions should match that time.

If I copy text when another keyboard is active, after 120 seconds, it still shows as a clipboard suggestion on this PR. I am not sure if it is related to this PR. Maybe it's start counting the time after loading the keyboard. But it's not an important thing after all.

I am not that level of coder. So, I am giving my experience and perspective as a user and/or tester.

I think it would make more sense to show the main toolbar instead of the "neutral" suggestions. That's how it works in GBoard etc.

I think it's cool.

@codokie
Copy link
Contributor Author

codokie commented Apr 10, 2024

I think clipboard suggestions shouldn't be very long.

If others won't claim that 2 min is too short then I'll leave it at that.

If the history retention time is less than 120 seconds, I think clipboard suggestions should match that time.

I have added a setting called "Clear primary clipboard" in #611 (still open) to clear the primary clipboard too after history retention time.
With that setting enabled, it should match this behavior. Currently the history retention time only affects the internal clipboard and the clipboard suggestions use the primary one.

If I copy text when another keyboard is active, after 120 seconds, it still shows as a clipboard suggestion on this PR.

I think it is because the variables that keep information (timestamp, etc.) about the last primary clipboard entry are destroyed when the keyboard is changed. To fix this I can move those variables to the companion object.

I am giving my experience and perspective as a user and/or tester

That's exactly what I was looking for. Thank you for the feedback!

@RHJihan
Copy link
Contributor

RHJihan commented Apr 10, 2024

It is perfect now for me.

To fix this I can move those variables to the companion object.

After a quick try, this still happening. Force stop the keyboard app and enable it again will suggest old clipboard content as suggestion.

@RHJihan
Copy link
Contributor

RHJihan commented Apr 10, 2024

long press the clipboard suggestion and then press the bin icon to remove it

Is Samsung Keyboard's way of deleting suggestions better?

Screenshot_20240410_143317_Keep Notes

@codokie
Copy link
Contributor Author

codokie commented Apr 10, 2024

Force stop the keyboard app

If you force stop the app, the companion object will be wiped. To keep track of the clipboard timestamp, we need to store it as a variable because the API does not provide one.
So if the primary clipboard has content copied when you reopen the keyboard after force stopping it, a suggestion will (re)appear.
Actually starting from API 26 (android 8+) there is a clipboard timestamp, so it can be used instead.
For those who alternate between different keyboards, it should work as intended because the companion object is not erased.

Is Samsung Keyboard's way of deleting suggestions better?

Depending on how you look on it. The X button just dismisses the suggestion but keeps it buried in history.
If you are like me and like your history clean, deleting the entry instead of hiding it is better.

I could adjust the bin button to just dismiss the suggestion without affecting the primary/internal clipboard, but I think the icon may convey a message that the entry should be deleted.

If you don't like the suggestion you can also collapse the toolbar to get it out of view. Granted, it will reappear within 2 minutes if you restart the input

By the way, I would appreciate some help in making the suggestion background distinct from the suggestion strip view background like in other apps. I tried modifying it programmatically but it seems to affect only the pressed state of the TextView.

@RHJihan
Copy link
Contributor

RHJihan commented Apr 10, 2024

By the way, I would appreciate some help in making the suggestion background distinct from the suggestion strip view background like in other apps. I tried modifying it programmatically but it seems to affect only the pressed state of the TextView.

As I said earlier, I am not that level of coder. So I cannot help you with this. @Helium314 might help you.

I could adjust the bin button to just dismiss the suggestion without affecting the primary/internal clipboard, but I think the icon may convey a message that the entry should be deleted.

What about both? X button to dismiss the clipboard suggestion, but it can be accessed later on the clipboard history if a user would like to retain history. And the long press bin button to delete the clipboard entry.

I'm probably asking for much change. I am sorry for that.

@codokie
Copy link
Contributor Author

codokie commented Apr 10, 2024

That's ok thanks for the genuine feedback. I'll see if I can add such button

@codokie
Copy link
Contributor Author

codokie commented Apr 10, 2024

On a second thought, such a button would have to replace the pinned toolbar keys to ensure the suggestion strip view is not cluttered/confusing.
The pinned keys may still be useful for the user whenever there is a clipboard suggestion

@RHJihan
Copy link
Contributor

RHJihan commented Apr 10, 2024

On a second thought, such a button would have to replace the pinned toolbar keys to ensure the suggestion strip view is not cluttered/confusing.

Is it possible to place that button after any pinned toolbar button or on the left side right after the toolbar button?

@codokie
Copy link
Contributor Author

codokie commented May 29, 2024

Ok so I think I managed to make it styled similarly to inline suggestions, thanks for the tip to make the clipboard suggestions a separate textview. Here is how it looks now:
example

@Helium314
Copy link
Owner

Since you still don't use setInlineSuggestionsView and didn't provide information on why, I had a deeper look, and I think it should be possible (and preferable, as per contribution guidelines).

The idea when clipboard suggestion could be shown:
First LatinIME checks whether clipboard suggestions are wanted, and if yes calls something like mClipboardHistoryManager.createClipboardSuggestionView(editorInfo) (analogous to InlineAutofillUtils.createView).
If there is suitable clipboard content, ClipboardHistoryManager returns a non-null view with the listeners already in place, and LatinIME calls mSuggestionStripView.setInlineSuggestionsView (as mentioned previously, this could be renamed).

Is there anything fundamentally wrong with this approach?

@codokie
Copy link
Contributor Author

codokie commented Jun 2, 2024

It sounds doable and may reduce code from SuggestionStripView, but it would litter ClipboardHistoryManager, so I don't know about that..
Unlike inline suggestions, we need to keep reference of the clipboard suggestion view to programmatically change it inside the removeSuggestion(), onLongClickSuggestion() and clear() functions Nvm, we could use the View parameter of those functions as reference, but there would be a need for an indication that the view is indeed a clipboard suggestion

@Helium314
Copy link
Owner

it would litter ClipboardHistoryManager,

If you think ClipboardHistoryManager is going to grow too much, you could have the view creation happen in a utility file.

The re-use of the inline suggestion view should also reduce the need for clipboard-suggestion specific code in LatinIME.

Also creating a new textview each time would be less efficient than just keeping reference to it since only the text needs to be altered

Possibly, depending on how often it's really created. If this is an issue, a prepared view can be cached, and updated before use.

but there would be a need for an indication that the view is indeed a clipboard suggestion

Why is this? I don't have a full overview over the PR right now.
In any case it should not be an issue, the tag can be set to "clipboard" or something like that.

@codokie
Copy link
Contributor Author

codokie commented Jun 2, 2024

The re-use of the inline suggestion view should also reduce the need for clipboard-suggestion specific code in LatinIME.

But I did not use inline suggestion view for creating the clipboard suggestion view which is a TextView mostly defined in the xml.
I only used the background of the inline suggestion.

Sorry but I don't think it's worthwhile to use the inline view for this, if it's at all possible.

I understand wanting the reduce the code footprint but I think it makes sense to set the touch listener in the suggestion strip view and not in a utility file.

After 40 commits it sounds very tiring to make such a change that won't affect the appearance or usability of the feature

@Helium314
Copy link
Owner

Seriously, those parts of the contribution guidelines have been there since OpenBoard times. And they don't exist just because someone likes writing guidelines.
For me the whole process is also very frustrating, as I have to discuss in order to convince you to follow these guidelines, and not just in this PR.

I really would like to spend my time on code instead of discussions, but this kind of adding yet another thing for almost the same functionality is making the app harder to maintain. And I understand you might not be concerned about maintainability ("won't affect the appearance or usability of the feature").
But I need to maintain this app, and in some places it's already a nightmare where changes are very prone to have unintendend effects. I'm not exactly innocent in this respect (specifically looking at the keyboard parsing, which I tried to improve several times already for this reason), but there is a difference between this sort of issue arising for historic reasons and when it's plainly in sight and can be avoided with relatively low effort.
Re-using existing mechanism consistently contributes to maintainability of the code, because when making a change (much) later I don't have to figure out / remember why there are two different ways of showing a single "special" suggestion, and when doing changes I only have to be careful about not breaking one functionality instead of two.

I'm trying to improve the current situation, and I've had "Internal cleanup (a lot of over-complicated and convoluted code)" in the to do section for a while now. Currently it feels like I have to not just (slowly) work on this, but also have to do discussions over and over again that such things do not find their way into the app via PRs.

I have "still it would be good if you announced that you are working on it, so we can discuss how changes are best implemented" in the contribution guidelines, and I think discussing on how to implement larger changes before most of the code is written is a good way to avoid me asking to completely change your code.
Could be in the issue, or by starting a draft PR of a (possibly) non-functional version sketching your idea.

I'm sorry if this comment comes across as overly criticising, so I want to explicitly mention that I definitely do value your contributions (and I'm not happy that I don't have time to keep up with your pace of PRs).

@codokie
Copy link
Contributor Author

codokie commented Jun 8, 2024

This PR is for adding clipboard suggestions, not abstracting the creation of views in the suggestion strip view.
I've tried before to use the existing mechanism of the inline suggestions but could not get it to work.
Clipboard suggestion is a TextView and its content is not generated by the autofill API.
If you want you can merge the PR as-is or implement the "relatively low effort" change you think is required.
I'm sorry but I don't time or will to work on this anymore

@Helium314
Copy link
Owner

Helium314 commented Jul 3, 2024

I adjusted the PR to use the (now renamed) setInlineSuggestionsView, and create the view in ClipboardHistoryManager.
Behavior is same, or very close to, except for:

  • always showing the dismiss icon (I assume users may want to dismiss the suggestion relatively often and should have an easy way to do it)
  • suggestion is not disappearing automatically after 3 min (I think there is no real benefit from this)

Further I removed some crash sources (using methods / resources not available on devices with the current minApi).

I'll have another look at it in the next few days and plan to merge it then.

@Helium314 Helium314 merged commit bdab98c into Helium314:main Jul 5, 2024
1 check passed
@RHJihan
Copy link
Contributor

RHJihan commented Jul 6, 2024

I haven't tested this merged PR yet. But I just wanted to know:

suggestion is not disappearing automatically after 3 min (I think there is no real benefit from this)

So, will the clipboard suggestion appear every time (if the clipboard has something copied already) I try to type something unless I dismiss it manually?

@Helium314
Copy link
Owner

It will not appear if the clip is more than 3 min old. But it will not disappear on its own if you just wait and stare at the keyboard for 3 minutes.

@codokie codokie deleted the clipboard-suggestions branch July 13, 2024 15:12
@codokie
Copy link
Contributor Author

codokie commented Jul 13, 2024

@Helium314 Thanks for finalizing this pull request. I look forward to test this feature in the upcoming beta

@Helium314
Copy link
Owner

I hope the implementation is ok for you. I tried to keep behavior close to your version when moving most of the stuff into that one view.
(Will also work on your other PRs, but that will take some time. Especially because I'll be barely available for the next 3 weeks.)

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.

Show paste button in suggestions after copy to clipboard
4 participants