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.

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

Conversation

oezingle
Copy link

@oezingle oezingle commented Feb 13, 2025

This is a pull request to mitigate the issues I found in this issue.

demo.lua is a simple Gtk demo, modified from the issue - this fork fixes the 2 issues I found with signal handling. Signal "pads" now provide the :disconnect(handler_id) method, and signal assignment now automatically manages memory.

However, this fork shouldn't be merged without modification. I was unable to figure out how to properly register a field to a GObject. As such, lines 324-326 of lgi/override/GObject-Object.lua are some evil code:

-- TODO: someone who understands the LGI codebase much better than myself no doubt knows where to put a definition similar to this one
-- create a table of signal handlers LGI automatically memory manages
self._signal_handlers = self._signal_handlers or {}

Please advise as to how I can properly register a field such as this.

 - Additional commit to fix whitespace
@psychon
Copy link
Collaborator

psychon commented Feb 13, 2025

someone who understands the LGI codebase much better than myself

I'm sorry, but we do not have anyone around like that. The original author went missing and ever since people in the same position as you tried not to break more stuff than they fixed. Sorry.


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.

if existing_handler then
signal_handler_disconnect(object, existing_handler)
end

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

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.

@oezingle
Copy link
Author

oezingle commented Feb 13, 2025

Still looking to make a few improvements before any chance of a merge, but I've implemented :connect() and :disconnect() for GObject.on_<signal>.<detail>, and I believe I've found the right location to create the _signal_handlers table

Edit: nope! Going to try again. self._signal_handler refers to all GObjects of that class, not a given instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants