Refactor internal closure handling and several API changes #706

diamondburned · 2020-12-29T05:54:07Z

Precaution

I would recommend merging this PR later, perhaps when a new major (0.x for v0?) gotk3 release is planned, since these breaking changes are quite significant. Perhaps it's worth maintaining a second branch (regularly rebased with master) for people to slowly migrate over.

The aforementioned requirement of needing a first argument when using Connect has been changed to no longer requiring it, as not all scenarios require such a case, such as win.Connect("destroy", gtk.MainQuit), and they do get annoying quickly over time.

This pull request introduces proper GDestroyNotify callbacks as well as cleaner closure and callback global registries to reduce (if not eliminate) memory leaks.

This pull request also introduces several breaking changes. Below lists those changes in undefined order:

Connect, IdleAdd, TimeoutAdd and others in GLib no longer return an error.
Any error with the function argument (type interface{}) is assumed to be programmer error and will therefore panic. GLib functions in C do not have nullable returns either, so the usual nilPtrErr does not apply, therefore all error returns are omitted.
Old Func types for callback setters no longer force a variadic list of arguments in their parameters, as Go's closure abilities allow referencing values from outside.

This pull request also adds a small stack trace that occurs when any callback cannot be handled gracefully. This helps find bugs easier:

panic: closure error: callback should have the object parameter to avoid circular references

Closure added at:
	main.main.func2.1 at /tmp/img-leak/main.go:89
	runtime.goexit at /nix/store/4glm9a63mqk5v9cdmv2nzp1j92fw4102-go-1.15.6/share/go/src/runtime/asm_amd64.s:1374

goroutine 6 [running]:
github.com/gotk3/gotk3/internal/closure.FuncStack.Panicf(0x5c13e0, 0x660a38, 0x13, 0xc0000227e0, 0x2, 0x3, 0x65f801, 0x46, 0x0, 0x0, ...)
	/home/diamond/Scripts/gotk3/internal/closure/funcstack.go:73 +0xabf
github.com/gotk3/gotk3/glib.(*Object).connectClosure(0xc000010088, 0xc00000e100, 0x656783, 0x1, 0x5c13e0, 0x660a38, 0x0)
	/home/diamond/Scripts/gotk3/glib/connect.go:86 +0x405

This pull request resolves #685 but might break #507.

Below lists several tidbits that were convenient for me to add during the refactor:

(*gtk.TreeView).SetSearchEqualFunc
GLib utility functions e.g. FormatSize{,Full}, SpacedPrimesClosest, ...
GApplication and GDBusConnection (low priority, can be a follow-up PR)

gdk/gdk_since_3_10.go

gdk/pixbuf.go

diamondburned · 2021-01-05T07:47:27Z

This PR to be rebased after PR #712.

hfmrow · 2021-01-05T13:09:29Z

Hi,
I'm currently testing your modifications (taken from your repository) rebased after #712 as mentioned.
I'm lucky, i use my own libraries that wrap lot of gotk3 code this avoid big changes in my softwares sources code, so i've just to modify them to complies with your modifications and that look working well, i globally noticed some performance falling (very light), and little more cpu usage overall, this is not really bad when it's for a better stability and reliable source code.
I will back to you if i have other things to comment.
Thanks for this hard work. I'm impatient to for final release.

diamondburned · 2021-01-05T23:36:56Z

Hi,
I'm currently testing your modifications (taken from your repository) rebased after #712 as mentioned.
I'm lucky, i use my own libraries that wrap lot of gotk3 code this avoid big changes in my softwares sources code, so i've just to modify them to complies with your modifications and that look working well, i globally noticed some performance falling (very light), and little more cpu usage overall, this is not really bad when it's for a better stability and reliable source code.
I will back to you if i have other things to comment.
Thanks for this hard work. I'm impatient to for final release.

I'm actually interested in this performance drop you're talking about. Can you try and pprof your application and see what's wrong?

hfmrow · 2021-01-06T01:21:46Z

Hi,
I'm currently testing your modifications (taken from your repository) rebased after #712 as mentioned.
I'm lucky, i use my own libraries that wrap lot of gotk3 code this avoid big changes in my softwares sources code, so i've just to modify them to complies with your modifications and that look working well, i globally noticed some performance falling (very light), and little more cpu usage overall, this is not really bad when it's for a better stability and reliable source code.
I will back to you if i have other things to comment.
Thanks for this hard work. I'm impatient to for final release.

I'm actually interested in this performance drop you're talking about. Can you try and pprof your application and see what's wrong?

Already done with pprof but not enough knowledge to understand all and getting useful conclusions ...
I will prepare an example for you, just need some time to doing it.
When it ready, i give you the link with all my tests and all you need to reproduce.
Then you can use pprof as your convenience and your knowledge ^^

hfmrow · 2021-01-06T04:08:57Z

