[w.i.p.] isWifiEnabled and isFullscreen use JNI:callBooleanMethod #73

mwoz123 · 2017-10-23T14:16:24Z

in response to koreader/koreader#3396 (review)

Commit from phone.
needs testing

merge upstream

mwoz123 · 2017-10-23T19:12:26Z

That code change didn't help:

10-23 21:07:19.608 4368 4386 E luajit-launcher: Failed to run script: [string "android.lua"]:1050: 'struct JNINativeInterface' has no member named 'callBooleanMethod'
10-23 21:07:19.625 4368 4386 V threaded_app: android_app_destroy!

"android.lua" line 1050:

return self.env[0].callBooleanMethod(self.env, object, methodID, ...)

Is that the reason why nobody used callBooleanMethod before? :(

Does any one know how what's wrong here?

Frenzie · 2017-10-23T19:27:11Z

assets/android.lua

@@ -1360,7 +1360,7 @@ local function run(android_app_state)
    end
    android.isFullscreen = function()
        return JNI:context(android.app.activity.vm, function(JNI)
-            local fullscreen = JNI:callIntMethod(
+            local fullscreen = JNI:callBooleanMethod(
                android.app.activity.clazz,
                "isFullscreen",
                "()Z"


Shouldn't this be something like B then?

You mean "()B" instead "()Z" ?
It's completely unreadable for me where it came from and what it does mean :(

If it caught my change the result is the same:
10-23 21:31:42.706 4577 4593 E luajit-launcher: Failed to run script: [string "android.lua"]:1050: 'struct JNINativeInterface' has no member named 'callBooleanMethod'
10-23 21:31:42.712 4577 4593 V threaded_app: android_app_destroy!

I must have overlooked doc for that. If there's such?

Pardon, B is byte and Z is boolean.

Just "Z" or "()Z" or "Z()"?

Oh, please link directly to the line. It's clearer. :-) You should change it to a capital C.

On line 1050.

Just "Z" or "()Z" or "Z()"?

No it's good.

Sorry for my earlier brevity; I was on my phone in a slight hurry but I wanted to give you something useful to work with in case you were still looking at it.

Anyway, it should be fine if you fix it to an uppercase C on the line you mentioned.

android-luajit-launcher/assets/android.lua

Line 1050 in bd88561

return self.env[0].callBooleanMethod(self.env, object, methodID, ...)

Frenzie · 2017-10-24T16:52:27Z

I apologize if I was confusing, but it's this line:

android-luajit-launcher/assets/android.lua

Line 1050 in bd88561

return self.env[0].callBooleanMethod(self.env, object, methodID, ...)

That has to be changed to
return self.env[0].CallBooleanMethod(self.env, object, methodID, ...)

The other line is the line I mentioned, not the one you mentioned. ;-)

mwoz123 · 2017-10-27T18:13:27Z

I did a quick test and something still seems not ok:

I/KOReader( 3016): is WifiEnabled =
I/KOReader( 3016): DEBUG input event => type: 3, code: 47(S), value: 0, time: 1509127951.788931

at least on 686 emulator (android 5.1)

It doesn't throw any expection, but return is missing.

There's a workaround to use int as
android.isCharging = function() and then in lua compare if result ==1
but it would be nice to also callBooleanMethod working

@Frenzie is it time to try workaround? Or do you have any ideas what else might be wrong?

mwoz123 · 2017-10-27T18:21:24Z

same result for isFullscreen:

I/KOReader( 3137): is fullscreen =
I/KOReader( 3137): DEBUG screen_fullscreen_menu_table.lua: Fullscreen swiching to: false

Frenzie · 2017-10-27T19:37:27Z

I suspect you'll need to cast this jboolean object into a C bool.

Presumably it'll give you these values

android-luajit-launcher/assets/android.lua

Lines 772 to 773 in bd88561

    
           static const int JNI_FALSE = 0; 
        
           static const int JNI_TRUE = 1;

There's a to_string.

android-luajit-launcher/assets/android.lua

Lines 1099 to 1104 in bd88561

    
           function JNI:to_string(javastring) 
        
               local utf = self.env[0].GetStringUTFChars(self.env, javastring, nil) 
        
               local luastr = ffi.string(utf, self.env[0].GetStringUTFLength(self.env, javastring)) 
        
               self.env[0].ReleaseStringUTFChars(self.env, javastring, utf) 
        
               return luastr 
        
           end

We'd probably have to add to_bool.

So you could fix it up properly if you want to. (I don't particularly feel like looking at that for the moment.)

But I think it's fine if you just keep the typo correction on CallBooleanMethod for the moment and use the int workaround for now. It doesn't look too horrible. :-)

mwoz123 · 2017-10-27T20:34:40Z

Changed to int + follow up changes in MainActivity.java but unfortunately on my x86 emulator result was exactly the same.

This can mean there's an issue in Java implementation, but it was just done excalty as on some recipes on internet (or issue with emulator?). Maybe it's again Java code has to be wrapped with something?

Any way if it's java issue then we would be able to uncommit ab96312

Frenzie · 2017-10-27T20:51:26Z

Oh, I thought you meant you'd already tested the int method. For wifi I would only trust results from a real device, although I would probably expect it to always return false in the emulator.

I don't have a working Android knowledge or anything. I only know various things that go into Android (such as XML and Java). But from browsing around the docs real quick it could be that you should use different methods for Android <4.4 and Android 4.4+.

API 16

https://developer.android.com/training/system-ui/immersive.html
https://developer.android.com/reference/android/view/View.html#SYSTEM_UI_FLAG_FULLSCREEN

API 1

https://developer.android.com/reference/android/view/WindowManager.LayoutParams.html#FLAG_FULLSCREEN

Basically like this (should've checked our own code before checking the docs) except get instead of set

android-luajit-launcher/src/org/koreader/launcher/MainActivity.java

Lines 45 to 61 in ab96312

    
           private void setFullscreenLayout() { 
        
               int SDK_INT = android.os.Build.VERSION.SDK_INT; 
        
               if(SDK_INT >= 11 && SDK_INT < 16) { 
        
                   getWindow().getDecorView().setSystemUiVisibility(View.STATUS_BAR_HIDDEN); 
        
               } else if (SDK_INT >= 16 && SDK_INT < 19) { 
        
                   getWindow().getDecorView().setSystemUiVisibility( 
        
                           View.SYSTEM_UI_FLAG_FULLSCREEN | View.SYSTEM_UI_FLAG_LOW_PROFILE); 
        
               } else if (SDK_INT >= 19) { 
        
                   getWindow().getDecorView().setSystemUiVisibility( 
        
                           View.SYSTEM_UI_FLAG_LAYOUT_STABLE | 
        
                           View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION | 
        
                           View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN | 
        
                           View.SYSTEM_UI_FLAG_HIDE_NAVIGATION | 
        
                           View.SYSTEM_UI_FLAG_FULLSCREEN | 
        
                           View.SYSTEM_UI_FLAG_IMMERSIVE_STICKY); 
        
               } 
        
           }

mwoz123 · 2017-10-28T05:47:46Z

I checked it using 5.1 emulator which is api 22 so should work.

I checked setFullscreen and setWifiEnabled, it was working fine. Must have overlooked the isWifiEnabled and isFulscreen. Sorry.

BTW as far as I remember the setFullscreenLayout was after my full screen commit.

It seems to be the same issue. In android.lua or MainActivity.java.
From Java perspective it seems ok unless it again needs some extra magic around that code but it's now constructed exactly like isCharging so should work.(unless isCharging is not working correctly, but is at least returns a value in logs).

At least I can see now no difference between isCharging is Fullscreen and isWiFienabled...

Frenzie · 2017-10-28T07:31:30Z

I checked it using 5.1 emulator which is api 22 so should work.

Okay, but we want to keep things working on API 9 and up unless there's a really good reason not to. Doesn't your own Android reader come with Android API <16? ;-)

BTW as far as I remember the setFullscreenLayout was after my full screen commit.

It was structured differently but it was there from the first time the Java layer was added in cb6deb8 (with a slight mistake fixed in 655bf22).

Frenzie · 2017-10-28T07:32:03Z

src/org/koreader/launcher/MainActivity.java

-    public boolean isFullscreen() {
-    	return (getWindow().getAttributes().flags & WindowManager.LayoutParams.FLAG_FULLSCREEN) != 0;
+    public int isFullscreen() {
+    	return ((getWindow().getAttributes().flags & WindowManager.LayoutParams.FLAG_FULLSCREEN) != 0) ? 1: 0;


Should this be &&?

& is a Java low level (bit) comparison as far as I remember from couple years back, Java trainings. It's almost not used in plain Java. Never had a chance to see it in real code.
But did seen it in android.

Also if this was the issue it should return at least false.

I'll double check if it's not the case..

I see, it's bitwise AND to extract the flag. Makes sense, although it seems more like a C kind of thing than something you'd expect to see in Java.

mwoz123 · 2017-10-28T09:22:20Z

This was a problem with x86 emulator.

Works on ARM. Please merge. I'll push new PR for API 11 Fullscreen

Frenzie · 2017-10-28T09:24:58Z

It's fine if it doesn't work but does it also not crash? :-P

mwoz123 · 2017-10-28T09:35:22Z

emulator x86 = doesn't crash, doesn't work :(

arm = doesn't crash, works;)

Frenzie · 2017-10-28T09:36:56Z

I meant API-wise, sorry for the confusion.

Edit: well, I suppose I meant both arch and API. I'll look into setting up some Android unit tests to prevent regressions (we currently only test if it builds) but I'm not entirely sure how yet.

Part of the problem is that it'll take 20 minutes or more, so they can't quite be made part of the regular CI. Maybe something involving a Gitlab mirror…

* Also fix callBoolean method typo

mwoz123 added 2 commits October 23, 2017 14:20

Merge pull request #1 from koreader/master

415049e

merge upstream

isWifiEnabled and isFulscreen use JNI:callBooleanMethod

00b0cc6

mwoz123 mentioned this pull request Oct 23, 2017

[w.i.p.] wifi status in footer koreader/koreader#3396

Merged

Frenzie reviewed Oct 23, 2017

View reviewed changes

Code review change

58a3827

Undo misunderstanding, fix callBoolean method

e18e344

replace JNI:callBooleanMethod with JNI:callIntMethod for boolean

ab96312

Frenzie reviewed Oct 28, 2017

View reviewed changes

Frenzie merged commit 5541846 into koreader:master Oct 28, 2017

mwoz123 added a commit to mwoz123/android-luajit-launcher that referenced this pull request Nov 4, 2017

[fix] isWifiEnabled and isFullscreen (koreader#73) (#3)

04e553f

* Also fix callBoolean method typo

mwoz123 deleted the mwoz123-use-call-boolean-method branch November 4, 2017 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[w.i.p.] isWifiEnabled and isFullscreen use JNI:callBooleanMethod #73

[w.i.p.] isWifiEnabled and isFullscreen use JNI:callBooleanMethod #73

mwoz123 commented Oct 23, 2017

mwoz123 commented Oct 23, 2017

Frenzie Oct 23, 2017

mwoz123 Oct 23, 2017

mwoz123 Oct 23, 2017

Frenzie Oct 23, 2017

mwoz123 Oct 23, 2017

Frenzie Oct 23, 2017

Frenzie Oct 23, 2017

Frenzie Oct 23, 2017

Frenzie Oct 23, 2017

Frenzie commented Oct 24, 2017

mwoz123 commented Oct 27, 2017 •

edited

Loading

mwoz123 commented Oct 27, 2017

Frenzie commented Oct 27, 2017

mwoz123 commented Oct 27, 2017

Frenzie commented Oct 27, 2017 •

edited

Loading

mwoz123 commented Oct 28, 2017

Frenzie commented Oct 28, 2017

Frenzie Oct 28, 2017

mwoz123 Oct 28, 2017

Frenzie Oct 28, 2017

mwoz123 commented Oct 28, 2017

Frenzie commented Oct 28, 2017

mwoz123 commented Oct 28, 2017

Frenzie commented Oct 28, 2017 •

edited

Loading

[w.i.p.] isWifiEnabled and isFullscreen use JNI:callBooleanMethod #73

[w.i.p.] isWifiEnabled and isFullscreen use JNI:callBooleanMethod #73

Conversation

mwoz123 commented Oct 23, 2017

mwoz123 commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie commented Oct 24, 2017

mwoz123 commented Oct 27, 2017 • edited Loading

mwoz123 commented Oct 27, 2017

Frenzie commented Oct 27, 2017

mwoz123 commented Oct 27, 2017

Frenzie commented Oct 27, 2017 • edited Loading

API 16

API 1

mwoz123 commented Oct 28, 2017

Frenzie commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwoz123 commented Oct 28, 2017

Frenzie commented Oct 28, 2017

mwoz123 commented Oct 28, 2017

Frenzie commented Oct 28, 2017 • edited Loading

mwoz123 commented Oct 27, 2017 •

edited

Loading

Frenzie commented Oct 27, 2017 •

edited

Loading

Frenzie commented Oct 28, 2017 •

edited

Loading