Skip to content

Progress callback#296

Merged
psychon merged 3 commits intolgi-devs:masterfrom
ntd:progress-callback
May 25, 2022
Merged

Progress callback#296
psychon merged 3 commits intolgi-devs:masterfrom
ntd:progress-callback

Conversation

@ntd
Copy link
Contributor

@ntd ntd commented May 23, 2022

This touches a quite delicate part of LGI, so it probably requires much more eyeballs than only mine.

ntd added 2 commits May 23, 2022 14:25
Avoid multiple indirections by storing the Param pointer for the data
argument in a temporary variable.
@pavouk
Copy link
Collaborator

pavouk commented May 24, 2022

LGTM. Thanks!

Data bound to a callback must be flagged as "internal". The problem is
also the callback itself is marked as such because of this issue:

https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/430

Avoid the problem by checking the scope: if it is invalid, it is likely
real data (and not a callback), hence it can be marked.

Fixes lgi-devs#241, lgi-devs#262 and lgi-devs#285.
@ntd ntd force-pushed the progress-callback branch from 5111efd to 7f8fc55 Compare May 24, 2022 07:44
@psychon
Copy link
Collaborator

psychon commented May 25, 2022

You got a "LGTM" from pavouk, produced a PR with nice, small, self-contained commits, you are touching code that I do not understand. I want you on the team!

Merging based on the "LGTM" from someone who definitely knows this stuff and since this seems like you know what you are doing. Thanks a lot!

@psychon psychon merged commit 3f47eb5 into lgi-devs:master May 25, 2022
@ntd
Copy link
Contributor Author

ntd commented May 25, 2022

...
I want you on the team!

Thank you: lately I'm using LGI sparingly but I would be happy to help where I can. Not sure what to do though...

Merging based on the "LGTM" from someone who definitely knows this stuff and since this seems like you know what you are doing.

"seems" is the keyword here 😄

I did not grasp that much: I just got the intent of the internal flag from the comments and checked this was not reflected by the code. Kudos to @pavouk for his comments: they are really helpful.

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.

3 participants