Scratchpad stop un and re subscribing (#141)

* Formatting and small improvements

* Refactor to stop resubscribing each time

* Remove prints

* This is less error prone

* Fix abort when tag is switched for the new changes

* Fix animation_turn_off not getting called when only 1 animation is used

module/scratchpad.lua

@@ -1,16 +1,111 @@
 local awful = require("awful")
 local gears = require("gears")
 local naughty = require("naughty")
-
-local ruled
-if awesome.version ~= "v4.3" then
-    ruled = require("ruled")
-end
-
 local helpers = require(tostring(...):match(".*bling") .. ".helpers")
+local capi = { awesome = awesome, client = client }
+local ruled = capi.awesome.version ~= "v4.3" and require("ruled") or nil
+local pairs = pairs
 
 local Scratchpad = { mt = {} }
 
+--- Called when the turn off animation has ended
+local function on_animate_turn_off_end(self, tag)
+    -- When toggling off a scratchpad that's present on multiple tags
+    -- depsite still being unminizmied on the other tags it will become invisible
+    -- as it's position could be outside the screen from the animation
+    self.client:geometry({
+        x = self.geometry.x + self.client.screen.geometry.x,
+        y = self.geometry.y + self.client.screen.geometry.y,
+        width = self.geometry.width,
+        height = self.geometry.height,
+    })
+
+    helpers.client.turn_off(self.client, tag)
+
+    self.turning_off = false
+
+    self:emit_signal("turn_off", self.client)
+end
+
+--- The turn off animation
+local function animate_turn_off(self, anim, axis)
+    self.screen_on_toggled_scratchpad = self.client.screen
+    self.tag_on_toggled_scratchpad = self.screen_on_toggled_scratchpad.selected_tag
+
+    if self.client.floating == false then
+        -- Save the client geometry before floating it
+        local non_floating_x = self.client.x
+        local non_floating_y = self.client.y
+        local non_floating_width = self.client.width
+        local non_floating_height = self.client.height
+
+        -- Can't animate non floating clients
+        self.client.floating = true
+
+        -- Set the client geometry back to what it was before floating it
+        self.client:geometry({
+            x = non_floating_x,
+            y = non_floating_y,
+            width = non_floating_width,
+            height = non_floating_height,
+        })
+    end
+
+    if axis == "x" then
+        anim.pos = self.client.x
+    else
+        anim.pos = self.client.y
+    end
+
+    anim:set(anim:initial())
+end
+
+-- Handles changing tag mid animation
+local function abort_if_tag_was_switched(self)
+    -- Check for the following scenerio:
+    -- Toggle on scratchpad at tag 1
+    -- Toggle on scratchpad at tag 2
+    -- Toggle off scratchpad at tag 1
+    -- Switch to tag 2
+    -- Outcome: The client will remain on tag 1 and will instead be removed from tag 2
+    if (self.turning_off) and (self.screen_on_toggled_scratchpad and
+        self.screen_on_toggled_scratchpad.selected_tag) ~= self.tag_on_toggled_scratchpad
+    then
+        if self.rubato.x then
+            self.rubato.x:abort()
+        end
+        if self.rubato.y then
+            self.rubato.y:abort()
+        end
+        on_animate_turn_off_end(self, self.tag_on_toggled_scratchpad)
+        self.screen_on_toggled_scratchpad.selected_tag = nil
+        self.tag_on_toggled_scratchpad = nil
+    end
+end
+
+--- The turn on animation
+local function animate_turn_on(self, anim, axis)
+    -- Check for the following scenerio:
+    -- Toggle on scratchpad at tag 1
+    -- Toggle on scratchpad at tag 2
+    -- The animation will instantly end
+    -- as the timer pos is already at the on position
+    -- from toggling on the scratchpad at tag 1
+    if axis == "x" and anim.pos == self.geometry.x then
+        anim.pos = anim:initial()
+    else
+        if anim.pos == self.geometry.y then
+            anim.pos = anim:initial()
+        end
+    end
+
+    if axis == "x" then
+        anim:set(self.geometry.x)
+    else
+        anim:set(self.geometry.y)
+    end
+end
+
 --- Creates a new scratchpad object based on the argument
 --
 -- @param args A table of possible arguments
@@ -25,10 +120,40 @@ function Scratchpad:new(args)
     end
 
     args.rubato = args.rubato or {}
-    args.in_anim = false
+
-    local ret = gears.object({})
+    local ret = gears.object{}
     gears.table.crush(ret, Scratchpad)
     gears.table.crush(ret, args)
+
+    if ret.rubato.x then
+        ret.rubato.x:subscribe(function(pos)
+            if ret.client and ret.client.valid then
+                ret.client.x = pos
+            end
+            abort_if_tag_was_switched(ret)
+        end)
+
+        ret.rubato.x.ended:subscribe(function()
+            if ((ret.rubato.y and ret.rubato.y.state == false) or (ret.rubato.y == nil)) and ret.turning_off == true then
+                on_animate_turn_off_end(ret)
+            end
+        end)
+    end
+    if ret.rubato.y then
+        ret.rubato.y:subscribe(function(pos)
+            if ret.client and ret.client.valid then
+                ret.client.y = pos
+            end
+            abort_if_tag_was_switched(ret)
+        end)
+
+        ret.rubato.y.ended:subscribe(function()
+            if ((ret.rubato.x and ret.rubato.x.state == false) or (ret.rubato.x == nil)) and ret.turning_off == true then
+                on_animate_turn_off_end(ret)
+            end
+        end)
+    end
+
     return ret
 end
 
@@ -56,6 +181,7 @@ function Scratchpad:apply(c)
         width = self.geometry.width,
         height = self.geometry.height,
     })
