Move the user interface free half of the sample clients into one library - #869
Merged
Merged
Conversation
Issue #865 recorded what the Workshop client split deliberately left open. Working through it turned up more duplication than the issue knew about, so most of this is deletion: 5961 lines out, 1653 in. Workshop/Common was the surprise. Its ModelUtils was a second copy of the instance declaration walk in ClientUtils, and the HistoricalEvents *client model* used it - so a model depended on a Windows Forms assembly, which is the one thing the model tier exists to prevent. Both copies are now SampleTypeModel. FormUtils duplicated SampleSession, FilterDefinition and SelectLocaleDlg were stale forks, and ParsedNodeId duplicated Opc.Ua.Server.ParsedNodeId - which is the type the sample servers were resolving to anyway, so nothing referenced it. The Quickstart library is now only the backward compatibility shim it is named for. The shared control library kept a connection state machine inside a UserControl, which meant the only way to open the kind of session the samples run on was to create one; the headless tests went through SampleSessionFactory instead and so exercised a second implementation of the same sequence. SampleConnection now owns discovery, the session, the reconnect and the bounded close, and ConnectServerCtrl is the tool bar over it. Certificate questions and thread marshalling stayed with the window on purpose: a form closing blocks its own message loop, so an event posted to that loop arrives after the form is gone. The two subscribe wizards each owned a copy of the same subscription plumbing, and the copies had drifted - their status strings disagreed. SampleSubscription backs both. SampleBrowser, SampleHistory and SampleDiscovery take the rest of the protocol out of the browse tree, the history view and the endpoint editor. Each Workshop client gained a composition root next to its entry point, Add<Sample>Client(), the counterpart of the Add<Sample>Server() of its server half, and its window takes the model through its constructor. Tier 2 builds each form through that same registration instead of calling the constructor, so a client which starts taking something else from the container needs no change to the table. The Global Discovery Client had its logic in three panels; it now has a Model namespace like the Workshop clients, and ModelContractTests covers it. It does not build on SampleClientModel - it has no session a connect control hands over - so the contract test learned to check a client's models without demanding that base class. Windows now dispose their model from Dispose(bool), which needed a synchronous Dispose on the base and removed all 18 CA2213 suppressions the issue counted. That turned the analyzer loose on the models' own fields and found a real leak: the refresh semaphore of StateMachinesClientModel was never disposed. Two more in the GDS certificate path are fixed rather than suppressed. Left for their own change: BrowseTreeCtrl and SessionTreeCtrl of Controls.Net4 still mix tree bookkeeping with protocol. SubscriptionHandle moved out of that assembly - it was user interface free already - but merging it with SampleSubscription would touch ten control files that one test covers, so the two document each other instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three conflicts, all of them two additions to the same place: SampleClientModel gained DisposeAsyncCore() on master while this branch gave it a synchronous Dispose. Keeping both is not enough - the synchronous path has to run DisposeAsyncCore as well, or a model which owns something of its own (the DataTypes model and its service provider) leaks exactly when a window disposes it, which is the normal way it is disposed. SampleClientFactories: master added a RuntimeNodeSets row to the old shape while this branch changed the shape. Kept the new shape and brought the new client into it - RuntimeNodeSetsClientHosting, the model through the constructor, the registration in Program.Main - so it matches the other eighteen. The README of the client library: master documented NodeSetExport and ReverseConnectListener, this branch documented six other types and the split of the connect control. Both lists kept. SetTriggeringDlg and SampleControlsTriggeringTests arrived from master naming MonitoredItemHandle and SubscriptionHandle, which moved into the user interface free library here; they take the using. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A concurrent session committed while this worktree held the temporary server side logging a role audit investigation had installed, so bf973ff carries a ProbeFileLoggerProvider wired into SampleServerHost and a stray .bak copy of the file. Neither belongs in the tests: the host deliberately registers no logging provider, which is what the comment above AddLogging says. Restores SampleServerHost.cs to what it was before the investigation and removes the backup file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nch uses It arrived from master while this branch was open. Its model needs the CA2213 justification the other ten carry, now that SampleClientModel is IDisposable and the analyzer therefore looks at the fields a model releases asynchronously in OnDetachingAsync. SessionTreeCtrl had picked up the same using twice across the merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Works through the follow-ups #865 recorded when the Workshop client split stopped where it did. Doing so turned up more duplication than the issue knew about, so a lot of this is deletion.
Workshop/Commonwas the surprise. ItsModelUtilswas a second copy of the instance declaration walk inClientUtils— and the HistoricalEvents client model used it, so a model depended on a Windows Forms assembly, which is the one thing the model tier exists to prevent. Both copies are nowSampleTypeModel.FormUtilsduplicatedSampleSession;FilterDefinitionandSelectLocaleDlgwere stale forks;ParsedNodeIdduplicatedOpc.Ua.Server.ParsedNodeId, which is the type the sample servers were resolving to anyway, so nothing referenced it. The Quickstart library is now only the backward-compatibility shim it is named for.The connection state machine left
ConnectServerCtrl. It lived inside aUserControl, so the only way to open the kind of session the samples run on was to create one; the headless tests went throughSampleSessionFactoryinstead and exercised a second implementation of the same sequence.SampleConnectionnow owns discovery, the session, the reconnect and the bounded close, and the control is the tool bar over it. Two things stayed with the window deliberately: asking a person about an untrusted certificate, and thread marshalling — a form closing blocks its own message loop, so an event posted to that loop arrives after the form is gone, while one raised inline still reaches it.The two subscribe wizards had drifted copies of the same subscription plumbing; their status strings disagreed.
SampleSubscriptionbacks both, on the more informative of the two formats.SampleBrowser,SampleHistoryandSampleDiscoverytake the remaining protocol out of the browse tree, the history view and the endpoint editor.Each Workshop client gained a composition root,
Add<Sample>Client(), the counterpart of theAdd<Sample>Server()of its server half, and its window takes the model through its constructor. Tier 2 builds each form through that same registration rather than calling the constructor, so a client which starts taking something else from the container needs no change to the factory table.The Global Discovery Client kept its logic in three panels and now has a
Model/namespace like the Workshop clients (RegistrationModel,CertificateModel,TrustListModel), covered byModelContractTests. It does not build onSampleClientModel— it has no session a connect control hands over, it works through the GDS, push-configuration and discovery clients of the stack — so the contract test learned to check a client's models without demanding that base class. The two questions only a person can answer reach the models as delegates: whether an existing certificate file may be overwritten, and which record is meant when the GDS holds several for one application URI.Windows now dispose their model from
Dispose(bool), which needed a synchronousDisposeon the base and removed all 18CA2213suppressions the issue counted. That turned the analyzer loose on the models' own fields and found a real leak — the refresh semaphore ofStateMachinesClientModelwas never disposed. Two more in the GDS certificate path are fixed rather than suppressed.Related Issues
Types of changes
Checklist
Further comments
Test results. Tier 0 237/237, tier 1 121/121, tier 1.5 178/178, tier 1.7 420/420, tier 2 65/65. Build warning count is unchanged from master.
This branch also removes a debug leftover from #864 that was making every server-side tier slow: a
ProbeFileLoggerProviderinTests/Samples.Tests.Common/SampleServerHost.cs, registered atLogLevel.Trace, doing oneFile.AppendAllTextper traced line under a process-wide lock, plus a straySampleServerHost.cs.bak. The comment directly above the registration still said "the tests log nothing from the server", which is the tell.It was making
PerfTestClientCountsTheUpdatesItMeasuresfail on master. A controlled A/B here, toggling only that registration, gave 3/3 passes at 3-7 s with it off against 3/3 failures at 41-50 s with it on, writing 97 MB in three runs. With it removed, the whole of tier 2 now runs in 2m28s instead of ~6m and passes 65/65.The fix is cherry-picked from
3d6873a8(branchromanett/ua-netstandard-836-8b642f), which never reached master.What was left for its own change.
BrowseTreeCtrlandSessionTreeCtrlofControls.Net4still mix tree bookkeeping with protocol.SubscriptionHandlemoved out of that assembly — it was user-interface-free already, only in the wrong place — but merging it withSampleSubscriptionwould touch ten control files that a single test covers, so the two document each other instead.ReferenceClientandClient.Net4needed nothing: their forms are 183 and 89 lines and already thin, because their logic was always in the shared control libraries.Reading order for review.
Samples/Client.Common/README.mddescribes what the library now holds and why the connect control is split the way it is;Samples/Hosting/README.mdcovers the container registration and the threading reason a model is a singleton;docs/TESTING.mdcovers the factory table change.🤖 Generated with Claude Code