Improve interactive mouse mapping

8 files changed, 116 insertions, 62 deletions

diff --git a/include/mouse.h b/include/mouse.h
index 11df6328c..5b48abf07 100644
--- a/include/mouse.h
+++ b/include/mouse.h
@@ -142,7 +142,7 @@ class MouseInterfaceInfoEntry final {
 public:
 	bool IsEmulated() const;
 	bool IsMapped() const;
-	bool IsMapped(const uint8_t device_idx) const;
+	bool IsMapped(const uint8_t physical_device_idx) const;
 	bool IsMappedDeviceDisconnected() const;
 
 	MouseInterfaceId GetInterfaceId() const;
@@ -196,11 +196,14 @@ public:
 	static bool CheckInterfaces(const ListIDs &list_ids);
 	static bool PatternToRegex(const std::string &pattern, std::regex &regex);
 
-	bool ProbeForMapping(uint8_t &device_id); // For interactive mapping in
-	                                          // MOUSECTL.COM only!
+	// This one is ONLY for interactive mapping in MOUSECTL.COM!
+	bool MapInteractively(const MouseInterfaceId interface_id,
+	                      uint8_t &physical_device_idx);
 
-	bool Map(const MouseInterfaceId interface_id, const uint8_t device_idx);
-	bool Map(const MouseInterfaceId interface_id, const std::regex &regex);
+	bool Map(const MouseInterfaceId interface_id,
+	         const uint8_t physical_device_idx);
+	bool Map(const MouseInterfaceId interface_id,
+	         const std::regex &regex);
 	bool UnMap(const ListIDs &list_ids);
 
 	bool OnOff(const ListIDs &list_ids, const bool enable);
@@ -228,6 +231,8 @@ public:
 private:
 	MouseControlAPI(const MouseControlAPI &)            = delete;
 	MouseControlAPI &operator=(const MouseControlAPI &) = delete;
+
+	bool was_interactive_mapping_started = false;
 };
 
 #endif // DOSBOX_MOUSE_H
diff --git a/src/dos/program_mousectl.cpp b/src/dos/program_mousectl.cpp
index efe47c87e..92759cc2c 100644
--- a/src/dos/program_mousectl.cpp
+++ b/src/dos/program_mousectl.cpp
@@ -377,8 +377,8 @@ bool MOUSECTL::CmdMap()
 		WriteOut(convert_ansi_markup("[color=cyan]%-4s[reset]   ?").c_str(),
 		         MouseControlAPI::GetInterfaceNameStr(interface_id).c_str());
 
-		uint8_t device_id = 0;
-		if (!mouse_config_api.ProbeForMapping(device_id)) {
+		uint8_t device_idx = 0;
+		if (!mouse_config_api.MapInteractively(interface_id, device_idx)) {
 			mouse_config_api.UnMap(empty);
 			WriteOut("\b");
 			WriteOut(MSG_Get("PROGRAM_MOUSECTL_MAP_CANCEL"));
@@ -387,9 +387,8 @@ bool MOUSECTL::CmdMap()
 		}
 
 		WriteOut("\b");
-		WriteOut(info_physical[device_id].GetDeviceName().c_str());
+		WriteOut(info_physical[device_idx].GetDeviceName().c_str());
 		WriteOut("\n");
-		mouse_config_api.Map(interface_id, device_id);
 	}
 
 	FinalizeMapping();
@@ -534,7 +533,7 @@ void MOUSECTL::AddMessages()
 	        "  -reset         restores all mouse settings from the configuration file\n"
 	        "\n"
 	        "Notes:\n"
-	        "  - if sensitivity or rate is omitted, it is reset to default value\n"
+	        "  If sensitivity or rate is omitted, it is reset to default value.\n"
 	        "\n"
 	        "Examples:\n"
 	        "  [color=green]mousectl[reset] [color=white]DOS[reset] [color=white]COM1[reset] -map    ; asks user to select mice for a two player game");
@@ -586,6 +585,6 @@ void MOUSECTL::AddMessages()
 	        "mouse interfaces.");
 	MSG_Add("PROGRAM_MOUSECTL_MAP_CANCEL", "(mapping cancelled)");
 	MSG_Add("PROGRAM_MOUSECTL_MAP_HINT",
-	        "Mouse captured. Seamless mouse integration is always disabled while mapping is\n"
-	        "in effect and mapped mice always receive raw input events.");
+	        "Seamless mouse integration is always disabled while mapping is in effect\n"
+	        "and mapped mice always receive raw input events.");
 }