+
     if self.autoclose then
         c:connect_signal("unfocus", function(c1)
             c1.sticky = false -- client won't turn off if sticky
@@ -64,82 +190,48 @@ function Scratchpad:apply(c)
     end
 end
 
---- The turn on animation
-local function animate_turn_on(self, c, anim, axis)
-    -- Check for the following scenerio:
-    -- Toggle on scratchpad at tag 1
-    -- Toggle on scratchpad at tag 2
-    -- The animation will instantly end
-    -- as the timer pos is already at the on position
-    -- from toggling on the scratchpad at tag 1
-    if axis == "x" and anim.pos == self.geometry.x then
-        anim.pos = anim:initial()
-    else
-        if anim.pos == self.geometry.y then
-            anim.pos = anim:initial()
-        end
-    end
-
-    anim:subscribe(function(pos)
-        if c and c.valid then
-            if axis == "x" then
-                c.x = pos
-            else
-                c.y = pos
-            end
-        end
-        self.in_anim = true
-    end)
-
-    if axis == "x" then
-        anim:set(self.geometry.x)
-    else
-        anim:set(self.geometry.y)
-    end
-
-    anim.ended:subscribe(function()
-        self.in_anim = false
-        anim:unsubscribe()
-        anim.ended:unsubscribe()
-    end)
-end
-
 --- Turns the scratchpad on
 function Scratchpad:turn_on()
-    local c = self:find()[1]
+    self.client = self:find()[1]
+
     local anim_x = self.rubato.x
     local anim_y = self.rubato.y
 
-    if c and not self.in_anim and c.first_tag and c.first_tag.selected then
+    local in_anim = false
-        c:raise()
+    if (anim_x and anim_x.state == true) or (anim_y and anim_y.state == true) then
-        client.focus = c
+        in_anim = true
+    end
+
+    if self.client and not in_anim and self.client.first_tag and self.client.first_tag.selected then
+        self.client:raise()
+        capi.client.focus = self.client
         return
     end
-    if c and not self.in_anim then
+    if self.client and not in_anim then
         -- if a client was found, turn it on
         if self.reapply then
-            self:apply(c)
+            self:apply(self.client)
         end
         -- c.sticky was set to false in turn_off so it has to be reapplied anyway
-        c.sticky = self.sticky
+        self.client.sticky = self.sticky
 
         if anim_x then
-            animate_turn_on(self, c, anim_x, "x")
+            animate_turn_on(self, anim_x, "x")
         end
         if anim_y then
-            animate_turn_on(self, c, anim_y, "y")
+            animate_turn_on(self, anim_y, "y")
         end
 
-        helpers.client.turn_on(c)
+        helpers.client.turn_on(self.client)
-        self:emit_signal("turn_on", c)
+        self:emit_signal("turn_on", self.client)
 
         return
     end
-    if not c then
+    if not self.client then
         -- if no client was found, spawn one, find the corresponding window,
         --  apply the properties only once (until the next closing)
         local pid = awful.spawn.with_shell(self.command)
-        if awesome.version ~= "v4.3" then
+        if capi.awesome.version ~= "v4.3" then
             ruled.client.append_rule({
                 id = "scratchpad",
                 rule = self.rule,
@@ -159,6 +251,8 @@ function Scratchpad:turn_on()
                         autostart = true,
                         single_shot = true,
                         callback = function()
+                            self.client = c
+
                             self:apply(c)
                             c.hidden = false
                             c.minimized = false
@@ -166,10 +260,10 @@ function Scratchpad:turn_on()
                             c:activate({})
 
                             if anim_x then
-                                animate_turn_on(self, c, anim_x, "x")
+                                animate_turn_on(self, anim_x, "x")
                             end
                             if anim_y then
-                                animate_turn_on(self, c, anim_y, "y")
+                                animate_turn_on(self, anim_y, "y")
                             end
 
                             self:emit_signal("inital_apply", c)
@@ -189,127 +283,50 @@ function Scratchpad:turn_on()
         else
             local function inital_apply(c1)
                 if helpers.client.is_child_of(c1, pid) then
+                    self.client = c1
+
                     self:apply(c1)
                     if anim_x then
-                        animate_turn_on(self, c1, anim_x, "x")
+                        animate_turn_on(self, anim_x, "x")
                     end
                     if anim_y then
-                        animate_turn_on(self, c1, anim_y, "y")
+                        animate_turn_on(self, anim_y, "y")
                     end
                     self:emit_signal("inital_apply", c1)
-                    client.disconnect_signal("manage", inital_apply)
+                    self.client.disconnect_signal("manage", inital_apply)
                 end
             end
-            client.connect_signal("manage", inital_apply)
+            self.client.connect_signal("manage", inital_apply)
         end
     end
 end
 
---- Called when the turn off animation has ended
-local function on_animate_turn_off_end(self, c, anim, tag, turn_off_on_end)
-    anim:unsubscribe()
-    anim.ended:unsubscribe()
-
-    if turn_off_on_end then
-        -- When toggling off a scratchpad that's present on multiple tags
-        -- depsite still being unminizmied on the other tags it will become invisible
-        -- as it's position could be outside the screen from the animation
-        c:geometry({
-            x = self.geometry.x + c.screen.geometry.x,
-            y = self.geometry.y + c.screen.geometry.y,
-            width = self.geometry.width,
-            height = self.geometry.height,
-        })
-        helpers.client.turn_off(c, tag)
-
-        self:emit_signal("turn_off", c)
-
-        self.in_anim = false
-    end
-end
-
---- The turn off animation
-local function animate_turn_off(self, c, anim, axis, turn_off_on_end)
-    local screen_on_toggled_scratchpad = c.screen
-    local tag_on_toggled_scratchpad = screen_on_toggled_scratchpad.selected_tag
-
-    if c.floating == false then
-        -- Save the client geometry before floating it
-        local non_floating_x = c.x
-        local non_floating_y = c.y
-        local non_floating_width = c.width
-        local non_floating_height = c.height
-
-        -- Can't animate non floating clients
-        c.floating = true
-
-        -- Set the client geometry back to what it was before floating it
-        c:geometry({
-            x = non_floating_x,
-            y = non_floating_y,
-            width = non_floating_width,
-            height = non_floating_height,
-        })
-    end
-
-
-    if axis == "x" then
-        anim.pos = c.x
-    else
-        anim.pos = c.y
-    end
-
-    anim:subscribe(function(pos)
-        if c and c.valid then
-            if axis == "x" then
-                c.x = pos
-            else
-                c.y = pos
-            end
-        end
-        self.in_anim = true
-
-        -- Handles changing tag mid animation
-        -- Check for the following scenerio:
-        -- Toggle on scratchpad at tag 1
-        -- Toggle on scratchpad at tag 2
-        -- Toggle off scratchpad at tag 1
-        -- Switch to tag 2
-        -- Outcome: The client will remain on tag 1 and will instead be removed from tag 2
-        if screen_on_toggled_scratchpad.selected_tag ~= tag_on_toggled_scratchpad then
-            on_animate_turn_off_end(self, c, anim, tag_on_toggled_scratchpad, true)
-        end
-    end)
-
-    anim:set(anim:initial())
-
-    anim.ended:subscribe(function()
-        on_animate_turn_off_end(self, c, anim, nil, turn_off_on_end)
-    end)
-end
-
 --- Turns the scratchpad off
 function Scratchpad:turn_off()
-    local c = self:find()[1]
+    self.client = self:find()[1]
-    if c and not self.in_anim then
-        -- Get the tweens
-        local anim_x = self.rubato.x
-        local anim_y = self.rubato.y
 
-        local anim_x_duration = (anim_x and anim_x.duration) or 0
+    -- Get the tweens
-        local anim_y_duration = (anim_y and anim_y.duration) or 0
+    local anim_x = self.rubato.x
+    local anim_y = self.rubato.y
 
-        local turn_off_on_end = (anim_x_duration >= anim_y_duration) and true or false
+    local in_anim = false
+    if (anim_x and anim_x.state == true) or (anim_y and anim_y.state == true) then
+        in_anim = true
+    end
+
+    if self.client and not in_anim then
         if anim_x then
-            animate_turn_off(self, c, anim_x, "x", turn_off_on_end)
+            self.turning_off = true
+            animate_turn_off(self, anim_x, "x")
         end
         if anim_y then
-            animate_turn_off(self, c, anim_y, "y", not turn_off_on_end)
+            self.turning_off = true
+            animate_turn_off(self, anim_y, "y")
         end
 
         if not anim_x and not anim_y then
-            helpers.client.turn_off(c)
+            helpers.client.turn_off(self.client)
-            self:emit_signal("turn_off", c)
+            self:emit_signal("turn_off", self.client)
         end
     end
 end
@@ -335,8 +352,8 @@ function Scratchpad:toggle()
             end
         end
     else
-        is_turn_off = client.focus
+        is_turn_off = capi.client.focus
-            and awful.rules.match(client.focus, self.rule)
+            and awful.rules.match(capi.client.focus, self.rule)
     end
 
     if is_turn_off then