fix(FlexView): copy federated views via the configuration API - #169
Open
Sario27 wants to merge 15 commits into
Open
fix(FlexView): copy federated views via the configuration API#169Sario27 wants to merge 15 commits into
Sario27 wants to merge 15 commits into
Conversation
…esolving to a live item Configuration.Instance.GetItem() never resolves a child-site view's rebuilt FQID back to a ViewAndLayoutItem - Views aren't part of MFA's federated client-session model (only devices/cameras/alarms/access are), so the existing ResolveFederatedView path always returned null and surfaced as "Open Failed... could not be resolved through the current session." FederationWalker already proved View.LayoutViewItems (plus Shortcut/LayoutCustomId/ LayoutIcon/ViewLayoutType) is readable straight off each site's Management Server config, federated children included. ViewFolder.AddView takes that same raw XML directly, so a child-site view can now be copied into a folder on this site purely through VideoOS.Platform.ConfigurationItems, without ever needing a client-side handle to the source view.
AddView/AddViewGroup return a ServerTask that can still be Progress<100 when the call returns; per the SDK docs, callers must poll UpdateState() until it completes. The previous commit called AddView and assumed success unconditionally, so a server-side failure (duplicate name, permission denied, invalid layout XML) would have been silently reported as a successful copy.
VideoOS.Platform.Client also defines a ViewGroup type, which collided with VideoOS.Platform.ConfigurationItems.ViewGroup once both namespaces were imported. Drop the blanket using and fully qualify the one config-plane ViewGroup construction instead, matching how ServerTask/StateEnum are already referenced in this file.
…nd load federated sites off the UI thread Two bugs found from a real test run: 1. "Copy Failed: Invalid kind for this constructor" - new ViewGroup(fqid) validates that the FQID came from the same configuration-API object graph. A client Item's FQID (from the folder browse dialog) never satisfies that, regardless of which folder is picked. Every other working config-plane read in this codebase reaches a ViewGroup by navigating properties from a genuine site FQID, never by reconstructing one from a client Item - so the destination is now found by walking the config tree and matching ViewGroup.Id instead. 2. Clicking Open View froze Smart Client for ~85 seconds. FederationWalker .CollectAllSiteViews() walks every site's configuration over the network, synchronously, directly on the UI thread. It's now offloaded to a background thread with a "Loading sites..." placeholder shown immediately.
…actually lives Confirmed against a real copy: layout/panes come through correctly via LayoutViewItems + AddView, but camera assignments don't. View.ViewItemChildItems (one per pane, keyed by ViewItemPosition) is a separate structure from LayoutViewItems and likely carries that content via ViewItemDefinitionXml - dump it for the first few views per site so the real XML shape is known before writing parsing/restore code against it.
Confirmed from a real ViewItemDefinitionXml dump: a camera slot looks like
<viewitem type="...CameraViewItem..."><iteminfo cameraid="{guid}" .../></viewitem>,
separate from LayoutViewItems (geometry only). FederationWalker now parses each
view's ViewItemChildItems and captures camera slot positions/ids.
After AddView creates the (empty-slotted) view, the destination is genuinely
local, so - unlike the source - Configuration.Instance.GetItem resolves it fine.
ServerTask.Path gives the new view's config-API path, used to read its Id and
rebuild a client FQID, then InsertBuiltinViewItem(position, CameraBuiltinId,
{CameraId}) restores each camera slot through the exact same pipeline the
same-site copy already uses. CameraBuiltinId was found via reflection (SDK docs
don't list it) alongside the sibling Hotspot/Carousel/Map/etc constants.
Non-camera slot types (maps, HTML, plugins) are left empty for now - camera
grids are the reported use case.
…g cameras Confirmed from a real run: AddView succeeds and camera slots parse correctly, but Configuration.Instance.GetItem returns null immediately afterward for the brand-new (genuinely local) view. The raw config API write goes through a side channel the client session's own item cache isn't synchronously notified about. Checked Configuration.RefreshConfiguration as the obvious fix first - its SDK doc explicitly rules it out for this case: "Only works for plug-in defined configurations ... built-in item types ... cannot be refreshed", and View is built-in. Also checked View.ApplyViewToView via IL as a same-server "clone from another view" alternative - it sets a ViewToApply property expecting a path on the same management server, which doesn't fit a cross-server source either way. With no way to force the client cache, retry GetItem with a short delay for up to ~7 seconds before giving up.
…ses, defer camera reads, and stop freezing the client on Save Three real problems found from live testing: 1. PopulateFederatedTree renders the master site from ClientViewRoots (the client-session tree, already fast/working) and never reads sv.FedViews for it - but CollectAllSiteViews was walking the master's full config-plane view tree anyway. Confirmed against a real system: that walk alone (3411 views) took ~73 of the ~90 second total "Loading sites..." time, entirely wasted. Only child sites (which have no working client-session tree - the reason this file exists) actually need that walk now. 2. The camera-restore fix in the previous commit read and parsed ViewItemChildItems for every view during the same walk, not just the one view actually being copied - roughly doubling an already expensive walk. FedView now only captures the (already-loaded, free) ServerId/Path needed to re-fetch one view's content later; FederationWalker.ReadCameraItems does that single fetch at copy time instead. 3. CopyFederatedViewToLocal ran AddView + the server-task poll + the client-cache retry synchronously on the UI thread, freezing Smart Client for however long all three took (confirmed up to ~45+ seconds in one run). The network-bound work now runs via Task.Run; only the two dialogs touch the UI thread.
Confirmed from a real run: AddView, its server-task poll, and reading the source's camera items (ReadCameraItems) all succeeded fine on the background thread introduced in the previous commit - but the final Save() threw "The calling thread cannot access this object because a different thread owns it." The ViewAndLayoutItem returned by Configuration.Instance.GetItem is a client-session object with UI-thread affinity, unlike the raw ConfigurationItems.* objects (ManagementServer, ViewGroup, View, ServerTask) used everywhere else in this file, which have no such restriction. Split the work accordingly: PrepareFederatedCopy (AddView + wait + read camera items) still runs via Task.Run since it's genuinely network-bound and thread- agnostic; RestoreCameraSlots (resolve the new view + InsertBuiltinViewItem + Save) now runs back on the UI thread after the await, bounded to a few seconds by its own retry cap rather than the whole operation's much longer duration.
…ailure, and stop blocking the UI for up to 46s while waiting Two things found by comparing two real runs: 1. On the run that hit the thread-affinity bug (Save() called from the wrong thread), the resulting view actually had working cameras despite the plugin reporting "0/4 restored" - InsertBuiltinViewItem's effects evidently persist independent of the subsequent Save() call. RestoreCameraSlots was resetting the whole restored count to 0 whenever that final Save() threw, which undercounted copies that had actually worked. A Save() failure is now logged but no longer discards an already-successful restore count. 2. On the next run (now correctly running on the UI thread per the previous commit), the client-cache retry legitimately took 45.9 seconds before giving up - individual Configuration.Instance.GetItem calls are themselves slow (~2-3s each, real network round-trips), not just the delay between them, and my 7.5-second cap was nowhere near enough. That whole wait ran via Thread.Sleep, fully blocking Smart Client's UI thread. The retry loop is now async (Task.Delay instead of Thread.Sleep, deadline raised to 90s) so the UI stays responsive between attempts, even though each individual attempt still has to run on this thread and takes however long the network call takes.
…dd manual Refresh Confirmed working end-to-end, but every Open View click re-walked every child site's full view tree from scratch, even though views rarely change site-to-site within a single Smart Client session. FederationWalker.CollectAllSiteViews now caches its result for the session; a Refresh button (only shown in federated Open View mode) calls FederationWalker.ForceRefresh() and re-walks on demand for the rare case something changed on another site. Deliberately no automatic/time-based expiry - manual refresh only, per explicit direction.
…tually instant The session cache from the previous commit skipped re-walking site configs, but PopulateFederatedTree was still calling CreateTreeNode(root) for the master's ~113+ view groups on every single Open View click - that recursively calls Item.GetChildren() live, over the network, for every node. That recursive fetch was the ~10-second cost still showing up after the cache "hit". FederationWalker.BuildClientNode now resolves that whole tree once, up front, while the site is walked (same place ClientViewRoots was already being populated). ViewBrowserWindow builds TreeViewItems from that cached structure (CreateTreeNodeFromClientNode) instead of calling GetChildren() itself, so rebuilding the on-screen tree from a cache hit is pure in-memory recursion.
Each openable child-site view in the browse tree now has a checkbox. Checking one or more switches btnSelect into "Copy N Selected" mode; confirming prompts for a single destination folder (not one per view) and copies each selected view into it, keeping each view's own name auto-deduped via UniqueName rather than prompting per view. One summary dialog covers the whole batch instead of one dialog per view, and a failure on one view doesn't stop the rest. DestinationInfo/ResolveDestination factor out what used to be per-copy work (resolving the ViewGroup, walking its existing view names) so a batch of N views only pays that cost once instead of N times. The existing single-view Select flow (SelectedFedView) is unchanged and still works when nothing is checked.
Owner
|
@Sario27 oh wow awesome bro :) let me review this tomorrow. Thank you for back reporting and creating a PR 🤠 |
Owner
|
@Sario27 how long does it now takes to load all sites ? |
Author
For 130 view groups across 3 MGMT servers it took about 90 seconds for the first time, after that if you open it again it only take about 5 seconds |
Owner
|
@Sario27 did you try to parallelize the view group walking ? |
Author
I didn't let me look into that |
Owner
|
@Sario27 should be possible to drop the 90 seconds or does the smart client also take so long to load ? |
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
2 participants
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.
Summary
Copying a view from a child site (in a Milestone Federated Architecture hierarchy) into a local View Group failed with "This view is on another site and could not be resolved through the current session", and browsing federated sites could take 90+ seconds and freeze the client.
Root cause: Views are not part of MFA's federated client-session model -
ClientControl/Configuration.Instance.GetItemcan never resolve a view living on a child site, unlike devices/cameras/alarms/access, which are genuinely federated.Fix: read views directly off each site's Management Server configuration (
ManagementServer -> ViewGroupFolder -> ViewGroup -> ViewFolder -> View) instead of the client session - this works uniformly for the master and every child site, since it doesn't depend on federation at the client layer at all. The copy is recreated viaViewGroup.ViewFolder.AddView(...)using the capturedLayoutViewItemsXML, then camera slots are restored afterward through the client session (InsertBuiltinViewItem, the only documented way to write per-slot content) - which now works because the destination is genuinely local.A few real bugs turned up from testing this against a live 3 MGMT servers, with 130 view groups, all fixed along the way:
ViewGroupis ambiguous betweenVideoOS.Platform.ClientandVideoOS.Platform.ConfigurationItems(CS0104)new ViewGroup(fqid)throws"Invalid kind for this constructor"when the FQID comes from a clientItemrather than the config-API's own object graph - the destination is now resolved by walking the tree and matchingIdinsteadGetChildren()calls on every open"The calling thread cannot access this object because a different thread owns it"onSave()Configuration.RefreshConfigurationexplicitly excludes built-in types), so restoring cameras needs a bounded async retrySave()failure was incorrectly zeroing out an already-successful camera restore in the reported countAlso added:
Test plan