diff --git a/src/hardware/mouse/mouse.cpp b/src/hardware/mouse/mouse.cpp
index ee249f2ba..0b270439d 100644
--- a/src/hardware/mouse/mouse.cpp
+++ b/src/hardware/mouse/mouse.cpp
@@ -51,13 +51,14 @@ static struct {
 	uint32_t clip_x    = 0;     // clipping = size of black border (one side)
 	uint32_t clip_y    = 0;
 
-	uint32_t cursor_x_abs  = 0;    // absolute position from start of drawing area
+	uint32_t cursor_x_abs  = 0;     // absolute position from start of drawing area
 	uint32_t cursor_y_abs  = 0;
-	bool cursor_is_outside = true; // if mouse cursor is outside of drawing area
+	bool cursor_is_outside = false; // if mouse cursor is outside of drawing area
 
-	bool has_focus             = false; // if our window has focus
-	bool gui_has_taken_over    = false; // if a GUI requested to take over the mouse
-	bool capture_was_requested = false; // if user requested mouse to be captured
+	bool has_focus              = false; // if our window has focus
+	bool gui_has_taken_over     = false; // if a GUI requested to take over the mouse
+	bool is_mapping_in_progress = false; // if interactive mapping is running
+	bool capture_was_requested  = false; // if user requested mouse to be captured
 
 	bool is_captured  = false; // if GFX was requested to capture mouse
 	bool is_visible   = false; // if GFX was requested to make cursor visible
@@ -171,7 +172,8 @@ static void update_state() // updates whole 'state' structure, except cursor vis
 
 	// Due to ManyMouse API limitation, we are unable to support seamless
 	// integration if mapping is in effect
-	const bool is_mapping = manymouse.IsMappingInEffect();
+	const bool is_mapping = state.is_mapping_in_progress ||
+	                        manymouse.IsMappingInEffect();
 	if (state.is_seamless && is_mapping) {
 		state.is_seamless = false;
 		static bool already_warned = false;
@@ -264,6 +266,12 @@ static void update_state() // updates whole 'state' structure, except cursor vis
 	                                 state.is_captured &&
 	                                 mouse_config.middle_release;
 
+	// Note: it would make sense to block capture/release on any mouse click
+	// while 'state.is_mapping_in_progress' - unfortunately this would lead
+	// to a race condition between events from SDL and ManyMouse at the end
+	// of mapping process, leading to ununiform (random) user experience.
+	// TODO: if SDL gets expanded to include ManyMouse, change the behavior!
+
 	// Select hint to be displayed on a title bar
 	if (state.is_fullscreen || state.gui_has_taken_over || !state.has_focus)
 		state.hint_id = MouseHint::None;
@@ -318,6 +326,14 @@ bool MOUSE_IsCaptured()
 	return state.is_captured;
 }
 
+bool MOUSE_IsProbeForMappingAllowed()
+{
+	// Conditions to be met to accept mouse clicks for interactive mapping:
+	// - we have a focus
+	// - no GUI has taken over the mouse
+	return state.has_focus && !state.gui_has_taken_over;
+}
+
 // ***************************************************************************
 // Interrupt 74 implementation
 // ***************************************************************************
@@ -660,6 +676,8 @@ MouseControlAPI::MouseControlAPI()
 MouseControlAPI::~MouseControlAPI()
 {
 	manymouse.StopConfigAPI();
+	if (was_interactive_mapping_started)
+		state.is_mapping_in_progress = false;
 	MOUSE_UpdateGFX();
 }
 
@@ -717,16 +735,36 @@ bool MouseControlAPI::PatternToRegex(const std::string &pattern, std::regex &reg
 	return true;
 }
 
-bool MouseControlAPI::ProbeForMapping(uint8_t &device_id)
+bool MouseControlAPI::MapInteractively(const MouseInterfaceId interface_id,
+                                       uint8_t &physical_device_idx)
 {
 	if (IsNoMouseMode())
 		return false;
 
+	if (!was_interactive_mapping_started) {
+		// Interactive mapping was started
+		assert(!state.is_mapping_in_progress);
+		// Capture the mouse, otherwise it might be confusing
+		// for the user when it gets captured after he clicks
+		// simply to select the mouse
+		state.capture_was_requested     = true;
+		// Tell the other code that mapping is in progress,
+		// so that it can disable seamless mouse integration,
+		// and possibly apply other changes to mouse behavior
+		state.is_mapping_in_progress    = true;
+		was_interactive_mapping_started = true;
+		MOUSE_UpdateGFX();
+	}
+
 	manymouse.RescanIfSafe();
-	return manymouse.ProbeForMapping(device_id);
+	if (!manymouse.ProbeForMapping(physical_device_idx))
+		return false;
+
+	return Map(interface_id, physical_device_idx);
 }
 
-bool MouseControlAPI::Map(const MouseInterfaceId interface_id, const uint8_t device_idx)
+bool MouseControlAPI::Map(const MouseInterfaceId interface_id,\
+                          const uint8_t physical_device_idx)
 {
 	if (IsNoMouseMode())
 		return false;
@@ -735,7 +773,7 @@ bool MouseControlAPI::Map(const MouseInterfaceId interface_id, const uint8_t dev
 	if (!mouse_interface)
 		return false;
 
-	return mouse_interface->ConfigMap(device_idx);
+	return mouse_interface->ConfigMap(physical_device_idx);
 }
 
 bool MouseControlAPI::Map(const MouseInterfaceId interface_id, const std::regex &regex)
diff --git a/src/hardware/mouse/mouse_config.cpp b/src/hardware/mouse/mouse_config.cpp
index 6474e54f0..e77ca6b80 100644
--- a/src/hardware/mouse/mouse_config.cpp
+++ b/src/hardware/mouse/mouse_config.cpp
@@ -340,9 +340,12 @@ static void config_init(Section_prop &secprop)
 	prop_str->Set_help(
 	        "COM (serial) port default mouse model:\n"
 	        "   2button:        2 buttons, Microsoft mouse.\n"
-	        "   3button:        3 buttons, Logitech mouse, mostly compatible with Microsoft mouse.\n"
-	        "   wheel:          3 buttons + wheel, mostly compatible with Microsoft mouse.\n"
-	        "   msm:            3 buttons, Mouse Systems mouse, NOT COMPATIBLE with Microsoft mouse.\n"
+	        "   3button:        3 buttons, Logitech mouse,\n"
+	        "                   mostly compatible with Microsoft mouse.\n"
+	        "   wheel:          3 buttons + wheel,\n"
+	        "                   mostly compatible with Microsoft mouse.\n"
+	        "   msm:            3 buttons, Mouse Systems mouse,\n"
+	        "                   NOT COMPATIBLE with Microsoft mouse.\n"
 	        "   2button+msm:    Automatic choice between 2button and msm.\n"
 	        "   3button+msm:    Automatic choice between 3button and msm.\n"
 	        "   wheel+msm:      Automatic choice between wheel and msm.\n"
diff --git a/src/hardware/mouse/mouse_interfaces.cpp b/src/hardware/mouse/mouse_interfaces.cpp
index 4015256c2..4c5a6b257 100644
--- a/src/hardware/mouse/mouse_interfaces.cpp
+++ b/src/hardware/mouse/mouse_interfaces.cpp
@@ -46,8 +46,8 @@ const MouseInterface &MouseInterfaceInfoEntry::Interface() const
 
 const MousePhysical &MouseInterfaceInfoEntry::MappedPhysical() const
 {
-	const auto mapped_idx = Interface().GetMappedDeviceIdx();
-	return ManyMouseGlue::GetInstance().physical_devices[mapped_idx];
+	const auto mapped_physical_idx = Interface().GetMappedDeviceIdx();
+	return ManyMouseGlue::GetInstance().physical_devices[mapped_physical_idx];
 }
 
 bool MouseInterfaceInfoEntry::IsEmulated() const
@@ -60,9 +60,9 @@ bool MouseInterfaceInfoEntry::IsMapped() const
 	return Interface().IsMapped();
 }
 
-bool MouseInterfaceInfoEntry::IsMapped(const uint8_t device_idx) const
+bool MouseInterfaceInfoEntry::IsMapped(const uint8_t physical_device_idx) const
 {
-	return Interface().IsMapped(device_idx);
+	return Interface().IsMapped(physical_device_idx);
 }
 
 bool MouseInterfaceInfoEntry::IsMappedDeviceDisconnected() const
@@ -326,12 +326,12 @@ uint8_t MouseInterface::GetInterfaceIdx() const
 
 bool MouseInterface::IsMapped() const
 {
-	return mapped_idx < mouse_info.physical.size();
+	return mapped_physical_idx < mouse_info.physical.size();
 }
 
-bool MouseInterface::IsMapped(const uint8_t device_idx) const
+bool MouseInterface::IsMapped(const uint8_t physical_device_idx) const
 {
-	return mapped_idx == device_idx;
+	return mapped_physical_idx == physical_device_idx;
 }
 
 bool MouseInterface::IsEmulated() const
@@ -367,7 +367,7 @@ MouseMapStatus MouseInterface::GetMapStatus() const
 
 uint8_t MouseInterface::GetMappedDeviceIdx() const
 {
-	return mapped_idx;
+	return mapped_physical_idx;
 }
 
 int16_t MouseInterface::GetSensitivityX() const
@@ -395,44 +395,47 @@ void MouseInterface::NotifyBooting() {}
 
 void MouseInterface::NotifyDisconnect()
 {
-	SetMapStatus(MouseMapStatus::Disconnected, mapped_idx);
+	SetMapStatus(MouseMapStatus::Disconnected, mapped_physical_idx);
 }
 
-void MouseInterface::SetMapStatus(const MouseMapStatus status, const uint8_t device_idx)
+void MouseInterface::SetMapStatus(const MouseMapStatus status,
+                                  const uint8_t physical_device_idx)
 {
-	MouseMapStatus new_map_status = status;
-	uint8_t new_mapped_idx        = device_idx;
+	MouseMapStatus new_map_status   = status;
+	uint8_t new_mapped_physical_idx = physical_device_idx;
 
 	// Change 'mapped to host pointer' to just 'host pointer'
 	if (new_map_status == MouseMapStatus::Mapped &&
-	    new_mapped_idx >= mouse_info.physical.size())
+	    new_mapped_physical_idx >= mouse_info.physical.size())
 		new_map_status = MouseMapStatus::HostPointer;
 
 	// if physical device is disconnected, change state from
 	// 'mapped' to 'disconnected'
 	if (new_map_status == MouseMapStatus::Mapped &&
-	    mouse_info.physical[new_mapped_idx].IsDeviceDisconnected())
+	    mouse_info.physical[new_mapped_physical_idx].IsDeviceDisconnected())
 		new_map_status = MouseMapStatus::Disconnected;
 
-	// Perform necessary updates for mapping change
-	if (map_status != new_map_status || mapped_idx != new_mapped_idx)
+	// Perform necessary updates after mapping change
+	if (map_status != new_map_status ||
+	    mapped_physical_idx != new_mapped_physical_idx)
 		ResetButtons();
 	if (map_status != new_map_status)
 		UpdateInputType();
-	if (mapped_idx != new_mapped_idx)
-		ManyMouseGlue::GetInstance().Map(new_mapped_idx, interface_id);
+	if (mapped_physical_idx != new_mapped_physical_idx)
+		ManyMouseGlue::GetInstance().Map(new_mapped_physical_idx,
+		                                 interface_id);
 
 	// Apply new mapping
-	mapped_idx = new_mapped_idx;
-	map_status = new_map_status;
+	mapped_physical_idx = new_mapped_physical_idx;
+	map_status          = new_map_status;
 }
 
