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

Fix locked hardware buttons on Android #4086

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
10 participants
@MoNTE48
Copy link
Contributor

commented May 3, 2016

Fixed #2122 and #1454
thank @est31

3 years no one wanted to fix this serious bug.
I spent a lot of time to understand what is the reason (I was looking for a bug in keycode.cpp, Irrlicht, JNI, but not in the patch). When I found the cause, I just could not stop laughing.
Why did to disable back button so hardcode methods?

@sofar

This comment has been minimized.

Copy link
Member

commented May 3, 2016

The profanity isn't acceptable in commit messages. Please remove it.

I understand that you're frustrated, I don't mind a swear word here or there, just not in the final git commits, please.

Fix locked hardware buttons on Android
Fix locked hardware buttons on Android
Fixed #2122 and #1454
@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

@sofar, I was not thinking, I'm sorry. Corrected.

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

@MoNTE48 you really shouldn't insult people who spend their freetime to write code that you are hugely profiting from.

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

@PilzAdam, github is not the place to discuss this issue, I believe.
What did sapier - or deliberate error (why?), or complete incompetence.

@paramat paramat added the Android label May 4, 2016

@paramat paramat added this to the 0.4.14 milestone May 4, 2016

}

device->postEventFromUser(event);
+ status = 1;

This comment has been minimized.

Copy link
@est31

est31 May 5, 2016

Contributor

Removing this is wrong as well as keeping it. status must be 1 if postEventFromUser returns true, otherwise it must be false. That's at least what I think is the most proper code here.

This comment has been minimized.

Copy link
@est31

est31 May 5, 2016

Contributor

My explaining theory:

  • As you can read from the definition of the onInputEvent function inside the android_app struct in android_native_app_glue.h (from the NDK), the function onInputEvent should "Return 1 if you have handled the event, 0 for any default dispatching".
  • Irrlicht code sets the onInputEvent function pointer to the address of CIrrDeviceAndroid::handleInput
  • While the docs of CGUIEnvironment::postEventFromUser don't reveal the meaning of the return value, the call of the onEvent method inside the implementation reveals the common practice of returning true if the event was handled, and false if it wasn't handled.

So if you kept the state as is, every key, including volume down and up, won't be processed by the OS anymore. This probably causes #2122 .

If you went with @MoNTE48 's proposed solution, the OS would process all keys. It should only do that if minetest doesn't want to prevent it.

This comment has been minimized.

Copy link
@MoNTE48

MoNTE48 May 5, 2016

Author Contributor

I'm not a programmer. You ought to know what's best. But this raises some problems, the players uncomfortable, and do not welcome Google

This comment has been minimized.

Copy link
@MoNTE48

MoNTE48 May 5, 2016

Author Contributor

@est31, https://github.com/zaki/irrlicht/blob/96cf9444667431279e25d7a1c8c0812748e1fbb3/source/Irrlicht/Android/CIrrDeviceAndroid.cpp#L489
Just look at this. Also, Minetest use ONLY ONE hardware button - BACK, instead of Esc. Should we worry about this issue?

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja May 10, 2016

Member

Actually, Minetest handles its regular keys (w, s, a, d, t, i, etc) too. You need an external keyboard, or something to force-open the soft-keyboard, but you can use the other keys.

This comment has been minimized.

Copy link
@est31

est31 May 12, 2016

Contributor

@MoNTE48 can you please change it to status = device->postEventFromUser(event) ? 1 : 0;? So no device->postEventFromUser(event); and also no status = 1 but the combined version like above.

I'll 👍 this part of the change then.

This comment has been minimized.

Copy link
@MoNTE48

MoNTE48 May 13, 2016

Author Contributor

@est31 Okay. I am back. I checked your code and it works well. I'm going to make changes in my pr after some testing.

