Conversation
NUnit Tests 1 files ± 0 1 suites ±0 11m 43s ⏱️ -5s Results for commit 5fe81b2. ± Comparison against base commit 53d5dbb. This pull request removes 2 and adds 43 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the “GetToolForList” lookup used during link-following (when a link’s tool is "default" and the target is a CmPossibilityList) from the obsolete XCore Mediator.SendMessage reflection pathway to the FieldWorks FwUtils Publisher/Subscriber pub-sub system, reducing reliance on obsolete APIs and cross-assembly mediator messaging.
Changes:
- Replace
m_mediator.SendMessage("GetToolForList", ...)withPublisher.Publish(EventConstants.GetToolForList, ...)inLinkListener. - Add
EventConstants.GetToolForListsubscription/unsubscription inAreaListener. - Convert the handler from
OnGetToolForList(mediator/reflection shape) to a pub-sub handler that returns the tool name viaparameters[1].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Src/xWorks/LinkListener.cs | Publishes GetToolForList via pub-sub instead of calling obsolete mediator reflection. |
| Src/LexText/LexTextDll/AreaListener.cs | Subscribes to GetToolForList and provides the handler to map a list to its editing tool name via the pub-sub payload. |
| @@ -512,9 +512,7 @@ private bool FollowActiveLink(bool suspendLoadingRecord) | |||
| // Thus we've created this method (on AreaListener) which we call awkwardly throught the mediator. | |||
| Subscriber.Subscribe(EventConstants.SetToolFromName, SetToolFromName); | ||
| Subscriber.Subscribe(EventConstants.ReloadAreaTools, ReloadAreaTools); | ||
| Subscriber.Subscribe(EventConstants.GetContentControlParameters, GetContentControlParameters); | ||
| Subscriber.Subscribe(EventConstants.GetToolForList, GetToolForList); |
There was a problem hiding this comment.
I think I disagree with this type of request in this type of PR. This PR is a tool replacement. If I understand this correctly, then I think this is a pre-existing coverage gap, not a gap introduced by this PR. The mediator.SendMessage() call did not have this type of test coverage.
We probably should discuss as a group if tests for pre-existing coverage gaps that would be “nice to have”, should block a PR.
I’m adding the Claude generated tests to continue moving this PR forward.
mark-sil
left a comment
mark-sil
left a comment
There was a problem hiding this comment.
@mark-sil made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion.
| Subscriber.Subscribe(EventConstants.SetToolFromName, SetToolFromName); | ||
| Subscriber.Subscribe(EventConstants.ReloadAreaTools, ReloadAreaTools); | ||
| Subscriber.Subscribe(EventConstants.GetContentControlParameters, GetContentControlParameters); | ||
| Subscriber.Subscribe(EventConstants.GetToolForList, GetToolForList); |
There was a problem hiding this comment.
I think I disagree with this type of request in this type of PR. This PR is a tool replacement. If I understand this correctly, then I think this is a pre-existing coverage gap, not a gap introduced by this PR. The mediator.SendMessage() call did not have this type of test coverage.
We probably should discuss as a group if tests for pre-existing coverage gaps that would be “nice to have”, should block a PR.
I’m adding the Claude generated tests to continue moving this PR forward.
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
@johnml1135 reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on mark-sil).
This change is