Hammerspoon / hammerspoon

Staggeringly powerful macOS desktop automation with Lua
http://www.hammerspoon.org
MIT License
12.11k stars 582 forks source link

How to handle `Warning: LuaSkin: hs.canvas:delete - explicit delete is no longer required for canvas objects; garbage collection occurs automatically` #3021

Open luckman212 opened 2 years ago

luckman212 commented 2 years ago

Hi, with 0.9.93 I am getting warnings from some of my modules that use hs.canvas:

2021-12-13 11:49:05: 11:49:05 ** Warning:   LuaSkin: hs.canvas:delete - explicit delete is no longer required for canvas objects; garbage collection occurs automatically

I'm not sure what the right way to deal with this is:

What's the preferred way for explicitly destroying a canvas object now?

cmsj commented 2 years ago

assigning nil to the objects in question would be the right way to go now.

luckman212 commented 2 years ago

Got it, thanks

luckman212 commented 2 years ago

Hmm - it's not working for me. Sorry to be a PITA @cmsj. But I removed the canvasObj:delete() calls and now my objects are not being removed, even after setting to nil

Also, the parser is still printing the warnings on the console, even though I've commented out the lines with -- - guess it's not checking for that?

cmsj commented 2 years ago

I’m going to ping @asmagill in case there is a subtlety I’m missing here, but in the mean time you could explicitly call collectgarbage()

luckman212 commented 2 years ago

Sorry, forgot to mention, I am already calling collectgarbage() but the object isn't being destroyed.

asmagill commented 2 years ago

Try the following in the console:

First, type in the following:

d = hs.canvas.new{x = 100, y = 100, h = 100, w = 100}:show():appendElements{ type = "rectangle" }

After the red rectangle appears, next type in the following and it should disappear:

d = nil ; collectgarbage()

If that doesn't work, then one or more of the recent updates has broken something; if it does, then your items are being held somewhere else, either as up-values or in other variables... I'd need to see your code to tell for certain.

asmagill commented 2 years ago

@cmsj, should we make delete a synonym for hide? I know it won't free memory like clearing it and collecting garbage does, but at least the item would "disappear" from view.

cmsj commented 2 years ago

@asmagill yeah that does seem like a good idea, although istr the PR description suggested that was going to be the new behaviour?

luckman212 commented 2 years ago

Thanks @asmagill - your test code worked, so I dug in a bit and I think I figured out what's going on.

I was using multiple layers/elements as well as click handlers/callbacks on my canvas object. I think even though I set the canvas to nil, having these attached prevented them from being gc'd. I found that I not only had to set the hs.canvas object itself to nil but also be sure to remove all the elements first, as well as set the callbacks to nil.

I made a simple routine to make this less repetetive:

function destroyCanvasObj(cObj,gc)
  if not cObj then return end
  for i=#cObj,1,-1 do
    cObj[i] = nil
  end
  cObj:clickActivating(false)
  cObj:mouseCallback(nil)
  cObj:canvasMouseEvents(nil, nil, nil, nil)
  cObj = nil
  if gc and gc == true then collectgarbage() end
end

d = hs.canvas.new{x = 100, y = 100, h = 100, w = 100}:show():appendElements{ type = "rectangle" }

destroyCanvasObj(d,true)

The only thing I couldn't figure out was how to actually destroy d altogether (I don't think this is possible by reference in Lua??) So I reworked my code a bit, instead of if d then ... I use if d and d:elementCount() > 0 ... which seems to work, although I imagine carries a bit more overhead.

Also: along the way, I think I found a little errata at https://www.hammerspoon.org/docs/hs.canvas.html#canvasMouseEvents

I think this should be canvasMouseEvents ? image

asmagill commented 2 years ago

Hmmm... the number of elements shouldn't matter (the example code had 1 after all that we didn't explicitly clear), but I'll dig in to it deeper next week (I have a whole week with nothing else going on for once!)... and the callbacks... again, they should be cleared in the __gc function for the object... unless one of them refers to the canvas as an up-value... I'll give it some thought and see if there might be a work around I can implement... or at least update the documentation or wiki examples to explain best practices and what to do to avoid the problem.

luckman212 commented 2 years ago

Thanks @asmagill ... Wow, a whole week to tinker around! What a luxury! 🌴

I think my code is probably at least one (or maybe all) of these things:

1) overly complicated 2) not optimal 3) flat out incorrect

So that could be the reason it's not working quite as expected. I'll put it up in a gist shortly so you can have a go with it.

luckman212 commented 2 years ago