-bool MouseInterface::ConfigMap(const uint8_t device_idx)
+bool MouseInterface::ConfigMap(const uint8_t physical_device_idx)
 {
 	if (!IsEmulated())
 		return false;
 
-	SetMapStatus(MouseMapStatus::Mapped, device_idx);
+	SetMapStatus(MouseMapStatus::Mapped, physical_device_idx);
 	return true;
 }
 
diff --git a/src/hardware/mouse/mouse_interfaces.h b/src/hardware/mouse/mouse_interfaces.h
index 063ce34f6..49222d218 100644
--- a/src/hardware/mouse/mouse_interfaces.h
+++ b/src/hardware/mouse/mouse_interfaces.h
@@ -33,8 +33,9 @@ void MOUSE_NotifyDisconnect(const MouseInterfaceId interface_id);
 void MOUSE_NotifyFakePS2(); // fake PS/2 event, for VMware protocol support
 void MOUSE_NotifyResetDOS();
 
-bool MOUSE_IsCaptured();
 void MOUSE_UpdateGFX();
+bool MOUSE_IsCaptured();
+bool MOUSE_IsProbeForMappingAllowed();
 
 // ***************************************************************************
 // DOS mouse driver
@@ -140,7 +141,7 @@ public:
 	void NotifyDisconnect();
 
 	bool IsMapped() const;