@diamondburned
My own tests, with vendor dir if needed (does not include gotk3 of course)

hfmrow

Good thing, this will save a lot of code rewriting.

diamondburned · 2021-02-13T05:27:27Z

Merge conflicts addressed.

diamondburned · 2021-02-13T09:38:01Z

~~I'm actually not sure if this branch tree is safe to merge. I will have to double check this.~~

I've fixed the branch up.

rubiojr · 2021-02-14T17:26:53Z

@diamondburned wondering if we could add some examples to the documentation or the OP here showing how to change existing code to adapt to the changes proposed in this PR.

I've got a medium sized app that will probably be affected by this, so happy to be the guinea pig and provide some feedback and code changes required for my app to compile/run again.

Does that sound like a reasonable plan to you?

rubiojr · 2021-02-14T17:44:49Z

Missed the updated OP that now states:

The aforementioned requirement of needing a first argument when using Connect has been changed to no longer requiring it, as not all scenarios require such a case, such as win.Connect("destroy", gtk.MainQuit), and they do get annoying quickly over time.

Tried to build my app against this branch and had to change a couple of lines, nothing too terrible:

diff --git a/internal/ui/inprogresslist/inprogresslist.go b/internal/ui/inprogresslist/inprogresslist.go
index d05f3b4..e25a0c8 100644
--- a/internal/ui/inprogresslist/inprogresslist.go
+++ b/internal/ui/inprogresslist/inprogresslist.go
@@ -57,7 +57,7 @@ func (i *InProgressList) isShown(tree *gtk.TreeView) {
 
 func (i *InProgressList) updateFileList() {
 	//TODO: use leveldb to speed things up without walking the filesystem
-	i.listBox.BindModel(nil, nil, nil)
+	i.listBox.BindModel(nil, nil)
 
 	for _, doc := range downloader.Instance().DownloadsInProgress() {
 		i.addFileRow(doc.Name)
diff --git a/internal/ui/tagger/tagger.go b/internal/ui/tagger/tagger.go
index 025ace2..6041e4a 100644
--- a/internal/ui/tagger/tagger.go
+++ b/internal/ui/tagger/tagger.go
@@ -98,7 +98,7 @@ func (t *Tagger) removeSelected() {
 
 func (t *Tagger) saveTags() {
 	var tl []tags.Tag
-	t.listStore.ForEach(func(model *gtk.TreeModel, path *gtk.TreePath, iter *gtk.TreeIter, userdata ...interface{}) bool {
+	t.listStore.ForEach(func(model *gtk.TreeModel, path *gtk.TreePath, iter *gtk.TreeIter) bool {
 		value, _ := t.listStore.GetValue(iter, 0)
 		tname, _ := value.GetString()
 		tag := tags.Tag{Name: tname}

diamondburned · 2021-02-14T18:52:45Z

@diamondburned wondering if we could add some examples to the documentation or the OP here showing how to change existing code to adapt to the changes proposed in this PR.

I've got a medium sized app that will probably be affected by this, so happy to be the guinea pig and provide some feedback and code changes required for my app to compile/run again.

Does that sound like a reasonable plan to you?

Overall, most of the breaking changes are Connect, IdleAdd and such no longer returning an error, and callback setters no longer take a function with a variadic ...interface{} argument.

If you could point out some places where I could notify of such changes, then I'm willing to PR in some changes. Perhaps I could start with examples elsewhere...

rubiojr · 2021-02-14T19:46:29Z

If you could point out some places where I could notify of such changes, then I'm willing to PR in some changes

Good point, maybe the release notes when bumped to 0.6? perhaps @andre-hub already planned for that.

This commit introduces proper GDestroyNotify callbacks as well as cleaner closure and callback global registries to reduce (if not eliminate) memory leaks. This commit also introduces several breaking changes. Below lists those changes in undefined order: Connect, IdleAdd, TimeoutAdd and others in GLib no longer return an error. Any error with the function argument (type interface{}) is assumed to be programmer error and will therefore panic. GLib functions in C do not have nullable returns either, so the usual nilPtrErr does not apply, therefore all error returns are omitted. All signal callbacks now also require the first argument to be the appropriate callback type to prevent cyclical references leading to memory leaks. Old Func types for callback setters no longer force a variadic list of arguments in their parameters, as Go's closure abilities allow referencing values from outside.

andre-hub · 2021-03-05T11:49:39Z

If you could point out some places where I could notify of such changes, then I'm willing to PR in some changes

Good point, maybe the release notes when bumped to 0.6? perhaps @andre-hub already planned for that.

now! :-)

firelizzard18 · 2021-07-01T23:54:01Z

glib/connect.go

+//
+// This constant can be changed by using go.mod's replace directive for
+// debugging purposes.
+const ClosureCheckReceiver = false


@diamondburned Can you explain how to use a go.mod replace directive to change this constant?

You can fork the repository or clone it down, then use the replace directive inside go.mod, like so:

# cloned replace github.com/gotk3/gotk3 => ../path/to/gotk3 # forked replace github.com/gotk3/gotk3 => github.com/username/gotk3 commithash...

diamondburned commented Dec 29, 2020

View reviewed changes

gdk/gdk_since_3_10.go Show resolved Hide resolved

diamondburned commented Dec 29, 2020

View reviewed changes

gdk/pixbuf.go Show resolved Hide resolved

diamondburned marked this pull request as ready for review December 29, 2020 09:28

diamondburned marked this pull request as draft December 29, 2020 09:42

andre-hub added this to the V0.6.0 milestone Dec 29, 2020

andre-hub added next version work in progress breaking changes labels Dec 29, 2020

andre-hub modified the milestones: V0.6.0, V0.7.0 Dec 29, 2020

diamondburned marked this pull request as ready for review January 2, 2021 22:49

diamondburned mentioned this pull request Jan 2, 2021

Inheritance issues while building diamondburned/handy#3

Open

diamondburned marked this pull request as draft January 5, 2021 07:47

diamondburned force-pushed the stalone/callback-refactor branch from 1f7dd22 to f687367 Compare January 5, 2021 08:18

diamondburned marked this pull request as ready for review January 11, 2021 01:55

hfmrow approved these changes Jan 11, 2021

View reviewed changes

andre-hub removed the work in progress label Jan 25, 2021

andre-hub added merge conflicts and removed breaking changes next version labels Feb 3, 2021

diamondburned force-pushed the stalone/callback-refactor branch from 77e9a82 to b564fb8 Compare February 13, 2021 18:49

diamondburned added 6 commits February 23, 2021 15:20

Fixed GDestroyNotify export name; added TreeView.SetSearchEqualFunc

d5ed22a

Added closure recover, allow interface for first receiver

7bd7125

Allow optionally disabling the receiver type check

94491fa

Omitted erroneous RegisterSignal check

0b022cf

ClosureCheckReceiver default to false

0ce0ad6

diamondburned force-pushed the stalone/callback-refactor branch from b564fb8 to 0ce0ad6 Compare February 23, 2021 23:21

andre-hub added breaking changes and removed merge conflicts labels Mar 5, 2021

andre-hub merged commit 63e33a1 into gotk3:master Mar 5, 2021

diamondburned mentioned this pull request Mar 10, 2021

Fix GLib compile from merge conflicts #750

Merged

firelizzard18 reviewed Jul 1, 2021

View reviewed changes

the-ress mentioned this pull request Jul 22, 2023

Switch to go mod and latest go, improve build Z-Bolt/OctoScreen#346

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor internal closure handling and several API changes #706

Refactor internal closure handling and several API changes #706

diamondburned commented Dec 29, 2020 •

edited

Loading

diamondburned commented Jan 5, 2021

hfmrow commented Jan 5, 2021 •

edited

Loading

diamondburned commented Jan 5, 2021

hfmrow commented Jan 6, 2021

hfmrow commented Jan 6, 2021

hfmrow left a comment

diamondburned commented Feb 13, 2021

diamondburned commented Feb 13, 2021 •

edited

Loading

rubiojr commented Feb 14, 2021 •

edited

Loading

rubiojr commented Feb 14, 2021

diamondburned commented Feb 14, 2021

rubiojr commented Feb 14, 2021 •

edited

Loading

andre-hub commented Mar 5, 2021

firelizzard18 Jul 1, 2021

diamondburned Jul 2, 2021

Refactor internal closure handling and several API changes #706

Refactor internal closure handling and several API changes #706

Conversation

diamondburned commented Dec 29, 2020 • edited Loading

Precaution

diamondburned commented Jan 5, 2021

hfmrow commented Jan 5, 2021 • edited Loading

diamondburned commented Jan 5, 2021

hfmrow commented Jan 6, 2021

hfmrow commented Jan 6, 2021

hfmrow left a comment

Choose a reason for hiding this comment

diamondburned commented Feb 13, 2021

diamondburned commented Feb 13, 2021 • edited Loading

rubiojr commented Feb 14, 2021 • edited Loading

rubiojr commented Feb 14, 2021

diamondburned commented Feb 14, 2021

rubiojr commented Feb 14, 2021 • edited Loading

andre-hub commented Mar 5, 2021

firelizzard18 Jul 1, 2021

Choose a reason for hiding this comment

diamondburned Jul 2, 2021

Choose a reason for hiding this comment

diamondburned commented Dec 29, 2020 •

edited

Loading

hfmrow commented Jan 5, 2021 •

edited

Loading

diamondburned commented Feb 13, 2021 •

edited

Loading

rubiojr commented Feb 14, 2021 •

edited

Loading

rubiojr commented Feb 14, 2021 •

edited

Loading