@asmagill I put up a gist of my module if you want to do any testing with it. I would really appreciate it!

float2.lua - https://gist.github.com/luckman212/615ad90c3ac4fce2460fc22505671a26

To use:

luckman212 commented 2 years ago

@cmsj I know this is closed (should it be?) but, I found the line that causes this to print: https://github.com/Hammerspoon/hammerspoon/blob/02f60327d753ccffe13fb4f7bf95492bdd66c60f/extensions/canvas/libcanvas.m#L3184 It looks like I was wrong about when this is printed to the console. Seems like it happens iff the :delete() method actually gets called, and only if the warnedAboutDelete counter is < 10.

I checked and re-checked my code, and I already removed all explicit references to hs.canvas:delete(). But I'm still getting 6 of these in my console every time HS launches. Is it possibly being referenced somewhere else internally? Any way to find out what module or line is calling?

edit: I found at least 2 internal references in drawing_canvasWrapper.lua at line 168 & 216 e.g:

drawingMT.delete = function(self)
    drawingMT.delete = function(self)
    self.canvas = self.canvas:delete()
    setmetatable(self, nil)
end

changing to:

drawingMT.delete = function(self)
    drawingMT.delete = function(self)
    -- self.canvas = self.canvas:delete()
    self.canvas = nil
    collectgarbage()
    setmetatable(self, nil)
end

eliminated the warnings on startup for me. I am sure there are others, I believe hs.alert has some as well...

cmsj commented 2 years ago

It could well be being called by something internal, so I'll re-open the Issue for investigation. Thanks!

luckman212 commented 2 years ago

@cmsj Is there any way to destroy a canvas object by reference only? I am trying to use a function like this to destroy my objects since I have quite a few different ones, with differing # of elements/layers. It seems that the root object is never destroyed.

function hstype(obj)
  if getmetatable(obj) and getmetatable(obj).__type then
    return getmetatable(obj).__type
  elseif getmetatable(obj._hk) and getmetatable(obj._hk).__type then
    return getmetatable(obj._hk).__type
  else
    return type(obj)
  end
end

function destroyCanvasObj(cObj,gc)
  if not cObj or hstype(cObj) ~= 'hs.canvas' then
    if gc and gc == true then collectgarbage() end
    return
  end
  for i=#cObj,1,-1 do
    cObj[i] = nil
  end
  cObj:clickActivating(false)
  cObj:mouseCallback(nil)
  cObj:canvasMouseEvents(nil, nil, nil, nil)
  cObj = nil
  if gc and gc == true then collectgarbage() end
end

e.g. if I call this as destroyCanvasObj(floater,true) it will remove the drawing from the screen, but the actual floater hs.canvas object is not destroyed and leads to an artifact later on. Maybe my Lua skills aren't up to this challenge.

kiryph commented 2 years ago

I have modified /Applications/Hammerspoon.app/Contents/Resources/extensions/hs/drawing_canvasWrapper.lua as following

diff --git a/drawing_canvasWrapper-orig.lua b/drawing_canvasWrapper.lua
index ce040a7..a59988d 100644
--- a/drawing_canvasWrapper-orig.lua
+++ b/drawing_canvasWrapper.lua
@@ -165,7 +165,8 @@ module.getTextDrawingSize = function(message, textStyle)
         if textStyle.lineBreak then drawingObject._default.textLineBreak = textStyle.lineBreak end
     end
     local frameSize = drawingObject:minimumTextSize(message)
-    drawingObject:delete()
+    drawingObject:hide()
+    drawingObject = nil
     return frameSize
 end

@@ -213,7 +214,8 @@ drawingMT.clippingRectangle = function(self, ...)
 end

 drawingMT.delete = function(self)
-    self.canvas = self.canvas:delete()
+    self.canvas = self.canvas:hide()
+    self.canvas = nil
     setmetatable(self, nil)
 end

This has removed the noise in the hammerspoon console after a restart of hammerspoon (not only a reload of the config)

Warning: LuaSkin: hs.canvas:delete - explicit delete is no longer required for canvas objects; garbage collection occurs automatically

which I had quite a bit since I use hs.alert to indicate the actions of my window managing hotkeys: Screenshot 2022-07-30 at 12 33 23

asmagill commented 2 years ago

@cmsj, while my time is somewhat limited again, I wonder if it would be worth it for me to go through the current spoons and modules and correct/remove all of the uses of hs.canvas:delete... I had hoped this message would prompt the original contributors to update their own code, but this has turned out to be on the optimistic side.