-	bool IsMapped(const uint8_t device_idx) const;
+	bool IsMapped(const uint8_t physical_device_idx) const;
 	bool IsEmulated() const;
 	bool IsUsingEvents() const;
 	bool IsUsingHostPointer() const;
@@ -153,7 +154,7 @@ public:
 	uint16_t GetMinRate() const;
 	uint16_t GetRate() const;
 
-	bool ConfigMap(const uint8_t device_idx);
+	bool ConfigMap(const uint8_t physical_device_idx);
 	void ConfigUnMap();
 
 	void ConfigOnOff(const bool enable);
@@ -183,7 +184,7 @@ protected:
 	uint8_t GetInterfaceIdx() const;
 
 	void SetMapStatus(const MouseMapStatus status,
-	                  const uint8_t device_idx = idx_host_pointer);
+	                  const uint8_t physical_device_idx = idx_host_pointer);
 
 	virtual void UpdateSensitivity();
 	virtual void UpdateMinRate();
@@ -212,8 +213,8 @@ protected:
 private:
 	const MouseInterfaceId interface_id = MouseInterfaceId::None;
 
-	MouseMapStatus map_status = MouseMapStatus::HostPointer;
-	uint8_t mapped_idx = idx_host_pointer; // index of mapped physical mouse
+	MouseMapStatus map_status   = MouseMapStatus::HostPointer;
+	uint8_t mapped_physical_idx = idx_host_pointer; // index of mapped physical mouse
 
 	MouseButtons12 buttons_12   = 0; // host side buttons 1 (left), 2 (right)
 	MouseButtons345 buttons_345 = 0; // host side buttons 3 (middle), 4, and 5
