WIP: Search in Card Browser for highlighted text from Review mode by Profpatsch · Pull Request #4708 · ankidroid/Anki-Android

Profpatsch · 2017-09-20T00:26:11Z

I tried my hands at #4496, but that’s as far as I can get. See commit message for the problem.
Basically I can’t find a way to add the callbacks to the action mode. Also, I have no idea what action mode I’m looking at.

I thought about tracking for long presses, but then the question becomes how to still get the text selection functionality.

@timrae: Maybe you have an idea how to implement this functionality?

timrae · 2017-09-20T01:43:15Z

@Profpatsch

Thanks for having a go at this. It took me a while to find the article, but this is the way that I was imagining it should be implemented in my comment on #4496. By default, that would add an item to the floating action bar whenever text is selected inside any app, which may not be what users want. But according to the comments section of that article, an issue was added and subsequently fixed that would allow you to limit the visibility of the intents to your own app if you set exported to false, which I think we should do.

timrae · 2017-09-20T01:44:08Z

FYI, this may also be helpful, though I'm not sure how up-to-date it is.

Profpatsch · 2017-09-28T01:52:11Z

Okay, I rewrote it to a very simple implementation based on ACTION_PROCESS_TEXT.

It’s still visible from other apps because the hiding functionality is not in Android yet. I also don’t know if I have implemented it in a backwards-compatible manner, since I only have a new device to test it on.

timrae

ACTION_PROCESS_TEXT is only available from API 23, so you'll need to move any code that uses it to our Compat classes.

timrae · 2017-09-28T02:11:53Z

Normally we just test with the simulator during development

timrae · 2017-09-28T02:17:54Z

setHandleNativeActionModesEnabled is available on 4.0 and above. I think we're finally ready to bump the minimum supported version up to 4.1 for v2.9, but we'd better announce it on the forums.

Profpatsch · 2017-09-28T15:15:04Z

Okay, I moved the ACTION_PROCESS_TEXT-specific logic into a Compat method.

timrae · 2017-09-28T15:21:29Z

Almost, but that's not quite going to work; you need to instantiate the correct Compat class for the correct API using CompatHelper:

https://github.com/ankidroid/Anki-Android/blob/master/AnkiDroid/src/main/java/com/ichi2/compat/CompatHelper.java

Profpatsch · 2017-09-28T15:38:10Z

Right. Added v23 to the dispatcher and switched to the singleton method.

Profpatsch · 2017-10-03T16:47:52Z

@timrae Any further remarks?

timrae · 2017-10-03T19:57:35Z

Thanks for reminding me. Have you tested it properly with the simulator running Android 4.0? I'm also still unsure about this `exported` issue... When was support for this added into the Android framework? One of us will need to check the framework code. I need to make a thread on the forum about the change in minimum API, and we should probably make a maintenance release soon before dropping support for 2.3.

…

On 4/10/2017 01:47, "Profpatsch" ***@***.***> wrote: @timrae <https://github.com/timrae> Any further remarks? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4708 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4ubBc3sRoiwFvucpcWC4WwZha4Pzks5somU6gaJpZM4PdL3B> .

timrae · 2017-10-03T21:36:38Z

I've made the announcement that Android 4.0 will be the new minimum going forward. We can bump the minimum again to 4.1 whenever the need arises.

Profpatsch · 2017-10-04T00:07:39Z

I'm also still unsure about this exported issue...
When was support for this added into the Android framework? One of us will
need to check the framework code.

I’m actually not sure if it wouldn’t be better to export it. That way, if you find e.g. a word in your browser, you can highlight it and directly see if you already have it in your vocab list, which is awesome.
The item text uses the Activity name, so it might be sensible to rename it to “Vocab Browser” for example?

Update: Right now the browser crashes once I click “Card Browser” though, probably a SecurityException?

timrae · 2017-10-04T00:35:33Z

I’m actually not sure if it wouldn’t be better to export it. That way, if you find e.g. a word in your browser, you can highlight it and directly see if you already have it in your vocab list, which is awesome.

This sounds like it could be useful if we could streamline the process of note addition from the browser a bit better, so that when you choose "Add note" from the browser while there is something in the search box, it automatically populates that text into the first field of the note editor.

timrae · 2017-10-04T00:36:52Z

The item text uses the Activity name, so it might be sensible to rename it to “Vocab Browser” for example?

There's no way to customize it without changing the Activity name?

Update: Right now the browser crashes once I click “Card Browser” though, probably a SecurityException?

What's the stacktrace?

Profpatsch · 2017-10-04T00:58:10Z

There's no way to customize it without changing the Activity name?

At least that’s what the article says:

I can’t find any documentation other than https://developer.android.com/reference/android/content/Intent.html#ACTION_PROCESS_TEXT and that medium article; apparently Medium is the way the Android team distributes their docs?

What's the stacktrace?

10-04 02:57:31.336  2182 22848 I ActivityManager: START u0 {act=android.intent.action.PROCESS_TEXT typ=text/plain cmp=com.ichi2.anki/.CardBrowser (has extras)} from uid 10050 on display 0
10-04 02:57:31.336  2182 22848 W ActivityManager: Permission Denial: starting Intent { act=android.intent.action.PROCESS_TEXT typ=text/plain cmp=com.ichi2.anki/.CardBrowser (has extras) } from ProcessRecord{5ad81a9 26760:org.lineageos.jelly/u0a50} (pid=26760, uid=10050) not exported from uid 10069
10-04 02:57:31.339 26760 26760 D AndroidRuntime: Shutting down VM
10-04 02:57:31.340 26760 26760 E AndroidRuntime: FATAL EXCEPTION: main
10-04 02:57:31.340 26760 26760 E AndroidRuntime: Process: org.lineageos.jelly, PID: 26760
10-04 02:57:31.340 26760 26760 E AndroidRuntime: java.lang.SecurityException: Permission Denial: starting Intent { act=android.intent.action.PROCESS_TEXT typ=text/plain cmp=com.ichi2.anki/.CardBrowser (has extras) } from ProcessRecord{5ad81a9 26760:org.lineageos.jelly/u0a50} (pid=26760, uid=10050) not exported from uid 10069
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.os.Parcel.readException(Parcel.java:1684)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.os.Parcel.readException(Parcel.java:1637)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.app.ActivityManagerProxy.startActivity(ActivityManagerNative.java:3101)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.app.Instrumentation.execStartActivity(Instrumentation.java:1641)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.app.Activity.startActivityForResult(Activity.java:4879)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.content.ContextWrapper.startActivityForResult(ContextWrapper.java:368)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.view.View.startActivityForResult(View.java:5845)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.widget.Editor$ProcessTextIntentActionsHandler.fireIntent(Editor.java:6246)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.widget.Editor$ProcessTextIntentActionsHandler.performMenuItemAction(Editor.java:6200)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.widget.Editor$TextActionModeCallback.onActionItemClicked(Editor.java:3786)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.policy.DecorView$ActionModeCallback2Wrapper.onActionItemClicked(DecorView.java:2303)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.view.FloatingActionMode$3.onMenuItemSelected(FloatingActionMode.java:88)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.view.menu.MenuBuilder.dispatchMenuItemSelected(MenuBuilder.java:761)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.view.menu.MenuItemImpl.invoke(MenuItemImpl.java:152)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:904)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:894)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.view.FloatingActionMode$4.onMenuItemClick(FloatingActionMode.java:114)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.widget.FloatingToolbar$FloatingToolbarPopup$15.onItemClick(FloatingToolbar.java:1379)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.widget.AdapterView.performItemClick(AdapterView.java:310)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.widget.AbsListView.performItemClick(AbsListView.java:1164)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.widget.AbsListView$PerformClick.run(AbsListView.java:3139)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.widget.AbsListView$3.run(AbsListView.java:4054)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.os.Handler.handleCallback(Handler.java:751)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.os.Handler.dispatchMessage(Handler.java:95)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.os.Looper.loop(Looper.java:154)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at android.app.ActivityThread.main(ActivityThread.java:6186)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at java.lang.reflect.Method.invoke(Native Method)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:889)
10-04 02:57:31.340 26760 26760 E AndroidRuntime: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:779)
10-04 02:57:31.343  2182 22597 W ActivityManager:   Force finishing activity org.lineageos.jelly/.MainActivity

timrae · 2017-10-04T01:21:05Z

Hmmm, this stack trace was generated with exported=true?

Permission Denial: starting Intent { act=android.intent.action.PROCESS_TEXT typ=text/plain cmp=com.ichi2.anki/.CardBrowser (has extras) } from ProcessRecord{5ad81a9 26760:org.lineageos.jelly/u0a50} (pid=26760, uid=10050) not exported from uid 10069

timrae · 2017-10-04T01:26:33Z

I'm a bit confused, as I would have thought that on older versions of Android the PROCESS_TEXT intent is simply never sent...

Profpatsch · 2017-10-04T01:42:46Z

Hmmm, this stack trace was generated with exported=true?

I understand now, I should have changed the exported attribute to true. Once I did, it works.

Calling an `ACTION_PROCESS_TEXT` intent from other applications was still possible, but they would crash with a `SecurityException` because the Activity was not exported.

timrae · 2017-10-04T01:46:25Z

