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

android: add support for 3rd party dictionary apps #5108

Merged
merged 1 commit into from Jul 8, 2019

Conversation

@pazos
Copy link
Contributor

commented Jul 4, 2019

Added support for Aard2, Alpus, Colordict, Fora, Goldendict, Kiwix, Mdict and Quickdic.

Goldendict Pro is untested. The Free version works fine.

All credits to @poire-z, who did most of the job in #2333 (comment)

how to add support for new dictionary apps ?

  1. find a target dict app (already installed, configured and ready to work)
  2. find its package name (using an app like "Package Names Viewer")
  3. try generic actions in this order: picker-share -> picker-send -> picker-text.
  4. hopefully the application is available as an option to open the query.
  5. try again removing the "picker-", It should open your dictionary without questions.

You might need to look at the app AndroidManifest.xml and find the kind of intents that the application handles.

Requires koreader/android-luajit-launcher#156
Fixes #3496
Fixes #5039

@pazos pazos requested review from Frenzie and poire-z Jul 4, 2019

@pazos pazos added this to the 2019.07 milestone Jul 4, 2019

@@ -86,6 +111,7 @@ function Device:init()
or ev.code == C.APP_CMD_WINDOW_REDRAW_NEEDED then
this.device.screen:_updateWindow()
elseif ev.code == C.APP_CMD_RESUME then
this.device.getExternalDictLookupList = getExternalDicts

This comment has been minimized.

Copy link
@poire-z

poire-z Jul 4, 2019

Contributor

What does that do?
It looks like you just re-assigning that function to the that name, and I see no reason for getExternalDictLookupList to have been assigned something else in between (?).

This comment has been minimized.

Copy link
@pazos

pazos Jul 4, 2019

Author Contributor

it updates the list of available dicts when the app is resumed (because the user can install or uninstall software)

This comment has been minimized.

Copy link
@poire-z

poire-z Jul 4, 2019

Contributor

But you're just re-assigning that function to that name, not actually calling it. So that's a no-op.
And you're actually calling it in doExternalDictLookup , so doing the android.isPackageEnabled at each lookup (as you got rid of my EXTERNAL_DICTS_AVAILABILITY_CHECKED flag).
If you really care about rechecking availability, you should keep that kind of flag (to avoid rechecking on each lookup in the same foreground session), and just unset it onResume, so the next lookup (only) do the availability check.
(Or do I miss something?)

