hammerspoon icon indicating copy to clipboard operation
hammerspoon copied to clipboard

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

Open luckman212 opened this issue 2 years ago • 17 comments

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:

  • ignore it?
  • comment out the hs.canvas:delete() lines?
  • change to hs.canvasObj = nil or something?

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

luckman212 avatar Dec 13 '21 16:12 luckman212

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

cmsj avatar Dec 13 '21 17:12 cmsj

Got it, thanks

luckman212 avatar Dec 13 '21 17:12 luckman212

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?

luckman212 avatar Dec 13 '21 17:12 luckman212

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()

cmsj avatar Dec 13 '21 18:12 cmsj

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

luckman212 avatar Dec 13 '21 18:12 luckman212

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 avatar Dec 13 '21 19:12 asmagill

@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.

asmagill avatar Dec 13 '21 19:12 asmagill

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

cmsj avatar Dec 13 '21 19:12 cmsj

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

luckman212 avatar Dec 14 '21 00:12 luckman212

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.

asmagill avatar Dec 14 '21 00:12 asmagill

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 avatar Dec 14 '21 02:12 luckman212

@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:

  • set your preferred hotkey (L15) default is F13/PrtScr
  • optional: set sounds (L33-36)
  • hit main hotkey while a window is focused to activate the floater (underlying window will be moved offscreen)
  • press again to cycle thru the various sizes
  • press CTRL+(hotkey) to cycle thru refresh intervals (also adjustable @L16)
  • press SHIFT+(hotkey) to close the floater
  • CMD+(hotkey) or double click the floater to toggle bring the source window back into view
  • right-click and drag the floater to pan around the underlying preview area

luckman212 avatar Dec 14 '21 15:12 luckman212

@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...

luckman212 avatar Dec 17 '21 16:12 luckman212

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

cmsj avatar Dec 17 '21 18:12 cmsj

@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.

luckman212 avatar Jan 13 '22 16:01 luckman212

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

kiryph avatar Jul 30 '22 10:07 kiryph

@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.

asmagill avatar Jul 30 '22 15:07 asmagill