Instead of that wrapper function inside the Compat classes, I think it might be more simple to just port over the missing constants from the framework.

public class CompatV10 implements Compat {
    public static final String ACTION_PROCESS_TEXT = "android.intent.action.PROCESS_TEXT";
    public static final String EXTRA_PROCESS_TEXT = "android.intent.extra.PROCESS_TEXT";
    // ....
}

public class CompatV23 implements Compat {
    public static final String ACTION_PROCESS_TEXT = Intent.ACTION_PROCESS_TEXT;
    public static final String EXTRA_PROCESS_TEXT = Intent.EXTRA_PROCESS_TEXT;
    // ...
}

Then you can write the code more naturally inside the reviewer using getCompat().ACTION_PROCESS_TEXT for the constant.

timrae · 2017-10-04T01:50:42Z

I understand now, I should have changed the exported attribute to true. Once I did, it works.

Ah right, yes, the activity itself...

Profpatsch · 2017-10-04T02:05:18Z

Instead of that wrapper function inside the Compat classes, I think it might be more simple to just port over the missing constants from the framework.

Ah right, yes, the activity itself...

So no need to copy the strings, right?

timrae · 2017-10-04T02:15:15Z

So no need to copy the strings, right?

Sorry I'm not following your question... Did you see the code sample I posted above?

Profpatsch · 2017-10-04T03:52:05Z

Sorry I'm not following your question... Did you see the code sample I posted above?

Yeah, but I’m not quite sure what this would simplify and how. Do you mean I should remove this function alltogether?

timrae · 2017-10-04T14:31:28Z

Yes, the only part of the function that is not available on all APIs is the two constants, right?

Profpatsch · 2017-10-04T23:03:13Z

Ah! Now I get it. Should have gone to bed earlier yesterday. :)
Will do.

Profpatsch · 2017-10-04T23:52:24Z

I implemented it; the final strings have to be in the interface, otherwise they cannot be used with getCompat() (which returns the interface). I refuse to cast and break the abstraction. ;)

Tested with KitKat Emulator and on API 23 device.

timrae · 2017-10-05T00:04:43Z

AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.java

