Object: Fix ToggleNotify called after callback is disposed #1128
Conversation
badcel
force-pushed
the
fix-1125
branch
3 times, most recently
from
afbfb04 to
9036145
Compare
|
@ravener can you confirm that this PR fixes the bug #1125 for you? @cameronwhite This fix updates code introduced due to a reported bug by a pinta user. Any thoughts on this? I think it should now work more reliable than before. I'm not sure if the dictionary access is "safe enough" though or if there is perhaps some concurrent dictionary needed. As those versions would probably be slower I think I will keep it like it is for now if there are no objections. |
|
I'll need to build Pinta against this change and test it out, but from reading through the code I had a couple questions:
|
|
@cameronwhite thanks for your answer. I misremembered the original pinta bug. I think both of your points are valid. I will update this PR accordingly. |
|
I'd love to test it, but I'm not sure how, is there an easy way to pull a PR and have it as a dependency? Not so used to working with dotnet sorry. |
|
@ravener you would basically do: $ git clone --recursive https://github.com/gircore/gir.core.git
$ cd gir.core/src
$ git checkout fix-1125
$ dotnet fsi GenerateLibs.fsx
$ dotnet build GirCore.Libs.slnfThis is mostly identical to the build instructions in the readme except that you checkout the branch of this PR instead of the Afterwards you have to use the build binaries in your project. The binaries are for example located in the |
|
I updated the It revealed that the problem was the file helper instance. You can simply copy and paste the code into the same file and should be able to test your issue without the need to do more than just coping over the code. using System;
using System.IO;
using Gtk;
var application = Adw.Application.New("org.gir.core", Gio.ApplicationFlags.FlagsNone);
application.OnActivate += (sender, args) =>
{
var w = new BugWindow((Application)sender);
w.Show();
};
return application.RunWithSynchronizationContext(null);
public class BugWindow : Adw.ApplicationWindow
{
private readonly Gtk.Button _button;
private readonly Gtk.DropTarget _drop;
private int count;
public BugWindow(Gtk.Application app)
{
Application = app;
var box = Gtk.Box.New(Gtk.Orientation.Vertical, 6);
box.Append(Adw.HeaderBar.New());
_button = Gtk.Button.NewWithLabel("File 1");
_button.OnClicked += OnFileSelect;
box.Append(_button);
SetContent(box);
_drop = Gtk.DropTarget.New(Gio.FileHelper.GetGType(), Gdk.DragAction.Copy);
_drop.OnDrop += OnDrop;
_button.AddController(_drop);
}
private bool OnDrop(Gtk.DropTarget drop, Gtk.DropTarget.DropSignalArgs e)
{
System.GC.Collect(); // Force gc for stress testing.
Console.WriteLine("dropped!");
Console.WriteLine("FilePointer: " + e.Value.GetObject()!.Handle);
var file = new Gio.FileHelper(e.Value.GetObject()!.Handle, false);
//var path = file.GetPath();
//Console.WriteLine(path);
//if (!File.Exists(path))
// return false;
Console.WriteLine("File exists.");
if (drop != _drop)
return false;
_button.SetLabel("Bla" + count++);
//_button.SetLabel(file.GetBasename()!);
Console.WriteLine("Set label");
return true;
}
private void OnFileSelect(Gtk.Button sender, EventArgs args)
{
Console.WriteLine("here we are.");
var chooser = Gtk.FileChooserNative.New("Open stuff", this, Gtk.FileChooserAction.Open, null, null);
chooser.Show();
}
}Please also confirm that the issure is present if you run the test code on the main branch. Checkout the code via After Checkout you do not need to generate the project again as no generated files are altered. Just start the Window project again and see that the error is there to double check that my sample code creates the same issue like your sample code on your machine. |
It can happen that calling "RemoveToggleRef" leads to a delayed invocation of it's toggle notify callback. If "RemoveToggleRef" is called the garbage collector is free to free the ToggleRef in which case the managed "ToggleNotify" callback is freed, too. This change uses an "UnmanagedCallersOnly" function which is out of scope of the GC and can always be called. In case it is called after the ToggleRef was freed it just does nothing. Fixes: #1125
|
Unfortunately I'm having issues building locally
If I just continue to change the adwaita sample and run I'm on Debian 12, maybe outdated system libraries are a potential reason? Recall that the original bug was not reproducible on my debian 12 machine anyway, it only happened inside flatpak runtime, so newer gnome runtimes probably introduced it. I'm still not comfortable with flatpak build tools or I would've tried to build this inside flatpak in case the build step requires newer system libraries (can you confirm this? or do you expect it to build on older distros regardless?) |
|
I can at least do the second part, but without building from source, continuing with the latest nuget version like before, I can confirm your sample is still the same issue as my original code. And again, works fine locally on Debian 12 but crashes on flatpak (on the second file drop) |
|
@ravener if you have dotnet 8 installed I would expect it to generate code on any system. To build you would need dotnet 6,7 and 8. The tool simply reads the Gir xml files which are part of the repository and outputs some textfiles which happen to be code: nothing system dependent here. I would even expect the sample to work on your Debian machine as the used adwaita API in the sample program should be "old enough". Yesterday I explicitly tested the commands I gave you in a fresh folder and it worked for me. I need to do some more investigation. On the other hand I expect the problem to be solved with this as you confirmed the problem occurs with my demo code on your side, too. |
|
If you want to keep Debian but want to create GTK apps which you distribute as a flatpak I would recommend using tools like distrobox and toolbx. With those you can get development environments which are a lot closer to the flatpak GNOME runtimes than Debian if you choose fedora for example. The GirCore nuget packages are build against the flatpak runtime. If you want to create apps for Debian you would probably be better served if you generate the binding binaries locally and use the Debian Gir files. Then the generated C# API matches the C api available in Debian and not in the flatpak runtime. |
|
Well I did try to build it locally, doesn't that target debian then? For my usual development, I have a nice VSCode + Flatpak workflow setup that allows me to build the app against the Flatpak runtime which works well so far. Anyway I'm not going to waste your time, I'll just wait for prebuilt binaries before I retest this since building proved to be harder than I thought, sorry for being unhelpful. |
|
You are not unhelpful. If you build the project according to the build instructions the binaries target the flatpak runtime because the gir files are part of the repository. The system Gir files are not used by default. In the beginning of the project I tried different sources of Gir files. Different problems occurred depending on which Gir files were used. As flatpaks are distro agnostic I settled with the flatpak runtime Gir files. Perhaps there could be some command line option to make it easier to use system Gir files. I would be very interested in your workflow to build against the flatpak runtime. Do you have some repository which shows how you do it? |
This is my first app so far, but since I'm close to finishing it and was gonna open source it soon anyway, I decided to make the repo now at https://github.com/ravener/Oto I was kind of inspired by Denaro but they used Cake scripts which I didn't feel like adding to my project so I opted to write a simple shell script that just copies the relevant files which gets the job done for now but I may need to improve a few things. Then I rely on flatpak-vscode extension, which automatically picks up the flatpak manifest and adds a 'Build & Run' button (shortcut CTRL + Alt + B) but you may as well manually use the flatpak builder tool if you are familiar with it, I never touched that myself yet, the extension is just a neat layer on top of it making vscode as easy as using something like gnome builder. In the meantime I can also just use |
Fixes: #1125