Android: uncaught exception leading to crash

[savilli]

Minetest version

1317cd1

OS / Hardware

Operating system: Android

Summary

After #12632 Android crashes with ERROR[Main]: Uncaught exception in main thread: .

1. The touchscreen code adds rangeview button to the screen -

   minetest/src/gui/touchscreengui.cpp

   Line 546 in 1317cd1

   m_settingsbar.addButton(range_id, L"rangeview", "rangeview_btn.png");

2. For that it resolves rangeselect keybinding -

   minetest/src/gui/touchscreengui.cpp

   Line 109 in 1317cd1

   case range_id:

3. g_settings->get("keymap_" + key).c_str() returns empty string and keyname_to_keycode throws UnknownKeycode exception.
4. The touchscreen code doesn't catch it.

Steps to reproduce

Join a server using Android. Make sure that keymap_rangeselect isn't set in your config.

[sfan5]

The touchscreengui button should be removed too, I guess.

[savilli]

Isn't it too radical? While the removed keybinding can be restored via config, there would be no way to restore the removed button.

[sfan5]

make the button conditionally depend on the keybind being set?

[savilli]

AFAIK you can't even change keybindings on Android

[savilli]

How about

diff --git a/src/defaultsettings.cpp b/src/defaultsettings.cpp
index f96149070..a71410653 100644
--- a/src/defaultsettings.cpp
+++ b/src/defaultsettings.cpp
@@ -87,7 +87,11 @@ void set_default_settings()
        settings->setDefault("keymap_cmd_local", ".");
        settings->setDefault("keymap_minimap", "KEY_KEY_V");
        settings->setDefault("keymap_console", "KEY_F10");
+#ifdef HAVE_TOUCHSCREENGUI // https://github.com/minetest/minetest/issues/12792
+       settings->setDefault("keymap_rangeselect", "KEY_KEY_R");
+#else
        settings->setDefault("keymap_rangeselect", "");
+#endif
        settings->setDefault("keymap_freemove", "KEY_KEY_K");
        settings->setDefault("keymap_pitchmove", "KEY_KEY_P");
        settings->setDefault("keymap_fastmove", "KEY_KEY_J");

[srifqi]

I have commented in #12632 (comment) about removing the range select button from Android (or touch screen GUI).

Patch

diff --git a/src/gui/touchscreengui.cpp b/src/gui/touchscreengui.cpp
index d483c136e..3d7940cc9 100644
--- a/src/gui/touchscreengui.cpp
+++ b/src/gui/touchscreengui.cpp
@@ -106,9 +106,6 @@ static irr::EKEY_CODE id2keycode(touch_gui_button_id id)
                case camera_id:
                        key = "camera_mode";
                        break;
-               case range_id:
-                       key = "rangeselect";
-                       break;
                default:
                        break;
        }
@@ -543,7 +540,6 @@ void TouchScreenGUI::init(ISimpleTextureSource *tsrc)
        m_settingsbar.addButton(fast_id,    L"fast",      "fast_btn.png");
        m_settingsbar.addButton(debug_id,   L"debug",     "debug_btn.png");
        m_settingsbar.addButton(camera_id,  L"camera",    "camera_btn.png");
-       m_settingsbar.addButton(range_id,   L"rangeview", "rangeview_btn.png");
        m_settingsbar.addButton(minimap_id, L"minimap",   "minimap_btn.png");

        // Chat is shown by default, so chat_hide_btn.png is shown first.
diff --git a/src/gui/touchscreengui.h b/src/gui/touchscreengui.h
index 6b36c0d59..4193048e7 100644
--- a/src/gui/touchscreengui.h
+++ b/src/gui/touchscreengui.h
@@ -48,7 +48,6 @@ typedef enum
        fast_id,
        debug_id,
        camera_id,
-       range_id,
        minimap_id,
        toggle_chat_id,
        chat_id,
diff --git a/textures/base/pack/rangeview_btn.png b/textures/base/pack/rangeview_btn.png
deleted file mode 100644
index e49a1b636..000000000
Binary files a/textures/base/pack/rangeview_btn.png and /dev/null differ

[savilli]

@rubenwardy do you have an opinion?

[srifqi]

I was going to create a PR that makes it detect key mapping before creating a button, but then realised that we cannot change the keymap_* settings on Android (or touch-screen interface). I was going to make the button disappears if its key is not mapped.

Patch

diff --git a/src/client/keycode.cpp b/src/client/keycode.cpp
index 85851cc9c..09f68b234 100644
--- a/src/client/keycode.cpp
+++ b/src/client/keycode.cpp
@@ -18,7 +18,6 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 */
 
 #include "keycode.h"
-#include "exceptions.h"
 #include "settings.h"
 #include "log.h"
 #include "debug.h"