@@ -19,6 +19,10 @@ public void onCreate(Bundle savedInstanceState) {
public void onDestroy() {
super.onDestroy();
}

@Override
public void onBackPressed() {

This comment has been minimized.

Copy link
@est31

est31 May 5, 2016

Contributor

What should this achieve?

This comment has been minimized.

Copy link
@MoNTE48

MoNTE48 May 5, 2016

Author Contributor

Standard Android closes the application by pressing the back button if this is the first activity.

This comment has been minimized.

Copy link
@est31

est31 May 13, 2016

Contributor

And why is this bad?

This comment has been minimized.

Copy link
@MoNTE48

MoNTE48 May 13, 2016

Author Contributor

Fuck. I press the Back button and the game stops. It is very good? :)

This comment has been minimized.

Copy link
@est31

est31 May 13, 2016

Contributor

When does it happen? Do I have to be in a special menu? For me it shows the esc menu when inside the game when I press the back button, is it different for you?

This comment has been minimized.

Copy link
@est31

est31 May 13, 2016

Contributor

@MoNTE48 can you test #4119?

This comment has been minimized.

Copy link
@MoNTE48

MoNTE48 May 13, 2016

Author Contributor

Lol, no!!!

This comment has been minimized.

Copy link
@est31

est31 May 13, 2016

Contributor

??

You want the bug to be fixed, its very important to you, no?

I have tried really hard to improve your PR and give you suggestions but you dont edit it. And now I do it myself (and creating that patch file wasn't as easy as it looks), and you say no? Do you want it to be fixed or not? I credit you in the commit description, if its that what upsets you.

This comment has been minimized.

Copy link
@MoNTE48

MoNTE48 May 13, 2016

Author Contributor

I could do everything myself, but I'll be home in the evening. I'm in the hands of only the iphone and I'm in another city

This comment has been minimized.

Copy link
@est31

est31 May 13, 2016

Contributor

Evening, that's okay. I'll close my PR if you adjust this one.

@est31 est31 added the Bug label May 5, 2016

@est31

This comment has been minimized.

Copy link
Contributor

commented May 5, 2016

And please excuse to @sapier . He did a big load of work with the android port so a bug might have sneaked in. And as the commit message of 1cc40c0 (which adds that particular patch) notes, he might not even have been the original author of the patch.

The insult is rude towards all the work sapier did and may even be addressed to the wrong person.

@paramat

This comment has been minimized.

Copy link
Member

commented May 5, 2016

Removed disrespect from first post.

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2016

@paramat, why I do not like github - so easy to fake someone else's message, cancel the commit or change the author. It is a pain

@est31

This comment has been minimized.

Copy link
Contributor

commented May 6, 2016

Well at least github is transparent about it and shows to people that paramat edited the post.

@paramat paramat added Bugfix and removed Bug labels May 7, 2016

@Wayward1

This comment has been minimized.

Copy link
Contributor

commented May 9, 2016

Edit: I can confirm this fixes both issues

@paramat

This comment has been minimized.

Copy link
Member

commented May 9, 2016

@est31 thoughts on this? I don't consider it absolutely essential but would be nice if it's acceptable and not too risky.

@est31

This comment has been minimized.

Copy link
Contributor

commented May 9, 2016

@paramat yeah its mostly harmless but it would be better if tested.

@paramat

This comment has been minimized.

Copy link
Member

commented May 9, 2016

Wayward1 reports volume key not fixed. We probably won't let this delay release.

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2016

@paramat what?
@Wayward1 it is a mistake on your part, since it completely fixes the problem, look MultiCraft.

@paramat

This comment has been minimized.

Copy link
Member

commented May 10, 2016

Wayward1 reports this PR does fix volume key. From IRC:
"Wayward_One > paramat, on a hunch, I tried a direct clone of MoNTE48's androidbuttons branch instead of a patch, and the volume buttons indeed work now"

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

It's good ;)

@paramat

This comment has been minimized.

Copy link
Member

commented May 10, 2016

DonBatman might give this another test tomorrow.

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented May 10, 2016

I'd like to see est31's comment addressed. Other than that this looks O.K.

@paramat

This comment has been minimized.

Copy link
Member

commented May 11, 2016

From IRC minetest-project channel:
Wayward_One > paramat, i saw your earlier message, and after about an hour of testing single and multiplayer it seems fine :)

@paramat paramat removed this from the 0.4.14 milestone May 11, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented May 11, 2016

Still not enough solid approval and issues remain, so removed from milestone.

@paramat paramat removed the One approval label May 11, 2016

@Wayward1

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

@DonBatman, sure. Check your forum inbox

@DonBatman

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

Seems to work good for me.

@paramat

This comment has been minimized.

Copy link
Member

commented May 12, 2016

See #4111

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2016

@paramat what is it?

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

@MoNTE48 I'm not sure why paramat added that... they are not related

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2016

@paramat @Zeno- please, back milestone. This is simple and good work bugfix...

@Wayward1

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

I agree, especially since the release has been pushed back

@paramat paramat added this to the 0.4.14 milestone May 12, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented May 12, 2016

Re-added to milestone, for consideration.

I linked to #4111 in case that was related to DonBatman's problems.

From IRC minetest-project channel 10-5-2016:

paramat> Wayward_One does android run ok with #4086 applied? no new bugs caused? any testing is welcome
Wayward_One> paramat, i saw your earlier message, and after about an hour of testing single and multiplayer it seems fine :)

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2016

@paramat, otherwise it can not be. Each change in MultiCraft passes very thoroughly tested before publication

@paramat

This comment has been minimized.

Copy link
Member

commented May 12, 2016

@MoNTE48 we're discussing on dev channel now and this will be considered.

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2016

@est31 Are you happy? :)

}

- device->postEventFromUser(event);
+ status = device->postEventFromUser(event) ? 1 : 0;

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 14, 2016

Contributor

What is this for?

Looking at the source code it seems to be to let the calling function know whether the event was absorbed or not. I think it's correct (your change) but I wonder why it wasn't always like this

Edit: Oh I see. est31 wanted it

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented May 14, 2016

Actually, I've just looked at the irrlicht source code again. These changes seem consistent.

+1

@est31

This comment has been minimized.

Copy link
Contributor

commented May 14, 2016

@MoNTE48 I am not sure, when does back close the application for you?

Does back close the application if you are ingame? Or only in the main menu?

For me, back does not close the application.

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2016

@est31 I have already explained this in IRC and wrote about it above. This is elementary and it knows every Android dev, I will not repeat it again.

@est31

This comment has been minimized.

Copy link
Contributor

commented May 14, 2016

comment removed

@MoNTE48

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2016

@est31 #4086 (comment)
You said such nonsense. You could just use Google to get an answer to your question. At the same time you are lazy enough to use Google and quite strange to write a number of the text.
[This part removed by Zeno-]

@MoNTE48 MoNTE48 closed this May 14, 2016

@est31

This comment has been minimized.

Copy link
Contributor

commented May 14, 2016

I was mistaken, and now think, while the onBackPressed is not perfect it fixes the bug and the PR should be merged before the release.

@est31 est31 reopened this May 14, 2016

@est31

This comment has been minimized.

Copy link
Contributor

commented May 14, 2016

+1 myself too.

@est31 est31 added >= Two approvals and removed One approval labels May 14, 2016

@rubenwardy

This comment has been minimized.

Copy link
Member

commented May 14, 2016

@rubenwardy rubenwardy closed this May 14, 2016

@MoNTE48 MoNTE48 deleted the MoNTE48:androidbuttons branch May 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.