{ "Aard2", "Aard2", false,
{
package="itkach.aard2",
action="aard2",

This comment has been minimized.

Copy link
@poire-z

poire-z Jul 4, 2019

Contributor

If there is never anything else than these 2 "package" and "action", you could as well have them in the main list, and access them with v[4] and v[5].

{ "Aard2", "Aard2", false, "itkach.aard2", "aard2"},
{ "Alpus", "Alpus", false, "com.ngcomputing.fora.android", "search"}
{ "ColorDict", "ColorDict", false, "com.socialnmobile.colordict", "colordict"}
[...]

would look nice enough.

local function genExternalDictItems()
local items_table = {}
for i, v in ipairs(Device:getExternalDictLookupList()) do
table.insert(items_table, {

This comment has been minimized.

Copy link
@Frenzie

Frenzie Jul 4, 2019

Member

Some people object to what they call useless variables, but I'm of the opinion that something like this is easier to comprehend at a glance and therefore preferable:

local setting = v[1]
local text = v[2]
local enabled = v[3]

@pazos pazos force-pushed the pazos:android_dicts branch from 60d4d65 to 08247fb Jul 4, 2019

@poire-z

poire-z approved these changes Jul 4, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@poire-z: about your comment in #2333 (comment)

We need to refresh the table when the activity is resumed (because an user can install/uninstall software in between).

Is the packages-installed-checks expensive?

Not really expensive but could be cheaper. The overhead is in the JNI interface for android.isPackageAvailable and we do this for each dict. It takes a few milliseconds anyways.

Is that really needed to do it each time KOReader is put to foreground? Once some dict are installed, we may never install any new ones. And if the user installs one and doesn't see it in the menu, he will naturally think that it needs a real quit/restart.

👍 I will remove the onResume code.

What is left to do is clean the hightlight before or after we start the new activity.

untitled

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

What is left to do is clean the hightlight before or after we start the new activity.

You could just call self.highlight:clear() before calling Device:doExternalDictLookup().

But if launching an external dict lookup always bring another window on the foregroud, and quitting it brings KOReader in the foreground, and if that triggers one of your elseif ev.code == C.APP_CMD_GAINED_FOCUS... or elseif ev.code == C.APP_CMD_RESUME then, you could pass a callback to Device:doExternalDictLookup() which would get called when one of these event happen, and it could do a bit of that:

if self.highlight then
-- delay unhighlight of selection, so we can see where we stopped when
-- back from our journey into dictionary or wikipedia
local clear_id = self.highlight:getClearId()
UIManager:scheduleIn(0.5, function()
self.highlight:clear(clear_id)
end)
end

Something like

-- local at the module level
local external_dict_when_back_callback = nil
[...]
    doExternalDictLookup = function (self, text, method, when_back_callback)
        external_dict_when_back_callback = when_back_callback
        local package, action = nil
        [...]

            elseif ev.code == C.APP_CMD_RESUME then
                EXTERNAL_DICTS_AVAILABILITY_CHECKED = false
                if external_dict_when_back_callback then
                    external_dict_when_back_callback()
                    external_dict_when_back_callback = nil
                end

    if Device:canExternalDictLookup() and G_reader_settings:isTrue("external_dict_lookup") then
        Device:doExternalDictLookup(word, G_reader_settings:readSetting("external_dict_lookup_method"), function()
            if self.highlight then
                -- delay unhighlight of selection, so we can see where we stopped when
                -- back from our journey into dictionary or wikipedia
                local clear_id = self.highlight:getClearId()
                UIManager:scheduleIn(0.5, function()
                    self.highlight:clear(clear_id)
                end)
            end
        end)
        return
    end
@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

thank you very much! It works nice for normal lookups inside documents. I can still reproduce the same issue on the Dictionary lookup history widget.

BTW, logcat reports

07-05 01:28:16.772  3300  3314 D DICT    : Package itkach.aard2 is installed. Enabled? -> true
07-05 01:28:16.773  3300  3314 D DICT    : Package com.ngcomputing.fora.android is not installed.
07-05 01:28:16.774  3300  3314 D DICT    : Package com.socialnmobile.colordict is installed. Enabled? -> true
07-05 01:28:16.774  3300  3314 D DICT    : Package com.ngc.fora is not installed.
07-05 01:28:16.775  3300  3314 D DICT    : Package mobi.goldendict.android.free is not installed.
07-05 01:28:16.775  3300  3314 D DICT    : Package mobi.goldendict.android.pro is not installed.
07-05 01:28:16.776  3300  3314 D DICT    : Package org.kiwix.kiwixmobile is installed. Enabled? -> true
07-05 01:28:16.776  3300  3314 D DICT    : Package cn.mdict is not installed.
07-05 01:28:16.777  3300  3314 D DICT    : Package de.reimardoeffinger.quickdic is installed. Enabled? -> true

So each jni call <= 1ms.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I can still reproduce the same issue on the Dictionary lookup history widget.

What issue? The thing I can gather from your animated screenshots (you didn't say it was an issue, so I didn't look for one before :) : the reverted black bar over the item you tap that doesn't get unreverted?

That may be some refresh/redraw issue: the unrevert is UIManager:scheduleIn'ed, and KOReader lose focus before the delay happen and may be luajit is suspended, or some push of the buffer to android layer is lost or something. I guess you don't get that if you uncheck Screen > EInk settings > Flash buttons and menu items ? (Pinging @NiLuJe who love these flashes :)

About the book selected text highlight and its clearing, also look how it behave if you do the Dict lookup from the multiple-words-selected dialog (my above suggestion may clear the highlight early - it should stay highlighted till that button dialog is closed - but may be just check it does not crash :)

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

About the book selected text highlight and its clearing, also look how it behave if you do the Dict lookup from the multiple-words-selected dialog (my above suggestion may clear the highlight early - it should stay highlighted till that button dialog is closed - but may be just check it does not crash :)

It behaves exactly as you expected 👍

That may be some refresh/redraw issue: the unrevert is UIManager:scheduleIn'ed, and KOReader lose focus before the delay happen and may be luajit is suspended, or some push of the buffer to android layer is lost or something. I guess you don't get that if you uncheck Screen > EInk settings > Flash buttons and menu items ?

That's correct. Cannot reproduce disabling "Flash buttons and menu items"

Here are the logs

07-05 17:01:15.505  3407  3420 V KOReader:  dict lookup word: location
07-05 17:01:15.505  3407  3420 V KOReader:  dict stripped word: location
07-05 17:01:15.517  3407  3420 D KOReader: Package itkach.aard2 is installed. Enabled? -> true
07-05 17:01:15.523  3407  3420 D KOReader: Package com.ngcomputing.fora.android is not installed.
07-05 17:01:15.528  3407  3420 D KOReader: Package com.socialnmobile.colordict is installed. Enabled? -> true
07-05 17:01:15.528  3407  3420 D KOReader: Package com.ngc.fora is not installed.
07-05 17:01:15.529  3407  3420 D KOReader: Package mobi.goldendict.android.free is not installed.
07-05 17:01:15.530  3407  3420 D KOReader: Package mobi.goldendict.android.pro is not installed.
07-05 17:01:15.534  3407  3420 D KOReader: Package org.kiwix.kiwixmobile is installed. Enabled? -> true
07-05 17:01:15.535  3407  3420 D KOReader: Package cn.mdict is not installed.
07-05 17:01:15.538  3407  3420 D KOReader: Package de.reimardoeffinger.quickdic is installed. Enabled? -> true
07-05 17:01:15.545  3407  3407 V threaded_app: WindowFocusChanged: 0xa662dfe0 -- 0
07-05 17:01:15.545  3407  3407 D KOReader: App paused
07-05 17:01:15.546  3407  3407 V threaded_app: Pause: 0xa662dfe0
07-05 17:01:15.550  3407  3420 V KOReader:  Explicit widgetRepaint: table: 0x91a86710 @ ( 18 , 1502.5 )
07-05 17:01:15.555  3407  3420 V KOReader:  setDirty via a func from widget nil
07-05 17:01:15.555  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:15.555  3407  3420 V KOReader:  _refresh: Enqueued ui update for region 18 1502.5 1044 72 
07-05 17:01:15.598  3407  3420 V KOReader:  input event => type: 4, code: 7(0), value: 1, time: 1562338875.598348
07-05 17:01:15.598  3407  3420 V KOReader:  Android application event 7
07-05 17:01:15.598  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:15.598  3407  3420 V threaded_app: activityState=13
07-05 17:01:15.599  3407  3420 V KOReader:  input event => type: 4, code: 13(6), value: 1, time: 1562338875.598819
07-05 17:01:15.599  3407  3420 V KOReader:  Android application event 13
07-05 17:01:15.599  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:15.882  3407  3420 V KOReader:  BackgroundRunner: _execute() @  1562338875
07-05 17:01:15.882  3407  3420 V KOReader:  BackgroundRunnerWidget: start running @  1562338875
07-05 17:01:15.882  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:15.883  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:17.643  3407  3407 V threaded_app: WindowFocusChanged: 0xa662dfe0 -- 1
07-05 17:01:17.643  3407  3420 V KOReader:  input event => type: 4, code: 6(nil), value: 1, time: 1562338877.643446
07-05 17:01:17.643  3407  3420 V KOReader:  Android application event 6
07-05 17:01:17.726  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:17.875  3407  3407 D KOReader: App resumed
07-05 17:01:17.884  3407  3420 V KOReader:  BackgroundRunner: _execute() @  1562338877
07-05 17:01:17.884  3407  3420 V KOReader:  BackgroundRunnerWidget: start running @  1562338877
07-05 17:01:17.884  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:17.884  3407  3420 V KOReader:  not painting 1 covered widget(s)
07-05 17:01:17.909  3407  3407 V threaded_app: Resume: 0xa662dfe0
07-05 17:01:17.910  3407  3420 V threaded_app: activityState=11
07-05 17:01:17.910  3407  3420 V KOReader:  input event => type: 4, code: 11(4), value: 1, time: 1562338877.910033
07-05 17:01:17.910  3407  3420 V KOReader:  Android application event 11
07-05 17:01:17.910  3407  3420 V KOReader:  not painting 1 covered widget(s)

I would expect a "cannot blit" message from https://github.com/koreader/koreader-base/blob/master/ffi/framebuffer_android.lua#L70 if the app tries to write to a window when the surface is not available.

android: add support for 3rd party dictionary apps
Co-Authored-By: poire-z <poire-z@users.noreply.github.com>

@pazos pazos force-pushed the pazos:android_dicts branch from 28d7abc to 897c186 Jul 5, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

It behaves exactly as you expected 👍

Meaning (a) the highlight is cleared (too early?) or (b) stay highlighted till that button dialog is closed? If (a), is it not bothering enough?

I would expect a "cannot blit" message

Well, I don't know enough about the android framebuffer/native surface business to give you any hint.
May be try to have KOReader do a full window repaint on Resume, or full transfer from its own buffer (if it has an intermediate one) to the Android surface (if it is different). May be the regional highlight-revert was lost and not pushed to android, and after the resume, only other regional paintings/refreshes were pushed to Android?

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@poire-z

Meaning (a) the highlight is cleared (too early?) or (b) stay highlighted till that button dialog is closed? If (a), is it not bothering enough?

sorry, I was not clear enough. I meant (b) stay highlighted till that button dialog is closed. All good.

Well, I don't know enough about the android framebuffer/native surface business to give you any hint.
May be try to have KOReader do a full window repaint on Resume, or full transfer from its own buffer (if it has an intermediate one) to the Android surface (if it is different). May be the regional highlight-revert was lost and not pushed to android, and after the resume, only other regional paintings/refreshes were pushed to Android?

The surface gets destroyed when the application is stopped (not when it is paused). That's why the log above has no "surface created/destroyed/changed" message). Anyways, from KOReader pov we just try to get a native buffer, lock the buffer to write and then unlock the buffer. These three steps obviously won't work if we cannot get the buffer on the first time (hence the cannot blit to a window).

In that specific case the log is saying:

  • KOReader is paused when we do a dict lookup.
  • The "pause" callback started on the Java framework, reachs our code via the app_glue (that is logged as threaded_app)
  • KOReader still works on the background (that's why you can see the Background Runner even when the activity is paused. In fact it should still tick on stopped state).
  • The messages saying "Explicit widgetRepaint: table: 0x91a86710 @ ( 18 , 1502.5 ) // not painting 1 covered widget(s)" could be related to this specific glitch.
  • Even when KOReader gains focus again and the activity is resumed the messages are still saying "not painting 1 covered widget(s)".

In any case I think it can be fixed later. AFAICT this is the only issue that I found with external dicts.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

the messages are still saying "not painting 1 covered widget(s)".

That's not related to your issue. It's just some optimisation in the KeyValuePage (and some other full screen widgets) when they are redrawn, to not redraw the widget that are behind it (eg: the FileManager or the Reader showing the book) because it's fully covered and it's useless (before that, each time you scrolled a page in KeyValuePage, crengine would be redrawing the book page - or FileManager would be redrawing the cover thumbnails).
Remove the following (or set it to false), and you won't see that message anymore:

if self.dimen.w == Screen:getWidth() and self.dimen.h == Screen:getHeight() then
self.covers_fullscreen = true -- hint for UIManager:_repaint()
end

The other bit is from:

-- Used to repaint a specific sub-widget that isn't on the _window_stack itself
-- Useful to avoid repainting a complex widget when we just want to invert an icon, for instance.
-- No safety checks on x & y *by design*. I want this to blow up if used wrong.
function UIManager:widgetRepaint(widget, x, y)
if not widget then return end
logger.dbg("Explicit widgetRepaint:", widget.name or widget.id or tostring(widget), "@ (", x, ",", y, ")")
widget:paintTo(Screen.bb, x, y)
end

which is called by this:
function KeyValueItem:onTap()
if self.callback then
if G_reader_settings:isFalse("flash_ui") then
self.callback()
else
self[1].invert = true
UIManager:widgetRepaint(self[1], self[1].dimen.x, self[1].dimen.y)
UIManager:setDirty(nil, function()
return "fast", self[1].dimen
end)
UIManager:tickAfterNext(function()
self.callback()
self[1].invert = false
UIManager:widgetRepaint(self[1], self[1].dimen.x, self[1].dimen.y)
UIManager:setDirty(nil, function()
return "ui", self[1].dimen
end)
end)
end
end
return true
end

So, you should have seen one Explicit widgetRepaint before all the Package enabled checks, and the log above shows the second one after the pause (from tickAfterNext) (with y=1502, which might be ok if you have a super tall window :) otherwise it's strange).

In any case I think it can be fixed later. AFAICT this is the only issue that I found with external dicts.

Fine with me, so go ahead merging :)

Frenzie added a commit to Frenzie/koreader that referenced this pull request Jul 8, 2019

@Frenzie Frenzie referenced this pull request Jul 8, 2019

Frenzie added a commit that referenced this pull request Jul 8, 2019

@Frenzie Frenzie merged commit b385e44 into koreader:master Jul 8, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.