Making the MetaMorpheus GUI usable on macOS and Linux - #2709
Conversation
|
The WPF GUI is the only way to drive MetaMorpheus interactively and it does not run outside Windows. This adds an Avalonia shell that builds the task list, adds spectra and databases, and runs EverythingRunnerEngine, plus settings dialogs for all six task types and fixed/variable modification selection. Two things worth knowing for review: - The view model subscribes to MetaMorpheusEngine's static events, so it is IDisposable and unsubscribes. Without that, tests passed alone and failed in a suite, because each surviving view model kept handling another's events. - Engine events arrive off the UI thread, so they are marshalled through Dispatcher.UIThread. 35 tests, headless via Avalonia.Headless.NUnit. One of them runs a real search end to end and asserts the defaults still yield 22 PSMs, so a settings regression that silently changes results fails the build rather than being noticed later. Not yet ported: file-specific parameters and the smaller auxiliary dialogs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds both projects to the solution, so the existing build job compiles them on all three platforms, and adds a test job that runs the 35 GUI tests on all three. The UbuntuMac mappings are set by hand because `dotnet sln add` points that configuration at Debug, and for GUI.Avalonia it omitted Build.0 entirely - the solution then builds "successfully" while silently skipping the project. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
16f6c37 to
e1452cf
Compare
|
Thanks for opening this as a draft with the questions stated up front — that framing made it much easier to review as a direction proposal rather than as a merge candidate. Answering Q1–Q3 below. A separate review with line-level findings follows; none of it is a merge objection, since this isn't offered for merge. Short version: the toolkit choice is sound, converge rather than coexist, and there is one piece of sequencing that has become urgent since you opened this. 0. The thing that changed this week: rebase onto .NET 10 before Milestone 1#2710 and smith-chem-wisc/mzLib#1141 (both "Move from .NET 8 to .NET 10", both non-draft and mergeable) are expected to land shortly. That reorders the work here. As it stands, this PR will not restore once #2710 merges. There is no textual merge conflict, which is the trap. I checked with a real three-way merge against the shared base: #2710's The fix is four lines, and it is much cheaper now than after Milestone 1:
Nothing in the dependency set objects: Avalonia 12.1.1 and The stronger reason to do it first, though, isn't the build break — it's that your compiled bindings aren't being checked right now. CI shows 8 × Milestone 1 is the milestone that adds most of the XAML and bindings. Written today, all of it compiles without the generator ever running, and the errors surface later across a much larger diff with no clean bisect point. So: stop any CS9057 suppression or Q1 — Avalonia as the toolkitNo objection, but two caveats the reasoning doesn't cover, and one argument in your favour you didn't make. The WPF-similarity argument holds, and there's a strong precedent you don't cite: JetBrains ported dotMemory and dotTrace from WPF to Avalonia — data-dense, chart-and-grid-heavy engineering tools, exactly this shape — and reached all three OSes. (Rider is not Avalonia; best not to cite it.) Unity collapsed WinForms + GTK# + Xamarin.Mac into one Avalonia codebase in ~2 months. Caveat 1: there is zero Avalonia prior art in mass spectrometry. I searched hard for it — web, Caveat 2: Wayland behaviour under Avalonia 12 is unconfirmed, and is the likeliest unpleasant Linux surprise. Two claims worth tightening before a maintainer pushes back:
The strongest argument for this PR is the peer landscape, and it isn't in the description. The dominant pattern in the field is cross-platform engine, Windows-only GUI: Skyline (WinForms; the official answer is Parallels), MaxQuant, DIA-NN and Spectronaut are all Windows-GUI/Linux-CLI; FragPipe ships no macOS asset at all. Only MZmine (JavaFX) and OpenMS/TOPPView (Qt) have real cross-platform GUIs — and both were born cross-platform; neither ported a Windows GUI. MetaMorpheus already has the cross-platform engine the others lack, so this would put it ahead of Skyline, MaxQuant, DIA-NN, Spectronaut and FragPipe on OS reach, not at parity. The counterweight is where the recurring cost actually lands: MZmine maintains an institutional Apple Developer ID with a documented sign/notarise/staple procedure and ships separate Intel and Silicon artefacts, and OpenMS explicitly does not conda-package its GUI tools. The cost is in shipping, which MM's CI has none of today. Q2 — Replace or coexistConverge, don't coexist — but the convergence is the extraction, not a rewrite, and it should be sequenced behind cheaper work. Coexistence is the expensive option, and divergence has already started. This PR makes fixed/variable modifications mutually exclusive because "the WPF window allows it" — and you're right: WPF harvests two independent trees with no cross-check ( For calibration: After the .NET 10 rebase, three things should still come before more GUI work. 1. Test whether Thermo I need to correct something I nearly told you: there is no x64 "pin" in mzLib to lift. All 19 mzLib projects declare The real suspect is much more specific. mzLib vendors Thermo CommonCore 8.0.37 as four checked-in DLLs (bare Load that one assembly on an arm64 Mac first. It's minutes of work and it's the most likely single point of failure. If it loads, 2. Let #2710 and mzLib#1141 land, and rebase onto them — as above. 3. Ship a bioconda recipe + BioContainer + an arm64 Docker manifest. MetaMorpheus is absent from bioconda entirely, while Then: the cross-platform One thing Bruker settles permanently. mzLib ships A Mac user with a timsTOF cannot use MetaMorpheus regardless of GUI toolkit. That's not an argument against this work; it's a scope boundary the PR should state up front. Two work items missing from the plan:
Milestone 1 — Calibrate → GPTMD → Search, cross-platformDeliverable: a macOS/Linux user builds and runs MetaMorpheus's standard three-task pipeline without hand-writing TOML. XLSearch, GlycoSearch and Average are explicitly out of scope, and their currently-unreachable code comes out. Why this scope rather than Search-only: Gates — binary, independently checkable, agreed before the workA — Parameter fidelity. Load a task from TOML with non-default values across the parameter space, open settings, click Save without editing, write TOML back. The two TOMLs must be equivalent. One test subsuming dozens. It fails today — see the inline findings on The repo already contains both halves of the mechanism, so this is small: No-go: if this can't pass without restructuring the engine's parameter objects, stop — every later task inherits the defect. On current evidence it can, easily: B — Result equivalence with the CLI. Build the three-task pipeline through the GUI, run it, run the same TOML through C — Event-surface completeness. A reflection test asserting that every event the WPF GUI subscribes to is either subscribed here or in an explicit, commented exclusion set. One ordering constraint: the marshalling has to handle events raised from arbitrary pool threads first, or newly-subscribed engine events get posted to an unpumped dispatcher and silently dropped, and the gate passes while delivering nothing. Details inline. D — CI genuinely exercises three platforms. E — No newly invented conventions. F — Claims match the UI. Every capability in the PR description reachable by clicking. Q3 — Distribution, merge and releaseMerged ≠ released, and that's the safety net. There is a live cost to the asymmetry today, though: adding On size — that's the wrong axis. GitHub allows 2 GiB per file, no total release limit, and unmetered bandwidth. The real constraints are:
Recommendation: framework-dependent for What abandonment would actually costThe code being "deletable in two directories" is true, and misses four things:
Suggested PR sequence. (0) Rebase onto .NET 10 — now the first move, not a later cleanup. (1) The cross-platform On the extraction's real size, correcting my own first estimate: Stop-without-waste points, so this never becomes sunk cost: after the .NET 10 rebase (pure gain regardless); after the A long-lived branch is the failure mode this PR already brushed against — seven commits behind, re-doing #2701. You caught and fixed that; the .NET 10 migration is the next instance and it's days away. Nice work on this. Several things here are done more carefully than the surrounding codebase: dropping the redundant #2701 commit in the rebase rather than carrying it, the deliberate |
trishorts
left a comment
trishorts
left a comment
There was a problem hiding this comment.
Line-level findings from a full pass over the diff, with the surrounding MetaMorpheus and mzLib code checked for each one. Commented rather than "request changes", because this is a draft opened for a direction decision — none of this is a merge objection. My answers to Q1–Q3 are in the conversation comment above; this review is just the detail.
Two framing notes before the findings.
Most of the duplication here was forced, not careless. GuiFunctions is net8.0-windows with UseWPF and 60+ WPF/System.Drawing dependencies, and GUI.Avalonia is plain net8.0 — so it cannot reference it at any price. For the MVVM base, the mod tree and the contaminant rule, writing new code was the only option available to you. Where I raise those, the ask is that they be recorded as deliberate, tracked duplication, not that they be undone here.
One theme ties the three most serious findings together. TaskSettingsViewModel.Apply() drops 28 of CommonParameters'' 42 constructor arguments and 6 of DigestionParams'' 12, and LoadFrom reads the wrong protease property. All three are fixed by the same move: CommonParameters.Clone() (EngineLayer/CommonParameters.cs:222) and DigestionParams.Clone() (mzLib Proteomics/ProteolyticDigestion/DigestionParams.cs:128) already exist, already copy every field forward, and the GUI has never used either. Clone, then overwrite only what the dialog edits. That single change takes out the highest-severity items in this review.
One item with no diff line to attach to, so it goes here: build.yml:56 asserts architecture-neutral PE headers by looping for project in CMD EngineLayer TaskLayer and deriving the DLL name from the project name (:57). GUI.Avalonia.csproj sets <AssemblyName>MetaMorpheus.Avalonia</AssemblyName>, so adding GUI.Avalonia naively would look for a file that doesn''t exist and hit the hard-failure branch at :60 — it needs project:assembly pairs. Worth doing: this PR''s .sln change means GUI.Avalonia now compiles on the Unix legs, so the desktop app users will actually launch on Apple Silicon is the one new assembly that assertion exists to protect, and the one left unchecked. It would pass — the csproj deliberately omits PlatformTarget. Also, build.yml:77 still carries TODO(#2503): make this step required after the mzLib bump lands, and 1.0.584 has landed, so that TODO is now actionable.
A few smaller things I didn''t comment inline, to keep the noise down: CanRun (MainWindowViewModel.cs:195) raises no notification and isn''t bound — the Run button uses IsEnabled="{Binding !IsRunning}" — so it''s test-only; same for the unused Version property. Log += line (:246) is O(n²) over a multi-MB string bound to a TextBox that re-measures the whole document per line, and per the dispatcher note it now executes on engine worker threads, making it a racy read-modify-write. DoQuantification (:85, assigned :145) is never read by Apply() and unbound. MaxThreadsPerFile loses the -1 sentinel, so a load/apply round trip bakes a machine-specific integer into the TOML. CreateSettingsFor (TaskSettingsViewModel.cs:182-185) can return null and MainWindow.axaml.cs:67-72 dereferences it unguarded. MassDiffAcceptorType.Custom is offered but Apply() never sets SearchParameters.CustomMdac, which WPF does at SearchTaskWindow.xaml.cs:748. Rebuild() re-materialises ~4,000 mods and raises Reset on every keystroke, discarding scroll offset and Expander state — though selection state is correctly preserved by reusing ModificationChoice instances, which is good design. GlobalVariables.SetUpGlobalVariables() runs per-test in four fixtures; [OneTimeSetUp] would save re-parsing the mod databases ~20 times per leg. And the namespace MetaMorpheus.Avalonia shadows the toolkit root — it compiles today only because every using Avalonia.* sits above the file-scoped namespace declaration, but with ~600 WPF type references in the stated MetaDraw roadmap, fully-qualified Avalonia.Point/Avalonia.Media references will fail with errors pointing at the wrong thing, fixable only with global::. Repo convention is a single token (MetaMorpheusGUI), so MetaMorpheusAvalonia both matches and dodges it. That one is rename-now-or-never.
Things I checked and specifically am not raising, so nobody re-litigates them: the .sln configuration rows are correct and complete across all three configurations; the deliberate PlatformTarget omission with its #2503/#2504 comment is right and any future extraction project should copy it; [assembly: AvaloniaTestApplication] is wired correctly; static-event unsubscription in Dispose is correct and every test view model uses using var; RunAsync re-entrancy can''t happen because [RelayCommand] disallows concurrent execution; Avalonia 12.1.1 against DataGrid 12.1.2 is fine (the DataGrid nuspec declares Avalonia >= 12.1.0 and ships out of band); the App.axaml DataGrid theme include is present; compiled bindings against internal view models are fine; and GitHub release size limits aren''t a real constraint. I also initially thought the end-to-end test never observed engine events — that was wrong, and I verified it by instrumenting the real view model: the events land, Progress reaches 100, and the tests genuinely pass 9/9 with the search writing 2 .psmtsv files in about 2 seconds.
Nice work on this — the test suite in particular is better than the WPF side it''s modelled on, and I''ve said so in the conversation comment.
| private void Cancel() | ||
| { | ||
| // The engines poll this cooperatively; there is no forceful abort. | ||
| GlobalVariables.StopLoops = true; |
There was a problem hiding this comment.
Result-changing: Cancel() permanently poisons the process.
This sets GlobalVariables.StopLoops = true and nothing ever resets it. The WPF GUI does both halves: GUI/MainWindow.xaml.cs:843 sets it true on cancel, and :896 sets it false as the very first statement of RunAllTasks_Click. EverythingRunnerEngine.Run() never touches it, and the ~19 TaskLayer/EngineLayer sites only read it.
So: click Cancel once, then Run again — every later run in that process returns in under a second, writes no results, and reports "Ready." The only recovery is restarting the app.
One-line fix at the top of RunAsync (around line 214), mirroring the WPF ordering.
There was a problem hiding this comment.
🤖 Fixed in b34c6d12, mirroring WPF's ordering (reset before validation). Worth noting the symptom is worse than described: the next run starts, then dies with ArgumentNullException (Parameter 'source') when an engine bails out early — a loud failure pointing nowhere near the cancel. The regression test cancels and then runs a real search requiring results, rather than reading the flag back.
|
|
||
| CommonParameters existing = _task.CommonParameters ?? new CommonParameters(); | ||
|
|
||
| _task.CommonParameters = new CommonParameters( |
There was a problem hiding this comment.
Result-changing: this silently resets 28 of CommonParameters'' 42 constructor arguments.
14 named arguments are passed here; EngineLayer/CommonParameters.cs:25-67 takes 42. The rest revert to constructor defaults rather than the task''s own values, on every Apply — including addCompIons, totalPartitions, separationType, pepQValueThreshold, trimMs1Peaks, addTruncations, assumeOrphanPeaksAreZ1Fragments, numberOfPeaksToKeepPerWindow, precursorDeconParams, productDeconParams, precursorMassMatchMode and rtPredictorName.
This is the exact failure mode the codebase warns about in capitals, in two places:
EngineLayer/CommonParameters.cs:150-151— "If you add a new property here, you must add it to MetaMorpheusTask.cs/SetAllFileSpecificCommonParams !! If you forget this, ... your settings will be overwritten by default values!"TaskLayer/MetaMorpheusTask.cs:584— "//NEED THESE OR THEY''LL BE OVERWRITTEN", above the 33 lines that pass every remaining argument forward.
MetaMorpheusTask.SetAllFileSpecificCommonParams (MetaMorpheusTask.cs:519) is the canonical "override a subset, lose nothing" implementation and is structurally identical to a settings dialog. For comparison, SearchTaskWindow.xaml.cs:622-654 passes 32 of 42, with most of the remainder carried through precursorDeconParams/productDeconParams.
The fix is one call, not 28 arguments. CommonParameters.Clone() already exists at CommonParameters.cs:222 (reflection over every property), with CloneWithNewDissociationType (:232) and CloneWithNewTerminus (:243) beside it. Four engine/task call sites use them; the GUI uses none. Clone existing, then overwrite only what the dialog actually edits.
Checkable today: GlycoSearchTask.cs:21-41 sets ms2childScanDissociationType: EThcD and numberOfPeaksToKeepPerWindow: 1000. Open its settings, click Save without editing anything, and they become Unknown and 200. SpectralAveragingTaskWindow.xaml.cs:60-67 deliberately sets dissociationType: LowCID ("ensures no filters occur when loading files in MM Task"); this dialog writes HCD.
Minor: the XML comment above this method, and the one on SettingsNotShownInTheDialogAreNotLost, both say "41-argument constructor". It''s 42.
There was a problem hiding this comment.
🤖 Fixed in 4ab29d9c via a new CommonParameters.CloneWithNewValues, a sibling of the existing CloneWithNewDissociationType. Also corrected the "41-argument" comments — it is 42.
| /// </summary> | ||
| public void Apply() | ||
| { | ||
| IDigestionParams digestion = new DigestionParams( |
There was a problem hiding this comment.
Result-changing: 6 of DigestionParams'' 12 constructor parameters are dropped, and RNA mode throws.
mzLib''s Proteomics/ProteolyticDigestion/DigestionParams.cs:15-18 takes 12 parameters. Six are passed here. The consequences differ in severity:
searchModeTypeis dropped, so a NonSpecific or Semi search silently becomesCleavageSpecificity.Fullon Save.SearchTaskWindow.xaml.cs:540preserves it precisely because only Search offers semi/non-specific — a shared VM coveringSearchTaskinherits that obligation.fragmentationTerminusis dropped (same reason,SearchTaskWindow.xaml.cs:541).initiatorMethionineBehavioris dropped. This one is preserved by 4 of the 5 WPF windows, plus the file-specific path (MetaMorpheusTask.cs:542) and mzLib''s ownClone. OnlyCalibrateTaskWindowomits it, and that''s the one window with no such combo box. Dropping it here regresses Search, GPTMD, XL and Glyco to Calibrate''s level.keepNGlycopeptide/keepOGlycopeptideare also dropped.
Separately, this line throws in RNA mode. All five WPF windows branch on GuiGlobalParamsViewModel.Instance.IsRnaMode and build RnaDigestionParams instead (SearchTaskWindow.xaml.cs:520-528, CalibrateTaskWindow.xaml.cs:280-288, GPTMDTaskWindow.xaml.cs:539-547). There''s no RNA branch here, and new DigestionParams(protease: ...) performs an unguarded ProteaseDictionary.Dictionary[protease] (mzLib DigestionParams.cs:20) — so any RNase name gives a KeyNotFoundException.
Same fix as the CommonParameters comment: DigestionParams.Clone(FragmentationTerminus? = null) (mzLib DigestionParams.cs:128) copies all 12 forward and already branches on SearchModeType == CleavageSpecificity.None.
There was a problem hiding this comment.
🤖 Fixed in beef683a, using the same clone-and-override shape. One correction: the RNA case never threw. LoadFrom's type test was is DigestionParams, which RnaDigestionParams fails, so Protease kept its "trypsin" default and Apply() replaced the RnaDigestionParams with a proteolytic one — no exception, an RNA task quietly became a protein search.
|
|
||
| if (common.DigestionParams is DigestionParams digestion) | ||
| { | ||
| Protease = digestion.Protease?.Name ?? Protease; |
There was a problem hiding this comment.
Result-changing: this displays an enzyme the user never chose.
The WPF convention is SearchTaskWindow.xaml.cs:200:
ProteaseComboBox.SelectedItem = digestionParams.SpecificProtease; //needs to be first, so nonspecific can override if necessaryFor a non-specific task, Protease is singleN/singleC while SpecificProtease holds the user''s actual choice (mzLib DigestionParams.cs:45 and RecordSpecificProtease()). Reading Protease?.Name means loading a semi- or non-specific task shows singleN in the enzyme box.
Combined with searchModeType being dropped in Apply(), the round trip both loses the mode and misreports the enzyme. DigestionParams.Clone() handles this case explicitly and would fix both.
There was a problem hiding this comment.
🤖 Fixed in beef683a — reads SpecificProtease, with a test that a non-specific task shows the user's enzyme in the box and keeps CleavageSpecificity.None across Save.
|
|
||
| private static Tolerance BuildTolerance(string value, bool isPpm, double fallback) | ||
| { | ||
| if (!double.TryParse(value, out double parsed) || parsed <= 0) |
There was a problem hiding this comment.
Cross-platform: culture-sensitive parsing, in the PR whose entire purpose is other platforms and locales.
double.TryParse(value, out double parsed) uses the current culture. The five WPF task windows contain 157 uses of CultureInfo.InvariantCulture with zero counter-examples — every double round-trip, without exception (SearchTaskWindow.xaml.cs:299,317,326,511,549; TaskValidator.cs:125-356). GUI.Avalonia contains no CultureInfo at all.
Two distinct failure modes on a de-DE machine:
double.TryParse("0.05")returnstrue, with the value5.0—.is the group separator there and .NET Core parses it leniently. A user typing0.4into Product tolerance silently gets a 10× looser tolerance, with no error and no visual cue.- When a parse genuinely fails, this method doesn''t report it — it silently substitutes
fallback(5 ppm / 20 ppm).Validate()uses the same current-culture parse, so it reports nothing either.
The formatting side has the same problem (Describe at :196-202 calls .ToString() with no culture), so a value written on one locale can be misread on another.
This also breaks SettingsAffectResultsTests.cs:80 on such a machine, with a failure message that falsely accuses correct wiring.
There was a problem hiding this comment.
🤖 Fixed in 8d35b030. Both failure modes reproduced against the unfixed parse: on de-DE "0.05" reads as 5.0, and on fr-FR the parse fails and BuildTolerance silently substitutes its 20 ppm fallback. The test covers de-DE, fr-FR and en-US, and the formatting side goes through InvariantCulture too.
| EndProject | ||
| Project("{B7DD6F7E-DEF8-4E67-B5B7-07EF123DB6F0}") = "Bootstrapper", "Bootstrapper\Bootstrapper.wixproj", "{E0EA5AC4-24A9-43DC-8FBC-CCEB3B9935B6}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "GUI.Avalonia", "GUI.Avalonia\GUI.Avalonia.csproj", "{BD94D208-D962-42AC-84A7-E66A39FBACA8}" |
There was a problem hiding this comment.
Both new projects use the legacy project-type GUID {FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}, while all seven existing C# projects in this solution use the SDK-style {9A19103F-16F7-4668-BE54-9A1E7A4F7556}.
MSBuild ignores it, but Visual Studio uses it to choose a project system. Cosmetic, one-character-class fix.
(The configuration rows below are correct and complete, incidentally — Debug|Any CPU, Release|Any CPU and UbuntuMac|Any CPU, each with both ActiveCfg and Build.0. That''s the thing most easily got wrong after #1127, and it''s right here.)
There was a problem hiding this comment.
🤖 Fixed in 9d2d3f6d — both entries now use the SDK-style GUID like the other seven. (And noted on the configuration rows; those were the part I was most worried about getting wrong.)
| /// Apply() rebuilds CommonParameters from a subset of its 41 constructor arguments, so anything | ||
| /// not represented in the dialog must survive rather than reverting to a constructor default. | ||
| /// </summary> | ||
| [Test] |
There was a problem hiding this comment.
This test certifies the opposite of its name.
It asserts only TaskDescriptor (:121) and ListOfModsFixed (:123) — and those are two of the 14 arguments Apply() does forward. None of the 28 arguments it drops are checked, so the test passes precisely because it examines the safe subset.
RemainingTaskTypeTests.GlycoSearchOptionsRoundTrip (ModificationAndTaskTypeTests.cs:143-160) has the same shape: it runs Apply() on a GlycoSearchTask and passes while silently degrading ms2childScanDissociationType from EThcD to Unknown.
The repo already has the mechanism to make this real. Test/TestToml.cs:61 asserts results.SequenceEqual(resultsToml) — running the original and the round-tripped task through EverythingRunnerEngine and comparing output byte-for-byte. That''s the strongest fidelity assertion in the codebase and it needs no property list. Combined with the reflective GetProperties() loop CommonParameters.Clone() already uses (CommonParameters.cs:225), an exhaustive version of this test is about 15 lines and would fail today on all 28.
That test is Gate A in my conversation comment — the one gate I''d want passing before any further task types are added, since every later task inherits the defect.
There was a problem hiding this comment.
🤖 Fixed in beef683a — this is Gate A. ApplyWithoutEditingChangesNothing loads a task holding non-default values, Saves without editing, and requires every CommonParameters property to compare equal by reflection, so a property added later is covered without the test knowing about it. Three explicit tests name the digestion settings, the non-specific round trip and the RNA case so a failure says which one broke. Verified all four fail against the unfixed view model.
| /// task is not the one that runs. These drive real searches and compare the results, which is the only | ||
| /// way to show the dialog is wired all the way through to the engines. | ||
| /// </summary> | ||
| [Category("LongRunning")] |
There was a problem hiding this comment.
[Category("LongRunning")] is filtered by nothing.
This category appears exactly twice in the repository — here and MainWindowTests.cs:78 — and the string LongRunning appears nowhere else: no workflow, no .runsettings, no csproj. The new job applies no --filter at all; the required job filters Category!=ExternalService.
So the attribute is currently decorative. Either wire it into a filter or drop it, so it doesn''t read as protection that isn''t there.
In fairness this costs almost nothing today — the legs run 35/35 in ~16 s, whole jobs 1m17s–3m18s against 37 minutes for the required Windows job. I''m flagging the misleading name, not a CI cost.
(For contrast, the repo''s existing categories are used: PlotModelStat 23 times, ExternalService 3, UniProt 1 — and ExternalService is genuinely filtered in three places.)
There was a problem hiding this comment.
🤖 Fixed in 9d2d3f6d — dropped the attribute rather than inventing a filter for it. Nothing filtered it, the suite runs in seconds, and a category that reads as protection while providing none is worse than no category.
| [Test] | ||
| public void DefaultSettingsReproduceTheKnownPsmCount() | ||
| { | ||
| Assert.That(RunSearch(_ => { }), Is.EqualTo(22), |
There was a problem hiding this comment.
The default baseline is recomputed from scratch in each of the three tests here (:70, :77, :93), so this fixture runs 6 full EverythingRunnerEngine searches per platform leg — 18 per push across the matrix — where 4 would do.
Hoisting the baseline to a [OneTimeSetUp] removes two searches immediately.
Two related nits in the same file: RunSearch deletes the output folder in its finally (:54-60), so a search that fails in CI leaves nothing to diagnose; and the -1 sentinel at :52 means a total search failure still satisfies Is.LessThan(baseline) at :96, so a broken run reads as a passing test.
There was a problem hiding this comment.
🤖 Fixed in 9d2d3f6d, all three points. The baseline is hoisted to [OneTimeSetUp] (six searches per leg down to four); RunSearch now fails instead of returning -1, since as you spotted -1 satisfied every Is.LessThan below it so a totally broken run read as a pass; and the output folder is kept when the run fails, which is the only time anyone wants it.
| viewModel.RunCommand.Execute(null); | ||
|
|
||
| // RunCommand is async; wait for it rather than sleeping a fixed amount. | ||
| DateTime deadline = DateTime.UtcNow.AddMinutes(10); |
There was a problem hiding this comment.
A hard 10-minute wall-clock deadline is likely to flake on a loaded macOS runner.
In practice the search completes in about 2 seconds, so the margin is enormous today — but hosted macOS runners are the slowest and most contended of the three, and this is the kind of assertion that fails once a quarter for reasons unrelated to the code.
If the intent is "don''t hang forever", timeout-minutes on the job expresses that without coupling a test assertion to runner load.
There was a problem hiding this comment.
🤖 Fixed in 9d2d3f6d — timeout-minutes: 20 on the job expresses "don't hang", as you suggested, and the in-test bound is now a 30-minute backstop reporting Assert.Inconclusive rather than a failure, so runner load cannot read as a correctness problem.
… it does not show The Avalonia projects were written when master targeted net8.0 and still said so, as did the SDK pin in their CI job. Master is on net10.0, so both projects and the workflow now match. The more serious half: TaskSettingsViewModel.Apply() rebuilt CommonParameters through its constructor, passing the 13 settings this dialog shows. CommonParameters takes 44 arguments, so the other ~30 were silently restored to their defaults every time a user pressed OK - deconvolution parameters, windowing, separation type, partition count. Several of those change search results, so the loss was invisible in the GUI and visible only in the output. Apply() now goes through a new CommonParameters.CloneWithNewValues, a sibling of the existing CloneWithNewDissociationType and CloneWithNewTerminus. It copies every setting via Clone() and overrides only what was passed, so a property added to CommonParameters later is carried through without this having to know about it. The existing tests asserted that edited values arrive; none asserted that unedited ones survive, which is where the defect lived. ApplyPreservesSettingsTheDialogDoesNotShow closes that: it sets five non-default values the dialog never shows, edits a tolerance, and requires all five to survive. Verified it fails against the constructor version - 1 failed of 36 - and passes with the clone. 36 pass on macOS arm64. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sibling of the CommonParameters fix in 4ab29d9, and the same defect: Apply() rebuilt DigestionParams through its constructor, passing the 6 settings this dialog shows. DigestionParams takes 12, so the other 6 went back to their defaults on every Save. Three consequences, in descending severity: - searchModeType was dropped, so a non-specific or semi-specific search silently became CleavageSpecificity.Full. fragmentationTerminus went with it. Both change results and neither is visible in the dialog. - initiatorMethionineBehavior was dropped, which 4 of the 5 WPF windows preserve, along with the SILAC and glycopeptide flags. - LoadFrom read Protease rather than SpecificProtease. For a non-specific task Protease holds singleN/singleC and SpecificProtease holds the user's actual choice, so the enzyme box showed singleN. Combined with the above, a round trip both lost the mode and misreported the enzyme. An RNA task fared worse than the review predicted. LoadFrom's type test was `is DigestionParams`, which RnaDigestionParams fails, so Protease kept its "trypsin" default and Apply replaced the RnaDigestionParams with a proteolytic one - no exception, just an RNA task quietly turned into a protein search. BuildDigestionParams now branches on the interface and carries the rnase through; the enzyme box is filled from ProteaseDictionary and has nothing to offer an RNA task. Tests: ApplyWithoutEditingChangesNothing is the general form - load a task holding non-default values, Save without editing, and require every CommonParameters property to compare equal by reflection, so a setting added later is covered without the test knowing about it. Three explicit tests name the digestion settings, the non-specific round trip and the RNA case so a failure says which one broke. Verified all four fail against the unfixed view model and pass with it. 40 pass on macOS arm64, up from 36. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cancel() set GlobalVariables.StopLoops and nothing lowered it again. EverythingRunnerEngine.Run() never touches it and the TaskLayer and EngineLayer sites only read it, so the flag stayed raised for the life of the process: one Cancel, and every later run was broken until the app was restarted. The WPF GUI does both halves - MainWindow.xaml.cs:843 raises it on cancel and :896 lowers it as the first statement of RunAllTasks_Click, before its own validation. RunAsync now matches that ordering. The symptom is worse than "no results". With the flag still raised the search starts, an engine bails out early leaving a collection unset, and the task dies with ArgumentNullException (Parameter 'source') - so the second run fails loudly but for a reason that points nowhere near the cancel that caused it. ASearchAfterACancellationStillProducesResults cancels, then runs a real search and requires results, rather than reading the flag back - the flag being false is not the property worth protecting. Verified it fails against the unfixed view model with exactly the ArgumentNullException above, and passes with the fix. TearDown lowers the flag too, since it is static and would otherwise leak into whichever test ran next. Also hoisted the run-completion wait the two end-to-end tests now share. 41 pass on macOS arm64, up from 40. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nvariantly
Nine review findings with a common shape: the Avalonia GUI re-derived
things the engine already owns, and got them slightly wrong.
Culture. The project had no CultureInfo at all, against 157 InvariantCulture
uses in the WPF task windows with no counter-examples. Both failure modes
are real and now covered by a test: on de-DE, "." is the group separator, so
double.TryParse("0.05") returns true with the value 5.0 - a product
tolerance a hundred times looser than the one typed, silently. On fr-FR the
parse fails and BuildTolerance substitutes its 20 ppm fallback without
telling anyone. Verified both against the unfixed parse; the test covers
de-DE, fr-FR and en-US.
Accepted formats. IsSupportedSpectraFile hard-coded four extensions,
dropping .msalign, .tdf and .tdf_bin, and AddDatabases filtered nothing at
all. Both now go through GlobalVariables.AcceptedSpectraFormats /
AcceptedDatabaseFormats with GetFileExtension, so .msp and .msl spectral
libraries load and a format added to the engine is picked up here for free.
Bruker. A .d is a folder, and a user who opens one picks a file inside it.
CMD and the WPF windows both map that back to the parent folder; this did
not, so the selection was dropped. ToBrukerFolderIfInside does it now.
File pickers. Patterns are projected from the same canonical lists rather
than written out. That fixes a Linux-only bug that could not reproduce on
the developer's machine: Avalonia's FreeDesktop backend passes patterns to
the XDG portal as GlobStyle globs and portal matching is case-sensitive, so
"*.mzML" hid sample.mzml on the one platform this GUI exists for. .d is
excluded from the file picker, since OpenFilePickerAsync cannot select a
directory - the .tdf remap above is how those get added.
Start-up. Constructing the view model reads the modification, protease and
glycan databases off disk. A missing or malformed data directory threw
straight out of OnFrameworkInitializationCompleted and killed the process
before any window existed - no message, on exactly the platforms whose
directory layout is most likely to differ. App now catches it and shows the
window with the reason in the log, with running disabled.
Also: .WithInterFont(), so a minimal Linux image with no fontconfig renders
text rather than blank boxes; the score cutoff threshold matched to
TaskValidator.CheckMinScoreAllowed (>= 1, not > 0) so the two GUIs agree on
what is valid; and the contaminant filename heuristic moved to
DbForTask.LooksLikeContaminant. That last one is additive - CMD and
ProteinDbForDataGrid still carry their own copies, including CMD's
culture-sensitive ToUpper(), and collapsing those touches shipping code that
does not belong in this PR.
Tests: 59 pass on macOS arm64, up from 41, and the suite went from 11s to
4s - a [SetUpFixture] now loads GlobalVariables once for the assembly
instead of four fixtures reloading ~4,300 modifications per test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…xpensive
The root namespace was MetaMorpheus.Avalonia, which shadows the toolkit's
own Avalonia root. Inside it, a fully-qualified Avalonia.Point binds to
MetaMorpheus.Avalonia.Point and fails with
CS0234: The type or namespace name 'Point' does not exist in the
namespace 'MetaMorpheus.Avalonia'
an error naming the wrong namespace, fixable only with global::. Verified by
reproducing it: a throwaway file in the old namespace referencing
Avalonia.Point and Avalonia.Media.Colors produced exactly that pair of
errors, and the same file compiles clean under the new name.
Nothing hit this because every file happens to carry a using directive above
its namespace declaration. That is luck, and it runs out at the MetaDraw
port, which the PR description measures at ~600 references to Avalonia types
across 36 files. Renaming afterwards would mean touching all of them again,
so this is cheaper now than at any later point.
MetaMorpheusAvalonia, matching the repo's single-token convention -
GUI.csproj uses MetaMorpheusGUI for both RootNamespace and AssemblyName, so
this follows for both. Renaming the assembly is free today because nothing
packages it: MetaMorpheusSetup.wixproj harvests only CMD, EngineLayer, GUI
and TaskLayer, and the release job zips CMD alone. It would not be free
after a release artifact exists.
The test namespace had the identical defect and is renamed too, Test.Avalonia
to Test.AvaloniaGui.
ToolkitNamespaceIsNotShadowed keeps this from coming back: two
fully-qualified toolkit references with no using directive, which stop
compiling if the root namespace ever contains a member called Avalonia
again. A compile-time guard rather than a test, since that is what the
failure is.
Note build.yml's arm64 PE-header assertion still derives the DLL name from
the project name, so it needs project:assembly pairs to cover this project -
the assembly is MetaMorpheusAvalonia, the project GUI.Avalonia. Unchanged
here; that finding is still open.
59 tests pass on macOS arm64.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ean something
Closes the UI-surface, threading, modification, test and CI clusters from the
review. Marshalling comes first in the diff because the engine events could
not be subscribed until it was right.
MARSHALLING. OnUiThread tested Dispatcher.UIThread for null, which is never
null in Avalonia 12, then Post()ed. With no Avalonia platform the dispatcher
is built over ManagedDispatcherImpl and binds to whichever thread first
touches it, so anything raised from another thread queued onto a dispatcher
nobody pumps and vanished. Engines raise their events from inside
Parallel.For, so that is most of them. Capturing the SynchronizationContext
at construction settles it: Avalonia installs one on the UI thread, and its
absence is exactly the case where inline is correct.
ENGINE EVENTS. MetaMorpheusEngine's three events were never subscribed, so
inner-engine progress and engine warnings never appeared - the progress bar
sat still for the whole of a search. Added those, MyFileManager.WarnHandler
(warnings about the spectra files themselves, the first thing a new platform
trips over), and the EverythingRunnerEngine events that report task chaining
swapping the database and spectra lists mid-run.
Gate C is a reflection test over every public static event on the four engine
types, each of which must be subscribed or listed as a deliberate exclusion
with a reason. It immediately found two I had missed -
MetaMorpheusEngine.StartingSingleEngineHander and FinishedSingleEngineHandler
- which is the point of writing it that way.
UI. The description claimed six task types and modification selection; the
window offered three task types and no modification UI at all, so
ModificationSelectionViewModel was reachable only from tests and three
Apply() arms were dead. There are now six buttons, a settings section per
task type, and a grouped filterable modification picker. GPTMD's
ListOfModsGptmd gets the same widget on a third, non-exclusive axis, since
that is the one setting a GPTMD task really has. The contaminant column is
editable and there is an "Add default contaminants" button, so the filename
heuristic is a guess the user can overrule rather than a verdict.
MODIFICATIONS. Exclusivity moved into ModificationChoice's setters, so
binding a CheckBox straight to IsFixed can no longer produce the
both-fixed-and-variable state the rule exists to prevent. ResetToDefaults
reads CommonParameters instead of repeating its literals, which also makes
it correct for RNA.
Two corrections to the review here, both settled by measurement rather than
argument. First, the dedup does not collapse distinct terminal variants: of
84 colliding (ModificationType, IdWithMotif) identities in the shipped
databases, zero differ in location restriction, mass or target - all 84 are
duplicate glycan entries, 64 O-linked and 20 N-linked. Collapsing is also
forced, because that pair is the identity TOML stores, so two modifications
sharing it cannot be selected apart at any price. Kept, with a test that
fails if a database change ever makes a collapsed pair genuinely distinct.
Second, and not in the review: the picker only ever offered
GlobalVariables.AllModsKnown, but RNA modifications live in the separate
AllRnaModsKnown, so an RNA task could not select any of its modifications.
Found while writing the RNA defaults test, which failed for that reason.
TESTS. Dropped [Category("LongRunning")], which nothing filtered and which
read as protection that was not there. The wall-clock deadline is no longer
an assertion about runner speed - timeout-minutes on the job says "do not
hang", and an over-run is Inconclusive rather than a failure. The baseline
search is hoisted to [OneTimeSetUp], taking the fixture from six searches per
leg to four. RunSearch now fails instead of returning -1 when no results are
written, because -1 satisfied every Is.LessThan below it, so a totally broken
run read as a pass; and it keeps the output folder when the run failed, which
is the only time anyone wants it.
CI. Coverage is collected from the Avalonia job under its own flag - without
it the new files sat outside the denominator, which is why Codecov reported a
zero-line delta on a four-thousand-line PR. build.yml's arm64 PE-header
assertion takes project:assembly pairs, so it now covers the desktop app
Apple Silicon users actually launch; deriving the file name from the project
name looked for GUI.Avalonia.dll, which does not exist. The Avalonia job uses
env.DOTNET_VERSION rather than a fourth hard-coded SDK version, and the two
new solution entries use the SDK-style project GUID like the other seven.
Still outstanding and not fixable from a fork: the three Avalonia contexts
are not required checks. Branch protection lists only "Test on Windows", so a
red Avalonia leg still merges silently. That needs a maintainer.
83 tests pass on macOS arm64, up from 59. Solution restore verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Checked each of the review's remaining minor items against the WPF GUI before touching anything, because fixing a shared defect in one front end only makes the two diverge further. Three of the four are this project's alone; the fourth is not a GUI defect at all and is left alone deliberately. CustomMdac. The combo box offered MassDiffAcceptorType.Custom and Apply() never wrote SearchParameters.CustomMdac, so the choice was made and silently ignored. SearchTaskWindow.xaml.cs has always handled it - loads at :439, validates at :732, writes at :748 - so this was ours. Now carried, shown in the dialog, and validated through SearchTask.GetMassDiffAcceptor so an unreadable expression is refused here instead of failing part-way into a run. The log. `Log += line` reallocated the whole document per line, quadratic over the thousands a search emits, and after the marshalling change it also ran on engine worker threads, making it a racy read-modify-write. Now a StringBuilder behind a lock. The WPF window does not have this: notificationsTextBox .AppendText() appends incrementally and it marshals through Dispatcher.CheckAccess first. The null dereference. CreateSettingsFor can return null and the caller used it unguarded. WPF has the same shape - its switch on taskType leaves dialog null and MainWindow.xaml.cs:2058 assigns dialog.Owner regardless - but MyTask is an exhaustive enum there, so it is unreachable. Here the value comes from a ListBox item, so it is reachable, and it is now guarded. Not touching the WPF twin: it is dead code in shipping software, and this PR is not the place. NOT FIXED, on purpose: MaxThreadsPerFile losing its -1 sentinel. This is not a GUI defect in either front end. CommonParameters' constructor resolves -1 to ProcessorCount - 1 at construction, so the sentinel is gone before any GUI sees it; SearchTaskWindow.xaml.cs:343 displays the resolved number too, and CMD is no different. The checked-in vignette TOMLs say "MaxThreadsToUsePerFile = 3", which is the machine that generated them. Preserving the sentinel means changing CommonParameters to resolve at use rather than at construction, which changes every TOML MetaMorpheus writes and wants its own issue and its own decision - not a quiet edit inside a GUI PR. 87 tests pass on macOS arm64, up from 83. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 Thanks — this was an unusually useful review, and most of it is now addressed. 29 of the 33 inline Four things are worth calling out, because in each case doing the work changed the answer. The modification dedup finding does not reproduce, and the measurement is the argument. You A gap neither of us spotted, found while fixing The RNA digestion case failed more quietly than predicted. You expected
On sequencing: the .NET 10 rebase landed first, as you argued. Gate A now has a real test — a Two behavioural forks from the WPF GUI, now stated in the description as you asked: fixed and What I need from a maintainer: the three Two things left open deliberately. |
…2 warning Coverage. Adding --collect:"XPlat Code Coverage" to the Avalonia job was not enough: the collector ships in coverlet.collector, which GUI.Avalonia.Tests did not reference. Without it the run prints "Unable to find a datacollector with friendly name 'XPlat Code Coverage'" on a line nobody reads, exits 0, and uploads nothing - so codecov/patch reported no coverage for a PR whose tests cover it well. Referenced in the csproj rather than added by the workflow, as the Windows job does, so a local run collects coverage too. The real figure, now that it is measured: 85.6% line coverage on MetaMorpheusAvalonia across the 87 tests. The code was covered; the reporting was broken, which is worse than a low number because it looks like a low number. CS8632. CommonParameters.DIAparameters carries a reference-type ? annotation with no annotations context, so it only produced a warning. This is pre-existing on master, not introduced here - it surfaces on this PR because the file is touched. Scoped the annotations context to that one member rather than the whole file, so the nullability metadata of every other member is unchanged for consumers that do enable nullable. The int?/double? properties above it are Nullable<T> and were never affected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
🤖
Making the MetaMorpheus GUI usable on macOS and Linux
MetaMorpheus's engine is cross-platform and the CLI runs anywhere, but the GUI is WPF and
therefore Windows-only. Users on macOS and Linux can run searches, but only by hand-writing
TOML — there is no way to build a task list, adjust settings, or see progress.
This adds an Avalonia front end that runs on all three platforms. It is deliberately additive:
GUIandGuiFunctionsare untouched, and the WPF GUI remains the Windows experience. If thisdirection is not wanted, deleting the two new directories reverts it.
Opened as a draft for a direction decision rather than for merge. That decision has now been
given (@trishorts, converge rather than coexist), and the review's line-level findings are addressed —
see the update below.
What works
EverythingRunnerEngine,live progress and log output
each reachable from its own button
plus GPTMD's
ListOfModsGptmdon the same widget87 tests, headless via
Avalonia.Headless.NUnit, running on Linux, macOS and Windows. Some run realsearches and assert the defaults still yield 22 PSMs, so a settings regression that quietly changes
results fails the build instead of being discovered later.
Two implementation notes for review:
MetaMorpheusEngine's static events, so it isIDisposableandunsubscribes. Without that, tests passed individually and failed in a suite, because each
surviving view model kept handling the next one's events.
Dispatcher.UIThread.Why Avalonia
Avalonia is the closest thing to WPF that also runs on Linux and macOS: same XAML dialect, same
MVVM and data-binding model, same attached-property pattern. That matters concretely for the port
ahead — every WPF type MetaDraw uses (
Canvas,TextBlock,Ellipse,Rectangle,Panel,Point,Color,Colors,SolidColorBrush,Key,Thickness,FontFamily, andCanvas.SetLeft/SetTop) exists in Avalonia under the same name with the same shape. I verifiedthis by compiling all of them against Avalonia 12.1.1 rather than assuming it.
The practical consequence is that most of the remaining work is namespace translation rather than
rewriting, and that WPF experience transfers directly. The alternatives are worse fits: MAUI has no
real Linux story, and Uno or a web front end would mean learning a genuinely different model.
arm64
Native Apple Silicon works as of master: #2701 made CMD, EngineLayer and TaskLayer
architecture-neutral and #2704 brought in mzLib 1.0.584. I had these as a commit here and have
dropped it as redundant. Verified on this branch: all 87 tests pass on native arm64.
(#2503 and #2504 both still appear open, though the fixes for them have landed.)
Not included
MetaDraw, file-specific parameters, and roughly 25 auxiliary dialogs.
Two deliberate differences from the WPF GUI
Both were raised in review and are stated here rather than left to be inferred:
independently with no cross-check (
SearchTaskWindow.xaml.cs:567-577); the only follow-up,TaskValidator.VariableModCheck, merely warns when more than one variable mod is selected. Amodification submitted as both makes the search treat that residue inconsistently.
ValidModificationfiltering applies to every task type, where WPF applies it only in Search.I think both are improvements, but they mean the two GUIs build different tasks from identical user
actions. I will open an issue against the WPF side so they converge rather than drift.
CI
Both projects are added to the solution, so the existing build job compiles them on
ubuntu/macos/windows, and a new job runs the 87 tests on all three, with coverage collected under its own flag.
Note for whoever reviews the solution diff:
dotnet sln addmapsUbuntuMactoDebug, and forGUI.Avaloniait omittedBuild.0entirely, which makes the solution build "successfully" whilesilently skipping the project. The mappings here are set by hand to match
GuiFunctions.Finishing the job, including MetaDraw
MetaDraw is the biggest remaining piece and it should be part of any port — a cross-platform GUI
without it would not be a real replacement. Having measured it, I think it is very doable.
It is ~36 files in
GuiFunctionswith about 600 references to WPF types:SolidColorBrush/Color/Colors/BrushAvalonia.MediaPointAvalonia.PointKeyAvalonia.Input.KeyCanvas/TextBlock/Ellipse/Rectangle/PanelICommandOnly one of those files is genuinely a WPF control (
ChimeraLegendCanvas); the other 35 are logicand view models that happen to reach for WPF types.
How hard this is depends entirely on the "replace or coexist" question below:
code. The 600 references map essentially 1:1.
the colour and geometry types with something both toolkits can consume and adding converters on
the WPF side. That is a public API change and real work.
Either way, one preliminary is worth doing on its own merits: extracting the UI-free part of
GuiFunctionsinto anet8.0project. I have this working locally — 25 files move with no APIchange, and it makes 177 of the existing tests run on macOS, which today are Windows-only purely
because
GuiFunctionstargetsnet8.0-windows. I would offer that as a separate PR whichever waythis one goes.
The remaining known dependency is OxyPlot: MetaDraw is on 2.0.0, and the live Avalonia path needs
2.2.0 (
OxyPlot.Avalonia2.1.0 pins Avalonia 0.10.11 and is effectively abandoned; the maintainedoption is
OxyPlot.SkiaSharp). I would do that upgrade in the WPF GUI first, so any plottingregression is attributable to the upgrade rather than to the port.
Releasing it
The current release job is Windows-only: it patches the version from the tag, builds the WiX MSI,
zips the CLI, and uploads with
softprops/action-gh-release@v2. The GUI would slot in as anadditional matrix job on the same tag trigger:
dotnet publish -c Release --self-containedforlinux-x64,osx-arm64,osx-x64andwin-x64, so users need no .NET install.appbundle (Info.plist, thenditto -c -k --keepParentto preserve thebundle structure) — a bare binary will not launch from Finder
Trade-off worth deciding explicitly: self-contained builds are large (roughly 80–150 MB each), so
this materially increases release size. Framework-dependent builds are ~10× smaller but require the
user to install .NET first, which is most of the friction we are trying to remove. Unsigned macOS
builds are also Gatekeeper-quarantined and need either a documented right-click-open or
notarisation, which requires an Apple Developer account.
Questions
that changes the most downstream: replacing makes the remaining port mostly mechanical, while
coexisting requires making the shared code UI-neutral.
Update since review (six commits)
🤖 29 of the 33 inline findings are fixed, plus the
build.ymlitem that had no line to attach to.Tests: 35 → 87, all passing on macOS arm64. Detail is in the review replies; the parts that changed
an answer rather than just closing a finding:
Apply()fidelity (Gate A).CommonParameterswas losing ~30 of its 42 settings per Save andDigestionParams6 of its 12. Both now clone-and-override. Gate A is a reflective load/Save/compareover every
CommonParametersproperty, so a setting added later is covered without the test knowingabout it. Verified failing against the unfixed code.
OnUiThreadtestedDispatcher.UIThreadfor null, which isnever null in Avalonia 12, and then
Posted to a dispatcher nobody pumped. Fixed first, thenMetaMorpheusEngine's three events,MyFileManager.WarnHandlerand the task-chaining events weresubscribed. Gate C is a reflection test requiring every static engine event to be handled or listed
as a deliberate exclusion with a reason; it found two I had missed.
CultureInfoat all. Reproduced against the unfixed parse: on de-DE"0.05"reads as 5.0, and on fr-FR the parse fails and silently substitutes the fallback.MetaMorpheusAvalonia. Confirmed the hazard first by reproducingCS0234: 'Point' does not exist in the namespace 'MetaMorpheus.Avalonia'; there is now acompile-time guard so it cannot return. Cheaper now than after MetaDraw's ~600 references.
variants. Of 84 colliding
(ModificationType, IdWithMotif)identities in the shipped databases,zero differ in location, mass or target — all are duplicate glycan entries. The collapse is also
forced, since that pair is the identity TOML stores. Kept, with a test that fails if that stops
being true.
GlobalVariables.AllModsKnown, but RNAmodifications live in
AllRnaModsKnown— so an RNA task could not select any of its ownmodifications.
Still open
Avalonia GUI tests (...)contexts are not required checks(branch protection lists only
Test on Windows), so a red Avalonia leg merges silently. A fork PRcannot change this.
CMDon the same TOML) is not written yet.IsThermoRawUnsupportedHerestill hard-codes the restriction, pending Docs: Thermo .raw reading works on macOS, not just Windows and Linux #2723.MaxThreadsPerFilelosing its-1sentinel is not a GUI defect:CommonParameters' constructorresolves it at construction, WPF displays the resolved number too, and the checked-in vignette TOMLs
say
MaxThreadsToUsePerFile = 3. Preserving it would change every TOML MetaMorpheus writes, so itwants its own issue.
GuiFunctionsextraction is still recommended as a separate PR ahead of further GUI work.