From 9453e20cfc6abb093508ced5c82026aeb11ae047 Mon Sep 17 00:00:00 2001 From: Alberto Balbo Date: Wed, 5 Aug 2026 15:18:39 +0200 Subject: [PATCH] Corregge il crash in chiusura e rende recuperabile una strategia rimossa MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chiusura. Il gestore annullava la chiusura, aspettava lo spegnimento del motore e la richiedeva alla fine. Un secondo clic sulla X durante l'attesa usciva pero' SENZA annullare: la finestra entrava nella propria sequenza di chiusura e la Close() del primo tentativo ci finiva dentro, sollevando "non e' possibile chiamare Close durante la chiusura di un oggetto Window". Ora ogni tentativo successivo viene annullato e la chiusura vera si rimanda a un frame nuovo del dispatcher, cosi' non puo' mai eseguire dentro il gestore. La correzione ovvia — annullare tutto quando lo spegnimento e' in corso — sarebbe stata peggiore del difetto: la chiusura finale ripassa dallo stesso gestore, veniva annullata anche lei e la finestra non si chiudeva piu'. Passa solo quella, riconosciuta da un flag alzato prima di chiamarla. Strategia. Un aggiornamento che rimuove una strategia lascia il suo nome nella configurazione dell'utente, perche' l'installazione la conserva — ed e' giusto, le tarature sono sue. Il campo pero' era di sola lettura: l'unica via d'uscita era modificare il JSON a mano. Ora e' un elenco a discesa che propone solo cio' che il programma sa costruire, quindi non ci si puo' scrivere un nome inesistente, e un valore obsoleto si segnala da se' all'apertura della pagina invece di aspettare che qualcuno prema AVVIA. All'avvio l'applicazione lo dice e porta in Impostazioni. Stessa cosa per gli altri campi a insieme chiuso — barre, tipo di ordine, livello del registro e i booleani — che ora si scelgono e non si scrivono. Trovato provando il giro completo a video: applicare due volte lo stesso lotto di modifiche falliva con "The node already has a parent", perche' la pagina lo applica prima a una copia temporanea per validarlo e poi al file vero, e un JsonNode appartiene a un albero solo. ConfigWriter ora clona. Co-Authored-By: Claude Opus 5 --- Encelado/Directory.Build.props | 7 +- Encelado/Modifiche.txt | 6 +- .../Encelado.Bot/Configuration/BotConfig.cs | 11 +- .../Configuration/ConfigWriter.cs | 8 + Encelado/src/Encelado.Bot/MainWindow.xaml.cs | 99 ++++- .../Encelado.Bot/Ui/Pages/SettingsPage.xaml | 29 +- Encelado/src/Encelado.Bot/Ui/SettingField.cs | 38 +- .../src/Encelado.Bot/Ui/SettingsCatalogue.cs | 30 +- Encelado/src/Encelado.Bot/Ui/Theme.xaml | 8 +- Encelado/tests/Encelado.Tests/EngineTests.cs | 7 +- .../tests/Encelado.Tests/SettingsFormTests.cs | 26 ++ .../ShutdownAndStrategyTests.cs | 355 ++++++++++++++++++ 12 files changed, 603 insertions(+), 21 deletions(-) create mode 100644 Encelado/tests/Encelado.Tests/ShutdownAndStrategyTests.cs diff --git a/Encelado/Directory.Build.props b/Encelado/Directory.Build.props index 26c7be0..70c0ab5 100644 --- a/Encelado/Directory.Build.props +++ b/Encelado/Directory.Build.props @@ -13,7 +13,12 @@ false Encelado Encelado - 3.2.0 + + 3.5.0 + ToolTip="{Binding FullTooltip}" + Visibility="{Binding IsFreeText, Converter={StaticResource BoolVis}}"> + + + Choices { get; init; } = []; + /// + /// I valori fra cui si può scegliere, o vuoto se il campo è a testo libero. + /// + /// I booleani entrano qui da sé: "sì" e "no" scritti a mano sono due modi per + /// sbagliare, e nessuno dei due aggiunge niente rispetto a sceglierli. + /// + /// + public IReadOnlyList Options => Kind switch + { + SettingKind.Choice => Choices, + SettingKind.Boolean => ["sì", "no"], + _ => [], + }; + + /// Vero se il campo si compila da un elenco. Vedi . + public bool IsList => Options.Count > 0; + + /// Vero se il campo si scrive. È l'opposto di . + public bool IsFreeText => !IsList; + + /// + /// Vero quando il valore salvato non è più fra quelli ammessi — tipicamente dopo un + /// aggiornamento che ha tolto una strategia. Il campo resta visibile con il suo + /// errore, ma l'elenco non lo ripropone: da lì si esce solo scegliendo un valore + /// che esiste. + /// + public bool IsObsolete => IsList && !IsReadOnly && + !Options.Contains(_value, StringComparer.OrdinalIgnoreCase); + /// Unit suffix shown after the box, e.g. "%" or "secondi". public string Suffix { get; init; } = string.Empty; @@ -87,6 +116,7 @@ public sealed class SettingField : INotifyPropertyChanged _value = value; Raise(); Raise(nameof(IsDirty)); + Raise(nameof(IsObsolete)); Validate(); } } @@ -121,9 +151,13 @@ public sealed class SettingField : INotifyPropertyChanged { _value = value; Original = value; - Error = null; Raise(nameof(Value)); Raise(nameof(IsDirty)); + + // Validato subito, non solo quando qualcuno lo tocca: un valore diventato non + // valido perché l'aggiornamento ha tolto una strategia deve segnalarsi da sé + // all'apertura della pagina, non restare lì con l'aria di andare bene. + Validate(); } public void Revert() => Load(Original); @@ -176,7 +210,7 @@ public sealed class SettingField : INotifyPropertyChanged case SettingKind.Choice: if (Choices.Count > 0 && !Choices.Contains(_value, StringComparer.OrdinalIgnoreCase)) { - Error = $"valori ammessi: {string.Join(", ", Choices)}"; + Error = $"'{_value}' non è più disponibile — scegli fra: {string.Join(", ", Choices)}"; } break; diff --git a/Encelado/src/Encelado.Bot/Ui/SettingsCatalogue.cs b/Encelado/src/Encelado.Bot/Ui/SettingsCatalogue.cs index 5421bbd..b857ef4 100644 --- a/Encelado/src/Encelado.Bot/Ui/SettingsCatalogue.cs +++ b/Encelado/src/Encelado.Bot/Ui/SettingsCatalogue.cs @@ -1,5 +1,6 @@ using System.Globalization; using Encelado.Bot.Configuration; +using Encelado.Core.Strategies; namespace Encelado.Bot.Ui; @@ -21,7 +22,7 @@ public static class SettingsCatalogue { ArgumentNullException.ThrowIfNull(config); - return + SettingGroup[] gruppi = [ Strategy(config, strategyName), Sizing(config), @@ -30,6 +31,17 @@ public static class SettingsCatalogue Engine(config), Logging(config), ]; + + // Validati subito, non alla prima modifica. Un valore diventato non valido + // perché un aggiornamento ha tolto una strategia deve segnalarsi da sé + // all'apertura della pagina: aspettare che qualcuno lo tocchi significa non + // dirglielo mai, visto che è proprio il campo che nessuno guarda. + foreach (SettingField campo in gruppi.SelectMany(static g => g.Fields)) + { + campo.Validate(); + } + + return gruppi; } // ------------------------------------------------------------------------ @@ -64,21 +76,29 @@ public static class SettingsCatalogue "mai stati validati su dati ETH.", }); + // Scelta e non sola lettura, anche se l'elenco ha una voce sola. Un + // aggiornamento che toglie una strategia lascia nel file dell'utente un nome + // che non esiste più, e con il campo bloccato l'unico modo per uscirne sarebbe + // aprire il JSON a mano. L'elenco propone solo ciò che l'applicazione sa + // costruire, quindi da qui non si può scrivere un nome sbagliato. g.Fields.Add(new SettingField { Path = "symbols[0].strategy", Label = "Strategia", Initial = strategyName, - Kind = SettingKind.Text, - IsReadOnly = true, - ReadOnlyReason = "ne esiste una sola. Le altre sono state cancellate, non disattivate.", + Kind = SettingKind.Choice, + Choices = StrategyFactory.Available, Tooltip = "Il modello che genera i segnali.\n\n" + "'trend-filter' compra quando il prezzo sta sopra la media a 100 giorni di una " + "certa percentuale, e vende quando scende sotto della stessa percentuale. Nient'altro: " + "nessun trailing stop, nessun target, nessun filtro di volatilità.\n\n" + "Sette modelli più elaborati sono stati scritti e misurati prima di questo. Tutti " + - "hanno perso, o contro il mercato o contro il semplice comprare e tenere.", + "hanno perso, o contro il mercato o contro il semplice comprare e tenere: per questo " + + "l'elenco ne contiene una sola.\n\n" + + "Se qui compare un errore, la configurazione porta il nome di una strategia rimossa " + + "da un aggiornamento — succede perché l'installazione conserva il tuo encelado.json. " + + "Scegli quella disponibile e salva.", }); g.Fields.Add(new SettingField diff --git a/Encelado/src/Encelado.Bot/Ui/Theme.xaml b/Encelado/src/Encelado.Bot/Ui/Theme.xaml index b0de6ae..0e41cde 100644 --- a/Encelado/src/Encelado.Bot/Ui/Theme.xaml +++ b/Encelado/src/Encelado.Bot/Ui/Theme.xaml @@ -223,9 +223,13 @@ RelativeSource={RelativeSource TemplatedParent}}"> + + BorderBrush="{Binding BorderBrush, + RelativeSource={RelativeSource AncestorType=ComboBox}}" + BorderThickness="1" CornerRadius="7"> diff --git a/Encelado/tests/Encelado.Tests/EngineTests.cs b/Encelado/tests/Encelado.Tests/EngineTests.cs index 8355f12..a8276d9 100644 --- a/Encelado/tests/Encelado.Tests/EngineTests.cs +++ b/Encelado/tests/Encelado.Tests/EngineTests.cs @@ -188,7 +188,12 @@ public class ConfigValidationTests config.Symbols[0].Strategy = "moon-phase"; InvalidOperationException ex = Assert.Throws(config.Validate); - Assert.Contains("unknown strategy", ex.Message, StringComparison.OrdinalIgnoreCase); + + // Il messaggio deve nominare il colpevole, le alternative e la via d'uscita: + // questo errore lo incontra chi aggiorna, non chi sviluppa. + Assert.Contains("moon-phase", ex.Message, StringComparison.Ordinal); + Assert.Contains("trend-filter", ex.Message, StringComparison.Ordinal); + Assert.Contains("Impostazioni", ex.Message, StringComparison.Ordinal); } [Fact] diff --git a/Encelado/tests/Encelado.Tests/SettingsFormTests.cs b/Encelado/tests/Encelado.Tests/SettingsFormTests.cs index d923eda..b4335c0 100644 --- a/Encelado/tests/Encelado.Tests/SettingsFormTests.cs +++ b/Encelado/tests/Encelado.Tests/SettingsFormTests.cs @@ -161,6 +161,32 @@ public class ConfigPathWriterTests : IDisposable Assert.Equal(before, File.ReadAllText(path)); } + + [Fact] + public void LoStessoLottoSiPuoApplicareDueVolte() + { + // La pagina delle impostazioni lo fa sempre: una volta su una copia temporanea + // per validare, una sul file vero. Senza clonare, la seconda falliva con + // «The node already has a parent» — un JsonNode appartiene a un albero solo. + string primo = Write(Sample); + string secondo = Write(Sample); + + Dictionary modifiche = new() + { + ["symbols[0].strategy"] = JsonValue.Create("trend-filter"), + ["risk.stakePct"] = JsonValue.Create(0.5), + }; + + ConfigWriter.Apply(primo, modifiche); + ConfigWriter.Apply(secondo, modifiche); + + foreach (string percorso in new[] { primo, secondo }) + { + BotConfig c = ConfigLoader.Load(percorso, out _); + Assert.Equal("trend-filter", c.Symbols[0].Strategy); + Assert.Equal(0.5, c.Risk.StakePct, 9); + } + } } /// diff --git a/Encelado/tests/Encelado.Tests/ShutdownAndStrategyTests.cs b/Encelado/tests/Encelado.Tests/ShutdownAndStrategyTests.cs new file mode 100644 index 0000000..07b7041 --- /dev/null +++ b/Encelado/tests/Encelado.Tests/ShutdownAndStrategyTests.cs @@ -0,0 +1,355 @@ +using System.ComponentModel; +using System.Windows; +using Encelado.Bot.Configuration; +using Encelado.Bot.Ui; +using Encelado.Core.Strategies; + +namespace Encelado.Tests; + +/// +/// Chiudere una finestra mentre uno spegnimento asincrono è in corso. +/// +/// Il difetto: il gestore annullava la chiusura, aspettava lo spegnimento e poi +/// richiamava Close(). Un secondo clic sulla X durante l'attesa usciva dal +/// gestore senza annullare, la finestra entrava nella propria sequenza di +/// chiusura, e la Close() del primo tentativo ci finiva dentro: +/// «Non è possibile […] chiamare Close durante la chiusura di un oggetto Window». +/// +/// +[Collection("wpf")] +public class WindowShutdownTests +{ + /// + /// Il meccanismo esatto del difetto: Close() chiamata mentre la finestra è + /// dentro la propria sequenza di chiusura non è ammessa, e non lo è nemmeno dopo + /// aver annullato — l'annullamento vale per l'uscita dal gestore, non per il tempo + /// in cui il gestore sta girando. + /// + [Fact] + public void ChiudereDentroIlGestoreDiChiusuraSollevaEccezione() + { + WpfRunner.Run(() => + { + Window finestra = new(); + Exception? errore = null; + + finestra.Closing += (_, e) => + { + e.Cancel = true; + + try + { + // È qui che finiva la Close() del primo tentativo quando un secondo + // clic la faceva riprendere troppo presto. + finestra.Close(); + } + catch (InvalidOperationException ex) + { + errore = ex; + } + }; + + finestra.Close(); + + Assert.NotNull(errore); + Assert.Contains("Close", errore!.Message, StringComparison.OrdinalIgnoreCase); + }); + } + + /// + /// La correzione: la chiusura vera si rimanda a un frame nuovo del dispatcher, così + /// non può mai eseguire dentro il gestore. + /// + [Fact] + public void RimandarlaAUnFrameNuovoNonSollevaNiente() + { + WpfRunner.Run(() => + { + Window finestra = new(); + Exception? errore = null; + bool chiusa = false; + + finestra.Closing += (_, e) => + { + if (chiusa) + { + return; + } + + e.Cancel = true; + + _ = finestra.Dispatcher.BeginInvoke(new Action(() => + { + try + { + chiusa = true; + finestra.Close(); + } + catch (InvalidOperationException ex) + { + errore = ex; + } + })); + }; + + finestra.Close(); + + // Fa girare la coda del dispatcher fino a quando la chiusura rimandata è + // stata eseguita. + finestra.Dispatcher.Invoke(() => { }, System.Windows.Threading.DispatcherPriority.ApplicationIdle); + + Assert.Null(errore); + Assert.True(chiusa); + }); + } + + /// + /// La distinzione che serve fra i due tipi di chiusura: quelle dell'utente durante + /// lo spegnimento vanno annullate, quella finale no. + /// + /// Annullarle tutte è il modo ovvio di correggere il difetto originale, ed è + /// sbagliato: la chiusura finale ripassa dallo stesso gestore, viene annullata + /// anche lei, e la finestra non si chiude più. Il bot resta aperto per sempre — + /// un difetto peggiore di quello che si voleva correggere. + /// + /// + [Fact] + public void LaChiusuraFinalePassaMentreQuelleDellUtenteNo() + { + WpfRunner.Run(() => + { + Window finestra = new(); + bool inChiusura = false; + bool finale = false; + int annullate = 0; + + finestra.Closing += (_, e) => + { + if (inChiusura) + { + if (!finale) + { + annullate++; + e.Cancel = true; + } + + return; + } + + inChiusura = true; + e.Cancel = true; + }; + + finestra.Close(); // primo tentativo: prende in carico + finestra.Close(); // l'utente insiste + finestra.Close(); // e ancora + Assert.Equal(2, annullate); + + finale = true; + finestra.Close(); // la chiusura finale dello spegnimento + + Assert.Equal(2, annullate); + Assert.False(finestra.IsVisible); + }); + } + + [Fact] + public void UnaChiusuraAnnullataLasciaLaFinestraUtilizzabile() + { + WpfRunner.Run(() => + { + Window finestra = new(); + int tentativi = 0; + + finestra.Closing += (_, e) => + { + tentativi++; + e.Cancel = true; + }; + + finestra.Close(); + finestra.Close(); + finestra.Close(); + + Assert.Equal(3, tentativi); + }); + } +} + +/// +/// Una strategia rimasta in configurazione dopo che un aggiornamento l'ha rimossa. +/// +/// Capita perché l'installazione conserva l'encelado.json dell'utente — che è +/// giusto, le tarature sono sue — quindi un nome tolto dal programma sopravvive nel +/// file. Deve essere una cosa che si vede e si corregge, non un vicolo cieco. +/// +/// +public class StrategiaObsoletaTests +{ + private static BotConfig ConfigCon(string strategia) + { + BotConfig c = new(); + + // Validate() controlla le credenziali per prime: senza, il test si fermerebbe + // lì invece di arrivare al controllo sulla strategia. + c.Alpaca.KeyId = "PKTESTTESTTESTTESTTE"; + c.Alpaca.SecretKey = "segretosegretosegretosegretosegretosegre"; + + c.Symbols.Add(new SymbolConfig + { + Symbol = "BTC/USD", + Strategy = strategia, + Enabled = true, + Parameters = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["period"] = 100, + ["band"] = 0.02, + }, + }); + + return c; + } + + private static SettingField CampoStrategia(BotConfig config) => + SettingsCatalogue.Build(config, config.Symbols[0].Strategy) + .SelectMany(static g => g.Fields) + .First(static f => f.Path == "symbols[0].strategy"); + + [Fact] + public void IlCampoStrategiaSiSceglieDaUnElenco() + { + SettingField campo = CampoStrategia(ConfigCon("trend-filter")); + + Assert.True(campo.IsList, "la strategia deve essere un elenco, non un testo libero"); + Assert.False(campo.IsFreeText); + Assert.Equal(StrategyFactory.Available, campo.Options); + } + + [Fact] + public void LElencoProponeSoloCioCheIlProgrammaSaCostruire() + { + SettingField campo = CampoStrategia(ConfigCon("trend-filter")); + + Assert.All(campo.Options, static nome => + Assert.True(StrategyFactory.IsKnown(nome), $"'{nome}' è nell'elenco ma non è costruibile")); + } + + [Fact] + public void IlCampoStrategiaEModificabile() + { + // Bloccarlo perché la strategia è una sola è esattamente ciò che rendeva + // impossibile correggere un valore obsoleto senza aprire il JSON. + SettingField campo = CampoStrategia(ConfigCon("trend-filter")); + + Assert.False(campo.IsReadOnly); + Assert.True(campo.IsEditable); + } + + [Fact] + public void UnaStrategiaRimossaSiSegnalaDaSolaAllApertura() + { + SettingField campo = CampoStrategia(ConfigCon("adaptive-regime")); + + Assert.True(campo.HasError, "il campo doveva segnalare l'errore senza essere toccato"); + Assert.True(campo.IsObsolete); + Assert.Contains("adaptive-regime", campo.Error!, StringComparison.Ordinal); + Assert.Contains("trend-filter", campo.Error!, StringComparison.Ordinal); + } + + [Fact] + public void LElencoNonRipropoleUnaStrategiaRimossa() + { + SettingField campo = CampoStrategia(ConfigCon("adaptive-regime")); + + Assert.DoesNotContain("adaptive-regime", campo.Options); + } + + [Fact] + public void SceglierneUnaValidaRisolveEDiventaSalvabile() + { + SettingField campo = CampoStrategia(ConfigCon("adaptive-regime")); + Assert.True(campo.HasError); + + campo.Value = StrategyFactory.Default; + + Assert.False(campo.HasError); + Assert.False(campo.IsObsolete); + Assert.True(campo.IsDirty, "la scelta va salvata, quindi deve risultare modificata"); + Assert.Equal(StrategyFactory.Default, campo.ToJson()!.GetValue()); + } + + [Fact] + public void LaConfigurazioneSpiegaComeUscirneInveceDiDireSoloCheEInvalida() + { + InvalidOperationException ex = + Assert.Throws(() => ConfigCon("adaptive-regime").Validate()); + + Assert.Contains("adaptive-regime", ex.Message, StringComparison.Ordinal); + Assert.Contains("Impostazioni", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void UnaConfigurazioneValidaNonSiLamenta() + { + SettingField campo = CampoStrategia(ConfigCon(StrategyFactory.Default)); + + Assert.False(campo.HasError); + Assert.False(campo.IsObsolete); + Assert.False(campo.IsDirty); + } +} + +/// I campi con un insieme chiuso di valori non devono essere scrivibili. +public class CampiAScelaTests +{ + private static IReadOnlyList Campi() + { + BotConfig c = new(); + c.Symbols.Add(new SymbolConfig { Symbol = "BTC/USD", Strategy = "trend-filter", Enabled = true }); + return [.. SettingsCatalogue.Build(c, "trend-filter").SelectMany(static g => g.Fields)]; + } + + [Theory] + [InlineData("symbols[0].strategy")] + [InlineData("engine.timeFrame")] + [InlineData("logging.level")] + [InlineData("engine.entryOrderType")] + public void SiCompilanoDaUnElenco(string percorso) + { + SettingField campo = Campi().First(f => f.Path == percorso); + + Assert.True(campo.IsList, $"{percorso} deve essere un elenco"); + Assert.NotEmpty(campo.Options); + } + + [Fact] + public void AncheIBooleaniSonoUnElenco() + { + // "sì" e "no" scritti a mano sono due modi per sbagliare. + SettingField campo = Campi().First(static f => f.Path == "engine.dryRun"); + + Assert.True(campo.IsList); + Assert.Equal(["sì", "no"], campo.Options); + } + + [Fact] + public void INumeriRestanoDaScrivere() + { + SettingField campo = Campi().First(static f => f.Path == "risk.stakePct"); + + Assert.True(campo.IsFreeText); + Assert.Empty(campo.Options); + } + + [Fact] + public void OgniValoreInizialeDiUnElencoEFraLeOpzioni() + { + // Con la configurazione di fabbrica nessun campo a scelta deve partire in + // errore: se succede, catalogo e valori consegnati sono fuori sincrono. + foreach (SettingField campo in Campi().Where(static f => f.IsList && !f.IsReadOnly)) + { + Assert.False(campo.IsObsolete, + $"{campo.Path} vale '{campo.Value}' che non è fra {string.Join(", ", campo.Options)}"); + } + } +}