vitrine/wmaker
2
0
Fork 2
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Rewrite WUtils notification.c in Rust #7

Merged
trurl merged 10 commits from trurl/wmaker:refactor/wutil-rs-notification into refactor/wutil-rs 2025-12-15 12:00:30 -05:00
Conversation 6 Commits 10 Files Changed 14 +412 -588
trurl commented 2025-11-14 00:51:39 -05:00
Owner
Copy Link

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
Member
Copy Link

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

Is it just me, or is the formatting here kind of off?
trurl commented 2025-12-08 13:30:12 -05:00
Author
Owner
Copy Link

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.

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
Member
Copy Link

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.

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
Author
Owner
Copy Link

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
Member
Copy Link

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

"...must be guaranteed by code..." (missing the word "by").
trurl commented 2025-12-08 13:07:07 -05:00
Author
Owner
Copy Link

Done. Thanks!

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
Member
Copy Link

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
Author
Owner
Copy Link

Ok, that makes sense.

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
Member
Copy Link

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
Author
Owner
Copy Link

Sure. Done.

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.
Reviewers
No Reviewers
cross
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
cross trurl
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vitrine/wmaker#7
Delete Branch "trurl/wmaker:refactor/wutil-rs-notification"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?