Fix sorting of actions

Snakybo · Feb 1, 2021 · 7b2da5a · 7b2da5a
1 parent f029b94
commit 7b2da5a
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 104 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,16 @@ The format of this changelog is based on [Keep a Changelog](https://keepachangel
 * The MINOR component is used whenever a version has backwards-compatible profile changes. This also indicates that the user can not switch back to a previous MINOR version without using a backup.
 * The PATCH component is used for versions that do not contain profile format changes. Users can freely switch between PATCH versions without risk of data loss.
 
+## [Unreleased]
+
+### Fixed
+
+- Fix binding target sorting not always following the specified order
+
+### Removed
+
+- Remove restriction to add target units after a dead end (i.e. player or cursor)
+
 ## [0.15.2] - 2021-01-21
 
 ### Added

diff --git a/Clicked/Config/Bindings.lua b/Clicked/Config/Bindings.lua
@@ -945,12 +945,10 @@ local function DrawBindingTargetPage(container, binding)
 				if Clicked:CanUnitBeDead(target.unit) then
 					DrawTargetSelectionVitals(group, enabled, target)
 				end
-
-				enabled = enabled and Clicked:CanUnitHaveFollowUp(target.unit)
 			end
 
 			-- new target
-			if enabled and Clicked:CanUnitHaveFollowUp(regular[#regular].unit) then
+			do
 				local group = GUI:InlineGroup(enabled and L["Or"] or L["Or (inactive)"])
 				container:AddChild(group)
 

diff --git a/Clicked/Core/BindingProcessor.lua b/Clicked/Core/BindingProcessor.lua
@@ -87,7 +87,7 @@ local regularBucket = {}
 
 local isPendingReload = false
 
-local function GetMacroSegmentFromAction(action, interactionType)
+local function GetMacroSegmentFromAction(action, interactionType, isLast)
 	local flags = {}
 	local impliedExists = false
 
@@ -151,7 +151,7 @@ local function GetMacroSegmentFromAction(action, interactionType)
 		table.insert(flags, "nopet")
 	end
 
-	if not impliedExists and interactionType == Clicked.InteractionType.REGULAR and Clicked:CanUnitHaveFollowUp(action.unit) then
+	if not impliedExists and interactionType == Clicked.InteractionType.REGULAR and not isLast then
 		table.insert(flags, "exists")
 	end
 
@@ -281,81 +281,70 @@ local function ConstructActions(binding, interactionType)
 		for _, target in ipairs(binding.targets.regular) do
 			local action = ConstructAction(binding, target)
 			table.insert(actions, action)
-
-			if not Clicked:CanUnitHaveFollowUp(target.unit) then
-				break
-			end
 		end
 	end
 
 	return actions
 end
 
-local function SortActions(left, right)
-	-- 1. @mouseover
-	if left.unit == Clicked.TargetUnits.MOUSEOVER and right.unit ~= Clicked.TargetUnits.MOUSEOVER then
-		return true
-	end
-
-	if left.unit ~= Clicked.TargetUnits.MOUSEOVER and right.unit == Clicked.TargetUnits.MOUSEOVER then
-		return false
-	end
-
-	-- 2. any hostility flags (help, harm)
-	if #left.hostility > 0 and #right.hostility == 0 then
-		return true
-	end
-
-	if #left.hostility == 0 and #right.hostility > 0 then
-		return false
-	end
-
-	-- 3. any vitals flags (dead, nodead)
-	if #left.vitals > 0 and #right.vitals == 0 then
-		return true
-	end
-
-	if #left.vitals == 0 and #right.vitals > 0 then
-		return false
-	end
-
-	-- 4. any combat flags (combat, nocombat)
-	if #left.combat > 0 and #right.combat == 0 then
-		return true
-	end
+local function SortActions(actions, indexMap)
+	local function SortFunc(left, right)
+		local priority = {
+			-- 1. Mouseover targets always come first
+			{ left = left.unit, right = right.unit, value = Clicked.TargetUnits.MOUSEOVER, comparison = "eq" },
+
+			-- 2. Hostility, vitals, combat, and form flags take presedence over actions
+			--    that don't specify them explicitly
+			{ left = #left.hostility, right = #right.hostility, value = 0, comparison = "gt" },
+			{ left = #left.vitals, right = #right.vitals, value = 0, comparison = "gt" },
+			{ left = #left.combat, right = #right.combat, value = 0, comparison = "gt" },
+			{ left = #left.forms, right = #right.forms, value = 0, comparison = "gt" },
+
+			-- 3. Any actions that do not meet any of the criteria in this list will be placed here
+
+			-- 4. The player, cursor, and default targets will always come last
+			{ left = left.unit, right = right.unit, value = Clicked.TargetUnits.PLAYER, comparison = "neq" },
+			{ left = left.unit, right = right.unit, value = Clicked.TargetUnits.CURSOR, comparison = "neq" },
+			{ left = left.unit, right = right.unit, value = Clicked.TargetUnits.DEFAULT, comparison = "neq" }
+		}
 
-	if #left.combat == 0 and #right.combat > 0 then
-		return false
-	end
+		for _, item in ipairs(priority) do
+			local l = item.left
+			local r = item.right
+			local v = item.value
+			local c = item.comparison
 
-	-- 5. any form flags (forms:N)
-	if #left.forms > 0 and #right.forms == 0 then
-		return true
-	end
+			if c == "neq" then
+				c = "eq"
 
-	if #left.forms == 0 and #right.forms > 0 then
-		return false
-	end
+				local t = l
+				l = r
+				r = t
+			end
 
-	-- 6. @player
-	if left.unit == Clicked.TargetUnits.PLAYER and right.unit ~= Clicked.TargetUnits.PLAYER then
-		return false
-	end
+			if c == "eq" then
+				if l == v and r ~= v then
+					return true
+				end
 
-	if left.unit ~= Clicked.TargetUnits.PLAYER and right.unit == Clicked.TargetUnits.PLAYER then
-		return true
-	end
+				if l ~= v and r == v then
+					return false
+				end
+			elseif c == "gt" then
+				if l > v and r == v then
+					return true
+				end
 
-	-- 7. default
-	if left.unit == Clicked.TargetUnits.DEFAULT and right.unit ~= Clicked.TargetUnits.DEFAULT then
-		return false
-	end
+				if l == v and r > v then
+					return false
+				end
+			end
+		end
 
-	if left.unit ~= Clicked.TargetUnits.DEFAULT and right.unit == Clicked.TargetUnits.DEFAULT then
-		return true
+		return indexMap[left] < indexMap[right]
 	end
 
-	return false
+	table.sort(actions, SortFunc)
 end
 
 local function GetInternalBindingType(binding)
@@ -827,6 +816,7 @@ function Clicked:GetMacroForBindings(bindings, interactionType)
 	local targetUnitAfterCast = false
 
 	local actions = {}
+	local actionIndexMap = {}
 
 	-- add a segment to remove the blue casting cursor
 	table.insert(result, "/click " .. Clicked.STOP_CASTING_BUTTON_NAME)
@@ -853,10 +843,6 @@ function Clicked:GetMacroForBindings(bindings, interactionType)
 						table.insert(extra, "/startattack [@target,harm]")
 						break
 					end
-
-					if not Clicked:CanUnitHaveFollowUp(target.unit) then
-						break
-					end
 				end
 			end
 
@@ -869,23 +855,28 @@ function Clicked:GetMacroForBindings(bindings, interactionType)
 				table.insert(result, 1, extra[i])
 			end
 
+			local next = 1
+
 			for _, action in ipairs(ConstructActions(binding, interactionType)) do
 				table.insert(actions, action)
+
+				actionIndexMap[action] = next
+				next = next + 1
 			end
 		end
 	end
 
 	-- Now sort the actions according to the above schema
 
-	table.sort(actions, SortActions)
+	SortActions(actions, actionIndexMap)
 
 	-- Construct a valid macro from the data
 
 	local allFlags = {}
 	local segments = {}
 
-	for _, action in ipairs(actions) do
-		local flags = GetMacroSegmentFromAction(action, interactionType)
+	for i, action in ipairs(actions) do
+		local flags = GetMacroSegmentFromAction(action, interactionType, i == #actions)
 
 		if #flags > 0 then
 			flags = "[" .. flags .. "] "

diff --git a/Clicked/Core/Utils.lua b/Clicked/Core/Utils.lua
@@ -458,34 +458,6 @@ function Clicked:CanUnitBeDead(unit)
 	return valid[unit] == true
 end
 
---- Check if a binding's target unit can have a follow up target. This will be
---- the case for most targets, but some targets act as a stop sign in macro code
---- as they will always be valid. For example [@player] or [@cursor] will always
---- be 'true' and thus it doesn't make sense to allow targets beyond.
----
---- @param unit string
---- @return string
-function Clicked:CanUnitHaveFollowUp(unit)
-	local valid = {
-		[Clicked.TargetUnits.TARGET] = true,
-		[Clicked.TargetUnits.TARGET_OF_TARGET] = true,
-		[Clicked.TargetUnits.PET] = true,
-		[Clicked.TargetUnits.PET_TARGET] = true,
-		[Clicked.TargetUnits.PARTY_1] = true,
-		[Clicked.TargetUnits.PARTY_2] = true,
-		[Clicked.TargetUnits.PARTY_3] = true,
-		[Clicked.TargetUnits.PARTY_4] = true,
-		[Clicked.TargetUnits.PARTY_5] = true,
-		[Clicked.TargetUnits.ARENA_1] = true,
-		[Clicked.TargetUnits.ARENA_2] = true,
-		[Clicked.TargetUnits.ARENA_3] = true,
-		[Clicked.TargetUnits.FOCUS] = true,
-		[Clicked.TargetUnits.MOUSEOVER] = true,
-	}
-
-	return valid[unit] == true
-end
-
 --- Notify the user that Clicked is currently in combat lockdown mode,
 --- this will print a message to the user's chat frame with a helpful message.
 function Clicked:NotifyCombatLockdown()

diff --git a/Clicked/Widgets/AceGUIContainer-ClickedTreeGroup.lua b/Clicked/Widgets/AceGUIContainer-ClickedTreeGroup.lua
@@ -564,10 +564,6 @@ local function Button_OnEnter(frame)
 			for i, target in ipairs(binding.targets.regular) do
 				local str = Clicked:GetLocalizedTargetString(target)
 				text = text .. "|cFFFFFFFF" .. i .. ". " .. str .. "|r\n"
-
-				if not Clicked:CanUnitHaveFollowUp(target.unit) then
-					break
-				end
 			end
 		end