Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory management for GObject signals #334

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Memory management for GObject signals #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4dccb0
Memory management for GObject signals
oezingle Feb 13, 2025
4c2be02
Reformatted detailed signal name to match GObject's documentation
oezingle Feb 13, 2025
bb3ae90
detailed signals now have access to :connect and :disconnect. Object.…
oezingle Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions demo.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
-- Demo modified from https://github.com/lgi-devs/lgi/issues/333 - the stated issues should be resolved.

-- hack to require lgi from this directory before /usr/ or wherever windows installs it.
local sep = package.path:match("[/\\]")
package.path = ("./?.lua;./?/init.lua;"):gsub("/", sep) .. package.path

local lgi, lgi_path = require("lgi")

-- check the hack worked
assert(lgi_path:match("%.[/\\]lgi.lua"), "Loaded wrong LGI!")

local Gtk = lgi.require("Gtk", "3.0")

local button_1 = Gtk.Button.new_with_label("I can be clicked multiple times, and now handlers are overwritten")

---@diagnostic disable-next-line:duplicate-set-field
button_1.on_clicked = function()
print("Button 2 Click handler 1!")
end
---@diagnostic disable-next-line:duplicate-set-field
button_1.on_clicked = function()
print("Button 2 Click handler 2!")
end

-- This button demonstrates the difficulty in disconnecting a signal:
local button_2 = Gtk.Button.new_with_label("I can be clicked once!")

local handler_id
-- using widget.<signal>:connect from https://github.com/lgi-devs/lgi/blob/master/docs/guide.md#341-connecting-signals
handler_id = button_2.on_clicked:connect(function(widget)
print("Button 1 Clicked!")
widget:set_label("I can no longer be clicked!")

-- Now I can disconnect nicely!
button_2.on_clicked:disconnect(handler_id)
end)

button_2.on_notify["has-focus"]:connect(function (...)
print("has-focus", ...)
end)

local window = Gtk.Window {
title = "GitHub PR demo",
default_width = 400,
default_height = 300,
on_destroy = Gtk.main_quit,
child = Gtk.VBox {
button_1,
button_2
},
}

window:present()
window:show_all()

Gtk.main()
1 change: 1 addition & 0 deletions lgi/class.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ function class.load_class(namespace, info)
class._method = component.get_category(info.methods, load_method)
class._signal = component.get_category(
info.signals, nil, load_signal_name, load_signal_name_reverse)
class._signal_handler = {} -- initialized empty in order to memory manage signal handlers created via assignment operator
class._constant = component.get_category(info.constants, core.constant)
class._field = component.get_category(info.fields)
local type_struct = info.type_struct
Expand Down
43 changes: 39 additions & 4 deletions lgi/override/GObject-Object.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ local function marshal_property(obj, name, flags, gtype, marshaller, ...)
local mode = select('#', ...) > 0 and 'WRITABLE' or 'READABLE'
if not flags[mode] then
error(("%s: `%s' not %s"):format(core.object.query(obj, 'repo')._name,
name, core.downcase(mode)))
name, core.downcase(mode)))
end
local value = Value(gtype)
if mode == 'WRITABLE' then
Expand Down Expand Up @@ -292,6 +292,7 @@ end
local quark_from_string = repo.GLib.quark_from_string
local signal_lookup = repo.GObject.signal_lookup
local signal_connect_closure_by_id = repo.GObject.signal_connect_closure_by_id
local signal_handler_disconnect = repo.GObject.signal_handler_disconnect
local signal_emitv = repo.GObject.signal_emitv
-- Connects signal to specified object instance.
local function connect_signal(obj, gtype, name, closure, detail, after)
Expand Down Expand Up @@ -319,9 +320,19 @@ end
-- Signal accessor.
function Object:_access_signal(object, info, ...)
local gtype = self._gtype

-- faster, as every branch indexes info.name more than once
local name = info.name

if select('#', ...) > 0 then
local existing_handler = self._signal_handler[name]

if existing_handler then
signal_handler_disconnect(object, existing_handler)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think I like that. There might very well be reasons to have more than one signal handler connected to the same signal. (And yes, I know that this is kinda weird that an assignment only adds but does not override, but that wasn't me 🤷.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but that's why signal handler pads are useful. GObject.on_<signal>:connect() allows us to connect multiple handlers, whereas my fork only allows a single handler at a time to be attached via the assignment operator. This, in my opinion is the best of both worlds.


-- Assignment means 'connect signal without detail'.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno what this comment refers to exactly, but whatever "connect signal with detail" is, does it provide a way to disconnect? Is that what the code below does / adds / implements?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment is original to the LGI codebase, not my fork. GObject signal documentation is a bit fuzzy with detailed signals, but one example I found from Stackoverflow is the notify signal. GObject::notify is emitted any time any property is changed, but the detailed GObject::notify::<property> signal is only emitted when that given property is changed. Detailed signals are handled differently to non-detailed signals in the GObject override, but I figure behavior should be identical for both, so I chose to implement the memory management utility for both.

I realize now I misformatted my detailed notification string - info.name .. "." .. detail should be info.name .. "::" .. detail

Copy link
Author

@oezingle oezingle Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some more research, looks like the detailed signal implementation LGI provides is less polished than the other signal handlers.

This code, inserted into demo.lua in my fork will work, but detail names are not transformed like their parent signal name. Also, the API for details here only seems to support assignment, which is a letdown as developers would either be able to disconnect detailed signals, or assign multiple, but never both, unless the LGI detailed signal API is rewritten. I should mention it's entirely undocumented.

button_2.on_notify["has-focus"] = function ()
    print("button 2 focus state changed")
end

In my opinion, the pad metatable should allow __index and __newindex events, so that :connect() and :disconnect() can be exposed for detailed signals, along with variadic assignment. If I have time I might throw an implementation together over the weekend.

connect_signal(object, gtype, info.name, Closure((...), info))
self._signal_handler[name] = connect_signal(object, gtype, name, Closure((...), info))
else
-- Reading yields table with signal operations.
local mt = {}
Expand All @@ -330,6 +341,11 @@ function Object:_access_signal(object, info, ...)
return connect_signal(object, gtype, info.name,
Closure(target, info), detail, after)
end

function pad:disconnect(handler_id)
return signal_handler_disconnect(object, handler_id)
end

function pad:emit(...)
return emit_signal(object, gtype, info, nil, ...)
end
Expand All @@ -345,8 +361,27 @@ function Object:_access_signal(object, info, ...)
return emit_signal(object, gtype, info, detail, ...)
end
function mt:__newindex(detail, target)
connect_signal(object, gtype, info.name, Closure(target, info),
detail)
local name_with_detail = name .. "::" .. detail
local existing_handler = self._signal_handler[name_with_detail]
if existing_handler then
signal_handler_disconnect(object, existing_handler)
end

self._signal_handler[name_with_detail] = connect_signal(object, gtype, name, Closure(target, info),
detail)
end

function mt.__index(_, detail)
local sub_pad = {}

function sub_pad:connect(target, after)
return connect_signal(object, gtype, name,
Closure(target, info), detail, after)
end

sub_pad.disconnect = pad.disconnect

return sub_pad
end
end

Expand Down