@@ -26,13 +25,6 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "util/string.h"
 #include "util/basic_macros.h"
 
-class UnknownKeycode : public BaseException
-{
-public:
-	UnknownKeycode(const char *s) :
-		BaseException(s) {};
-};
-
 struct table_key {
 	const char *Name;
 	irr::EKEY_CODE Key;
diff --git a/src/client/keycode.h b/src/client/keycode.h
index 7036705d1..6b1c10c86 100644
--- a/src/client/keycode.h
+++ b/src/client/keycode.h
@@ -19,6 +19,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #pragma once
 
+#include "exceptions.h"
 #include "irrlichttypes.h"
 #include "Keycodes.h"
 #include <IEventReceiver.h>
@@ -58,6 +59,13 @@ class KeyPress
 extern const KeyPress EscapeKey;
 extern const KeyPress CancelKey;
 
+class UnknownKeycode : public BaseException
+{
+public:
+	UnknownKeycode(const char *s) :
+			BaseException(s) {};
+};
+
 // Key configuration getter
 KeyPress getKeySetting(const char *settingname);
 
diff --git a/src/gui/touchscreengui.cpp b/src/gui/touchscreengui.cpp
index 70f1c5163..14c8665fb 100644
--- a/src/gui/touchscreengui.cpp
+++ b/src/gui/touchscreengui.cpp
@@ -197,6 +197,16 @@ void AutoHideButtonBar::addButton(touch_gui_button_id button_id,
 				<< std::endl;
 		return;
 	}
+
+	irr::EKEY_CODE button_keycode = irr::EKEY_CODE::KEY_UNKNOWN;
+	try {
+		button_keycode = id2keycode(button_id);
+	} catch (UnknownKeycode &e) {
+		warningstream << "Button " << button_id << " doesn't have a key mapped"
+				<< std::endl;
+		return;
+	}
+
 	int button_size = 0;
 
 	if ((m_dir == AHBB_Dir_Top_Bottom) || (m_dir == AHBB_Dir_Bottom_Top))
@@ -247,7 +257,7 @@ void AutoHideButtonBar::addButton(touch_gui_button_id button_id,
 	btn->guibutton->setVisible(false);
 	btn->guibutton->setEnabled(false);
 	btn->repeatcounter     = -1;
-	btn->keycode           = id2keycode(button_id);
+	btn->keycode           = button_keycode;
 	btn->immediate_release = true;
 	btn->ids.clear();
 
@@ -434,12 +444,21 @@ TouchScreenGUI::TouchScreenGUI(IrrlichtDevice *device, IEventReceiver *receiver)
 void TouchScreenGUI::initButton(touch_gui_button_id id, const rect<s32> &button_rect,
 		const std::wstring &caption, bool immediate_release, float repeat_delay)
 {
+	irr::EKEY_CODE button_keycode = irr::EKEY_CODE::KEY_UNKNOWN;
+	try {
+		button_keycode = id2keycode(id);
+	} catch (UnknownKeycode &e) {
+		warningstream << "Button " << id << " doesn't have a key mapped"
+				<< std::endl;
+		return;
+	}
+
 	button_info *btn       = &m_buttons[id];
 	btn->guibutton         = m_guienv->addButton(button_rect, nullptr, id, caption.c_str());
 	btn->guibutton->grab();
 	btn->repeatcounter     = -1;
 	btn->repeatdelay       = repeat_delay;
-	btn->keycode           = id2keycode(id);
+	btn->keycode           = button_keycode;
 	btn->immediate_release = immediate_release;
 	btn->ids.clear();

 

[srifqi]

How about

I agree with savilli's diff that gives KEY_KEY_R mapping for touch-screen GUI. It is the most trivial fix that I can think:

• no need to remove any button
• no need to check for key mapping

The better fix for this issue is to create direct input handling from touch-screen (like physical joystick) that does not need to use keyboard handler. But, that is way harder and more time-consuming.

[TurkeyMcMac]

IMO the button should just be removed entirely, since it has already been decided that it is not very useful. People who still want it will probably be playing on a PC anyway. I'd also be OK with savilli's proposal.

[AbduSharif]

I'm an Android player and I use the button alot and would rather it doesn't get removed.

And I don't have a PC to play on.

[TurkeyMcMac]

Fair enough.

[okias]

... a year later :)

I'm working on making touchscreen available as a runtime option to avoid having two binaries on Linux (and other systems ofc), where touchscreen may be available (example tablet which also can be docked with mouse and keyboard).

My solution is so far just assign R key all the time, since under "desktop" setup is not assigned either.

I would probably love to have ability to not show range, when R key will get unassigned rather than crash, but maybe we could do it differently?

Thanks!