Skip to content

Some Vala things #1429

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Some Vala things #1429

wants to merge 2 commits into from

Conversation

bugaevc
Copy link
Contributor

@bugaevc bugaevc commented May 20, 2025

I'm surprised about the g_type_ensure thing. I remember having to add one of those lines myself, so now I got the idea that I should teach Vala to just do it automatically, and turns out it already does, and everything seems to just work without them indeed.

@GeopJr
Copy link
Owner

GeopJr commented May 20, 2025

it already does

is this something done in a recent release? Because ~half year ago it was still a problem #1170

@GeopJr
Copy link
Owner

GeopJr commented May 20, 2025

note that I couldn't reliably reproduce the issues #1170 fixed (mostly freezes and crashes when using custom widgets in builder files - specifically the custom emoji picker in the profile editor), but had more than 1 reports about it and it was targeted down after debugging their installations; even if they were flatpaks 🤷

If it's done in a recent release, we might have to check in meson and ensure them behind a flag

@bugaevc
Copy link
Contributor Author

bugaevc commented May 20, 2025

No, it's been doing that since https://gitlab.gnome.org/GNOME/vala/-/commit/8f9979da7a75cb91a46e47e61bf35a41c8ff8083

Maybe there is a Heisenbug in Vala (that I could fix)? How do I reproduce that?

@GeopJr
Copy link
Owner

GeopJr commented May 20, 2025

How do I reproduce that?

If I remember correctly, the common reproducer was opening the profile editor (open your profile => pencil headerbar button) without constructing a custom emoji picker prior (e.g. without opening the composer)

@GeopJr
Copy link
Owner

GeopJr commented May 20, 2025

For completeness, if Vala is supposed to generate g_type_ensures in the same file, it just doesn't seem to do so:

left is with the type ensure commented out and that's pretty much the only change in the generated c code
image

@bugaevc
Copy link
Contributor Author

bugaevc commented May 20, 2025

It does so in instance init (below in your screenshot), right before gtk_widget_init_template

@GeopJr
Copy link
Owner

GeopJr commented May 20, 2025

It doesn't seem to do so in Vala 0.56.18, the following is identical between the two files:

image

@GeopJr
Copy link
Owner

GeopJr commented May 20, 2025

@bugaevc
Copy link
Contributor Author

bugaevc commented May 20, 2025

Aha, I can reproduce that. It's indeed missing an ensure for TubaWidgetsCustomEmojiChooser. I'll look into it.

(By the way, a thought for another day: it would make more sense to me if in C, these classes only had Tuba as their prefix, not TubaWidgets or TubaDialogs or TubaViews, so the emoji chooser would be TubaCustomEmojiChooser. You can accomplish that by setting [CCode (cprefix = "Tuba", lower_case_cprefix = "tuba_")] on namespace Tuba.Widgets etc.)

@GeopJr
Copy link
Owner

GeopJr commented May 20, 2025

Not opposed to it as long as it doesn't create conflicts (Tuba.API.Account vs Tuba.Widgets.Account); we had enough of them in vala land (GLib.DateTime vs Tuba.DateTime)

@bugaevc
Copy link
Contributor Author

bugaevc commented May 23, 2025

Okay, so I have a Vala branch that fixes this. Other than Tuba.Dialogs.ProfileEdit / g_type_ensure (TUBA_WIDGETS_TYPE_CUSTOM_EMOJI_CHOOSER);, the only type affected seems to be Tuba.Dialogs.Preferences, which now has:

+	g_type_ensure (TUBA_TYPE_COLOR_SCHEME_LIST_MODEL);
+	g_type_ensure (TUBA_TYPE_COLOR_SCHEME_LIST_ITEM);
+	g_type_ensure (TUBA_INSTANCE_ACCOUNT_TYPE_VISIBILITY);
+	g_type_ensure (TUBA_UTILS_LOCALES_TYPE_LOCALE);
+	g_type_ensure (TUBA_INSTANCE_ACCOUNT_TYPE_STATUS_CONTENT_TYPE);

Does that match your expectations, or do you know of more instances of missing g_type_ensures?

P.S. https://floss.social/@bugaevc/114556715462967488, any idea?

@GeopJr
Copy link
Owner

GeopJr commented May 23, 2025

Okay, so I have a Vala branch that fixes this. Other than Tuba.Dialogs.ProfileEdit / g_type_ensure (TUBA_WIDGETS_TYPE_CUSTOM_EMOJI_CHOOSER);, the only type affected seems to be Tuba.Dialogs.Preferences, which now has:

Does that match your expectations, or do you know of more instances of missing g_type_ensures?

Awesome, thanks! Yes I think it's alright, the missing g_type_ensure on the Preferences dialog were never a problem so I didn't include them (all of those are used on startup in other parts).

P.S. https://floss.social/@bugaevc/114556715462967488, any idea?

I think the main problem is that we do keep a reference to the main window under app, you can set it to null on shutdown and it should hit them:

diff --git a/src/Application.vala b/src/Application.vala
index d8ba5539..50add463 100644
--- a/src/Application.vala
+++ b/src/Application.vala
@@ -391,6 +391,7 @@ namespace Tuba {
                        #endif
                        network.flush_cache ();
 
+                       app.main_window = null;
                        base.shutdown ();
                }
 

@GeopJr
Copy link
Owner

GeopJr commented May 23, 2025

Maybe I should write a more graceful shutdown process...

@bugaevc
Copy link
Contributor Author

bugaevc commented May 24, 2025

I did this instead:

diff --git a/src/Application.vala b/src/Application.vala
index d8ba5539..8e2dc82e 100644
--- a/src/Application.vala
+++ b/src/Application.vala
@@ -32,7 +32,7 @@ namespace Tuba {
 	public class Application : Adw.Application {
 
 		public GLib.ProxyResolver? proxy { get; set; default=null; }
-		public Dialogs.MainWindow? main_window { get; set; }
+		public unowned Dialogs.MainWindow? main_window { get; set; }
 		public Dialogs.NewAccount? add_account_window { get; set; }
 		public bool is_mobile { get; set; default=false; }
 		public bool is_online { get; private set; default=true; }
diff --git a/src/Dialogs/MainWindow.vala b/src/Dialogs/MainWindow.vala
index bf8e96c6..1ad4bbd6 100644
--- a/src/Dialogs/MainWindow.vala
+++ b/src/Dialogs/MainWindow.vala
@@ -261,4 +261,11 @@ public class Tuba.Dialogs.MainWindow: Adw.ApplicationWindow, Saveable {
 
 		last_view = view;
 	}
+
+	public override void dispose () {
+		if (app.main_window == this) {
+			app.main_window = null;
+		}
+		base.dispose ();
+	}
 }

Could've also used WeakRef I guess. But it works, thanks! (And indeed immediately runs into https://gitlab.gnome.org/GNOME/gtk/-/issues/5834 w/ my Vala patch to insert gtk_widget_dispose_template calls.)

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