diff --git a/src/hardware/mouse/mouse_manymouse.cpp b/src/hardware/mouse/mouse_manymouse.cpp
index a2cd51023..34b9aa472 100644
--- a/src/hardware/mouse/mouse_manymouse.cpp
+++ b/src/hardware/mouse/mouse_manymouse.cpp
@@ -227,7 +227,7 @@ void ManyMouseGlue::RescanIfSafe()
 	InitIfNeeded();
 }
 
-bool ManyMouseGlue::ProbeForMapping(uint8_t &device_id)
+bool ManyMouseGlue::ProbeForMapping(uint8_t &physical_device_idx)
 {
 	// Do not even try if NoMouse is configured
 	if (mouse_config.capture == MouseCapture::NoMouse)
@@ -266,7 +266,10 @@ bool ManyMouseGlue::ProbeForMapping(uint8_t &device_id)
 		// Wait for mouse button press
 		if (event.type != MANYMOUSE_EVENT_BUTTON || !event.value)
 			continue;
-		device_id = static_cast<uint8_t>(event.device);
+		// Drop button events if we have no focus, etc.
+		if (!MOUSE_IsProbeForMappingAllowed())
+			continue;
+		physical_device_idx = static_cast<uint8_t>(event.device);
 
 		if (event.item >= 1)
 			break; // user cancelled the interactive mouse mapping
@@ -274,13 +277,13 @@ bool ManyMouseGlue::ProbeForMapping(uint8_t &device_id)
 		// Do not accept already mapped devices
 		bool already_mapped = false;
 		for (const auto &interface : mouse_interfaces)
-			if (interface->IsMapped(device_id))
+			if (interface->IsMapped(physical_device_idx))
 				already_mapped = true;
 		if (already_mapped)
 			continue;
 
 		// Mouse probed successfully
-		device_id = static_cast<uint8_t>(event.device);
+		physical_device_idx = static_cast<uint8_t>(event.device);
 		success   = true;
 		break;
 	}
@@ -312,16 +315,17 @@ uint8_t ManyMouseGlue::GetIdx(const std::regex &regex)
 	return max_mice + 1; // return value which will be considered out of range
 }
 
-void ManyMouseGlue::Map(const uint8_t physical_idx, const MouseInterfaceId interface_id)
+void ManyMouseGlue::Map(const uint8_t physical_device_idx,
+                        const MouseInterfaceId interface_id)
 {
 	assert(interface_id != MouseInterfaceId::None);
 
-	if (physical_idx >= physical_devices.size()) {
+	if (physical_device_idx >= physical_devices.size()) {
 		UnMap(interface_id);
 		return;
 	}
 
-	auto &physical_device = physical_devices[physical_idx];
+	auto &physical_device = physical_devices[physical_device_idx];
 	if (interface_id == physical_device.GetMappedInterfaceId())
 		return; // nothing to update
 	physical_device.mapped_id = interface_id;
diff --git a/src/hardware/mouse/mouse_manymouse.h b/src/hardware/mouse/mouse_manymouse.h
index 66d6edb94..69753e145 100644
--- a/src/hardware/mouse/mouse_manymouse.h
+++ b/src/hardware/mouse/mouse_manymouse.h
@@ -56,10 +56,11 @@ public:
 	void StartConfigAPI();
 	void StopConfigAPI();
 
-	bool ProbeForMapping(uint8_t &device_id);
+	bool ProbeForMapping(uint8_t &physical_device_idx);
 	uint8_t GetIdx(const std::regex &regex);
 
-	void Map(const uint8_t physical_idx, const MouseInterfaceId interface_id);
+	void Map(const uint8_t physical_device_idx,
+	         const MouseInterfaceId interface_id);
 
 	bool IsMappingInEffect() const;