+        Compat compat = CompatHelper.getCompat();
+        if (intent.getAction() == compat.ACTION_PROCESS_TEXT) {
+            CharSequence search = intent.getCharSequenceExtra(compat.EXTRA_PROCESS_TEXT);
+            assert search != null;


Thanks, this is much clearer! I think we don't really want to crash if an app developer accidentally doesn't add EXTRA_PROCESS_TEXT... Can you get rid of the assert and just write the following?

if (search != null && search.length() != 0) {

Last nitpick I promise :)

I think we don't really want to crash if an app developer accidentally doesn't add EXTRA_PROCESS_TEXT

That’s something that should never happen, which is why I use an assert.
From the ACTION_PROCESS_TEXT documentation:

Activity Action: Process a piece of text. Input: EXTRA_PROCESS_TEXT contains the text to be processed. EXTRA_PROCESS_TEXT_READONLY states if the resulting text will be read-only. Output: EXTRA_PROCESS_TEXT contains the processed text.

From the ACTION_PROCESS_TEXT documentation

It doesn't actually say anywhere there that it won't be null, or that it's guaranteed to be present. In fact it will only be there if the developer sending the intent calls putExtra(). You can see this from the getCharSequenceExtra() documentation, which says:

(returns) the value of an item that previously added with putExtra() or null if no CharSequence value was found.

FYI: In general, we try to only use assert() to pick up programming errors by us, whereas this would be picking up programming errors by others (i.e. other apps, android framework).

timrae · 2017-10-05T00:06:22Z

AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.java

 import com.ichi2.async.DeckTask.TaskData;
+import com.ichi2.compat.Compat;
+import com.ichi2.compat.CompatHelper;
+import com.ichi2.compat.CompatV23;


Oh I guess this import isn't needed anymore btw

timrae · 2017-10-05T00:06:50Z

AnkiDroid/src/main/java/com/ichi2/compat/CompatV23.java

+import android.annotation.TargetApi;
+import android.content.Intent;
+
+import timber.log.Timber;


Also not needed

The compiler doesn’t show these as warnings … :(

Yeah, most compilers don't unfortunately, even in other programming languages...You can run "optimize imports" with Android Studio which will take care of this for you automatically.

Profpatsch · 2017-10-06T01:42:06Z

Unneeded imports & assert have been fixed.

Want to say that I’m impressed by the amount attention and help you have given for this rather minor feature, @timrae.

timrae · 2017-10-06T01:55:36Z

AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.java

+        if (intent.getAction() == compat.ACTION_PROCESS_TEXT) {
+            CharSequence search = intent.getCharSequenceExtra(compat.EXTRA_PROCESS_TEXT);
+            if (search != null && search.length() != 0) {
+                Timber.d("CardBrowser :: Called with search intent", search);


I guess you forgot to add the %s formatting string here so that the content of search gets printed out...?

Oh, it’s a format string. Wasn’t clear from the documentation, thought it would display them automatically in a smart way.

It is enough to mock the Intent string constants for older API versions, this way the logic stays at the correct place.

timrae · 2017-10-06T02:27:29Z

Thank you! :)

timrae · 2017-10-06T02:28:25Z

Are you interested in implementing the idea I posted above to allow adding a new note from the content of the browser search bar?

Profpatsch · 2017-11-03T11:34:06Z

Are you interested in implementing the idea I posted above to allow adding a new note from the content of the browser search bar?

At the moment I’m short on time, but I’ll put it on my side-project list. (The tab with this question has been open in my browser for nearly a month now. :))

Profpatsch added 2 commits September 28, 2017 03:44

Add ACTION_PROCESS_TEXT capability to CardBrowser

1799cae

Automatically start search on ACTION_PROCESS_TEXT

bd2842c

Profpatsch force-pushed the search-from-review branch from b9778fe to bd2842c Compare September 28, 2017 01:46

timrae suggested changes Sep 28, 2017

View reviewed changes

Profpatsch force-pushed the search-from-review branch from 8ad8c02 to ac8eb01 Compare September 28, 2017 15:13

Move ACTION_PROCESS_TEXT logic into Compat class

8b7f2f5

Profpatsch force-pushed the search-from-review branch from ac8eb01 to 8b7f2f5 Compare September 28, 2017 15:37

timrae approved these changes Oct 4, 2017

View reviewed changes

Export Card Browser to prevent SecurityException

03c529b

Calling an `ACTION_PROCESS_TEXT` intent from other applications was still possible, but they would crash with a `SecurityException` because the Activity was not exported.

timrae reviewed Oct 5, 2017

View reviewed changes

Profpatsch force-pushed the search-from-review branch from 8bff82d to 18c5e08 Compare October 6, 2017 01:41

timrae reviewed Oct 6, 2017

View reviewed changes

Move ACTION_PROCESS_TEXT handling from Compat23 to CardBrowser

971a53e

It is enough to mock the Intent string constants for older API versions, this way the logic stays at the correct place.

Profpatsch force-pushed the search-from-review branch from 18c5e08 to 971a53e Compare October 6, 2017 02:21

timrae merged commit ce0404d into ankidroid:master Oct 6, 2017

timrae mentioned this pull request Oct 9, 2018

Clear search intent info after retrieving it so we aren't stuck in a loop #5038

Merged

5 tasks

Uh oh!

Conversation

Profpatsch commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timrae commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timrae commented Sep 20, 2017

Uh oh!

Profpatsch commented Sep 28, 2017

Uh oh!

timrae left a comment

Choose a reason for hiding this comment

Uh oh!

timrae commented Sep 28, 2017

Uh oh!

timrae commented Sep 28, 2017

Uh oh!

Profpatsch commented Sep 28, 2017

Uh oh!

timrae commented Sep 28, 2017

Uh oh!

Profpatsch commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Profpatsch commented Oct 3, 2017

Uh oh!

timrae commented Oct 3, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timrae commented Oct 3, 2017

Uh oh!

Profpatsch commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timrae commented Oct 4, 2017

Uh oh!

timrae commented Oct 4, 2017

Uh oh!

Profpatsch commented Oct 4, 2017

Uh oh!

timrae commented Oct 4, 2017

Uh oh!

timrae commented Oct 4, 2017

Uh oh!

Profpatsch commented Oct 4, 2017

Uh oh!

timrae commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timrae commented Oct 4, 2017

Uh oh!

Profpatsch commented Oct 4, 2017

Uh oh!

timrae commented Oct 4, 2017

Uh oh!

Profpatsch commented Oct 4, 2017

Uh oh!

timrae commented Oct 4, 2017

Uh oh!

Profpatsch commented Oct 4, 2017

Uh oh!

Profpatsch commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Profpatsch commented Sep 20, 2017 •

edited

Loading

timrae commented Sep 20, 2017 •

edited

Loading

Profpatsch commented Sep 28, 2017 •

edited

Loading

timrae commented Oct 3, 2017 via email •

edited

Loading

Profpatsch commented Oct 4, 2017 •

edited

Loading

timrae commented Oct 4, 2017 •

edited

Loading

Profpatsch commented Oct 4, 2017 •

edited

Loading

Profpatsch commented Oct 6, 2017 •

edited

Loading

Profpatsch commented Nov 3, 2017 •

edited

Loading