Rewrite WUtils notification.c in Rust #7

2025-11-14T00:51:39-05:00

trurl commented

2025-11-14 00:51:39 -05:00

Most of the work here was in figuring out what exactly the notification code was being used for (and removing things that weren't being used anymore or refactoring to eliminate API surface area). This rewrite may not be 100% identical, in that every possible combination of API invocations in the old code will result in the same behavior if they're made against the new code, but I think it will do the right thing for current use of the WMNotification API.

Most of the work here was in figuring out what exactly the notification code was being used for (and removing things that weren't being used anymore or refactoring to eliminate API surface area). This rewrite may not be 100% identical, in that every possible combination of API invocations in the old code will result in the same behavior if they're made against the new code, but I think it will do the right thing for current use of the `WMNotification` API.

trurl added 6 commits 2025-11-14 00:51:40 -05:00

Make some unused public WMNotification APIs private. 6f4978d633

`WMRetainNotification`, `WMGetDefaultNotificationQueue`,
`WMDequeueNotificationMatching`, `WMEnqueueNotification`, and
`WMEnqueueCoalesceNotification`, are only used in WINGs/notification.c. To
reduce the API surface area that needs to be migrated to Rust, they are being
made private.

Drop unused macro (dead code) that invokes WMCreateNotification. ffcf0a2d53

Drop dead notification queue code. b6025e8f93

This appears to have been used by now-defunct support for network
connections (WMConnection). No live code instantiates a notification queue or
pushes/dequeues notifications from a notification queue. (The global
NotificationCenter in WINGs/notification.c is still in use, so it is not going
anywhere in this commit.)

Ditch the notification function WMRemoveNotificationObserverWithName. e81377a4b4

This reduces the notification API in a way that is helpful for rewriting in
Rust. This function is only used in one place, and the object that is being
deregistered is free'd immediately after WMRemoveNotificationObserverWithName is
called, so it should be safe just to use WMRemoveNotificationObserver (since I
hope it's an error to keep a notification to a free'd object registered).

Remove direct creation and posting of notifications from WINGs. ab0272fe84

We can reduce the WMNotification API surface area further by getting rid of
WMCreateNotification and WMPostNotification. WMNotification remains a
first-class object, but it is no longer possible for client code to create a
notification object directly. Notifications must now be posted through
WMPostNotificationName, which handles notification creation and destruction on
its own.

This will simplify the notification lifecycle and make the Rust rewrite
simpler. (Notifications no longer need to be reference-counted, heap-allocated
objects that might be saved somewhere after they are dispatched.)

WTextField code which reused the WMNotification struct has been modified to take
parameters of the correct type directly, instead of through a WMNotification's
void* data field.

Rewrite WINGs/notification.c in Rust. 050daba36d

trurl requested review from cross 2025-11-14 00:51:51 -05:00

cross reviewed 2025-12-02 11:20:00 -05:00

WINGs/wtextfield.c Outdated

						
				@@ -907,3 +901,3 @@

						paintTextField(tPtr);

						NOTIFY(tPtr, didBeginEditing, WMTextDidBeginEditingNotification, NULL);

				                if (tPtr->delegate && tPtr->delegate->didBeginEditing) {

cross commented

2025-12-02 11:20:00 -05:00

Is it just me, or is the formatting here kind of off?

trurl commented

2025-12-08 13:30:12 -05:00

It's not you. This was a tabs/spaces thing. I think I've fixed it in the .c files I touched in this PR.

trurl marked this conversation as resolved

cross reviewed 2025-12-02 13:48:02 -05:00

WINGs/wtextfield.c Outdated

						
				@@ -906,7 +900,10 @@ static void handleEvents(XEvent * event, void *data)

						paintTextField(tPtr);

						NOTIFY(tPtr, didBeginEditing, WMTextDidBeginEditingNotification, NULL);

cross commented

2025-12-02 13:48:02 -05:00

I'm mildly bummed out that this went away, because it's decently concise, which is nice.

But if one digs into the macro, it seems like it does three things:

Handle the lifecycle of a notification (create/allocate and then "release").
If the delegation machinery is set up, invoke it.
Post the notification.

These are all conceptually separate steps, but what kind of bums me out is that (2) has now inlined everywhere.

One imagines, perhaps, a better design might abstract the delegation machinery by providing a thunk that polls for delegation, instead of a macro expanding into a bespoke condition on a specific structure member.

I'm mildly bummed out that this went away, because it's decently concise, which is nice. But if one digs into the macro, it seems like it does three things: 1. Handle the lifecycle of a notification (create/allocate and then "release"). 2. If the delegation machinery is set up, invoke it. 3. Post the notification. These are all conceptually separate steps, but what kind of bums me out is that (2) has now inlined everywhere. One imagines, perhaps, a better design might abstract the delegation machinery by providing a thunk that polls for delegation, instead of a macro expanding into a bespoke condition on a specific structure member.

trurl commented

2025-12-08 13:37:58 -05:00

It is not entirely clear to me if this delegation machinery is going to stick around, so I don't want to spend time on making it nicer to use right now. (As far as I can tell, it is partially used by WPrefs.app, and it could just as easily have added a notification listener instead of using the WMTextFieldDelegate interface.)

It is not entirely clear to me if this delegation machinery is going to stick around, so I don't want to spend time on making it nicer to use right now. (As far as I can tell, it is partially used by WPrefs.app, and it could just as easily have added a notification listener instead of using the `WMTextFieldDelegate` interface.)

trurl marked this conversation as resolved

cross reviewed 2025-12-02 13:49:00 -05:00

wutil-rs/src/notification.rs Outdated

						
				@@ -0,0 +49,4 @@

				/// Wraps a type-erased pointer (which it does not own) and marks it as `Send`.

				///

				/// The `Send`-ability of the wrapped pointer must be guaranteed code that

cross commented

2025-12-02 13:49:00 -05:00

"...must be guaranteed by code..." (missing the word "by").

trurl commented

2025-12-08 13:07:07 -05:00

Done. Thanks!

trurl marked this conversation as resolved

cross reviewed 2025-12-02 13:55:16 -05:00

wutil-rs/src/notification.rs Outdated

						
				@@ -0,0 +79,4 @@

				#[derive(Default)]

				pub struct NotificationCenter {

				    /// Notification subscriptions that match on name and source.

				    exact: HashMap<(&'static CStr, Sendable), Vec<(Option<Sendable>, Action)>>,

cross commented

2025-12-02 13:55:16 -05:00

It strikes me that if you used BTreeMap here instead of HashMap, you could make the constructor for NotificationCenter by pub const fn new() and do away with the OnceLock bits in with_global_default, instead using a static Mutex<NotificationCenter>.

It strikes me that if you used `BTreeMap` here instead of `HashMap`, you could make the constructor for `NotificationCenter` by `pub const fn new()` and do away with the `OnceLock` bits in `with_global_default`, instead using a static `Mutex<NotificationCenter>`.

trurl commented

2025-12-08 13:27:02 -05:00

Ok, that makes sense.

trurl marked this conversation as resolved

cross reviewed 2025-12-02 13:56:37 -05:00

wutil-rs/src/notification.rs Outdated

						
				@@ -0,0 +119,4 @@

				        observer: Option<Sendable>,

				        action: Action,

				    ) {

				        match self.exact.entry((name, source)) {

cross commented

2025-12-02 13:56:37 -05:00

The actions in the match here are repeated verbatim in all cases (I think). Perhaps delegate to a small helper function that implements just that logic?

The actions in the `match` here are repeated verbatim in all cases (I think). Perhaps delegate to a small helper function that implements just that logic?

trurl commented

2025-12-08 13:14:44 -05:00

Sure. Done.

trurl marked this conversation as resolved

trurl force-pushed refactor/wutil-rs-notification from 050daba36d to 0893be1cea

2025-12-08 13:26:23 -05:00

Compare

trurl added 1 commit 2025-12-08 13:32:11 -05:00

Indent with tabs, as is standard for this file. d3dac752cc

cross approved these changes 2025-12-12 07:17:26 -05:00

trurl merged commit d3dac752cc into refactor/wutil-rs

2025-12-15 12:00:30 -05:00

trurl deleted branch refactor/wutil-rs-notification

2025-12-15 12:00:47 -05:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vitrine/wmaker#7

Allow edits from maintainers