From 33d7dca15545efc6bed2c0ebb29e0418e5e57faa Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Thu, 6 Jun 2024 10:45:44 +0700 Subject: [PATCH] fix(developer): prevent two touch layout editors opening for the same file The keyboard editor has a complex edit state machine, which has grown a lot over time. This is a minimal patch to address one specific edge case scenario on that state machine, without any attempt to improve the state machine overall. The biggest change here is bubbling failure up to the main form so that it can destroy (aka Release, which is an asynchronous destroy) the editor window if it fails to load completely. Fixes: #11715 Fixes: KEYMAN-DEVELOPER-1JC --- developer/src/tike/child/UfrmKeymanWizard.pas | 28 +++-- developer/src/tike/main/UfrmMain.pas | 6 +- .../oskbuilder/UframeTouchLayoutBuilder.pas | 118 ++++++++++-------- 3 files changed, 89 insertions(+), 63 deletions(-) diff --git a/developer/src/tike/child/UfrmKeymanWizard.pas b/developer/src/tike/child/UfrmKeymanWizard.pas index d8a62db410..baa906e836 100644 --- a/developer/src/tike/child/UfrmKeymanWizard.pas +++ b/developer/src/tike/child/UfrmKeymanWizard.pas @@ -453,13 +453,13 @@ type procedure ConfirmSaveOfOldEditorWindows; procedure ConfirmSaveOfOldEditorWindow(FeatureID: TKeyboardParser_FeatureID; FModified: Boolean; const FOldFilename: string; DoSave: TProc; DoLoad: TProc); - procedure LoadFeature(ID: TKeyboardParser_FeatureID); + function LoadFeature(ID: TKeyboardParser_FeatureID): Boolean; function FeatureTab(kf: TKeyboardParser_FeatureID): TTabSheet; procedure InitFeatureTab(ID: TKeyboardParser_FeatureID); procedure FeatureModified(Sender: TObject); function SaveFeature(ID: TKeyboardParser_FeatureID): Boolean; procedure SelectTouchLayoutTemplate(APromptChange: Boolean); - procedure LoadTouchLayout; // I4034 + function LoadTouchLayout: Boolean; // I4034 function GetFontInfo(Index: TKeyboardFont): TKeyboardFontInfo; // I4057 procedure SetFontInfo(Index: TKeyboardFont; const Value: TKeyboardFontInfo); // I4057 @@ -1628,7 +1628,7 @@ begin FLayoutSetup := FOldLayoutSetup; end; -procedure TfrmKeymanWizard.LoadFeature(ID: TKeyboardParser_FeatureID); +function TfrmKeymanWizard.LoadFeature(ID: TKeyboardParser_FeatureID): Boolean; begin if FKeyboardParser.Features.ContainsKey(ID) then begin @@ -1651,7 +1651,8 @@ begin end; kfTouchLayout: begin - LoadTouchLayout; // I4034 + if not LoadTouchLayout then + Exit(False); end; else begin @@ -1667,6 +1668,7 @@ begin end; end; FFeature[ID].Modified := False; + Result := True; end; function TfrmKeymanWizard.SaveFeature(ID: TKeyboardParser_FeatureID): Boolean; @@ -2007,7 +2009,12 @@ begin LoadSettings; for kf in FKeyboardParser.Features.Keys do - LoadFeature(kf); + begin + if not LoadFeature(kf) then + begin + Exit(False); + end; + end; if FKeyboardParser.IsComplex then // I4557 pagesLayout.ActivePage := pageLayoutCode; @@ -3163,18 +3170,17 @@ begin FFeature[kfTouchLayout].Modified := True; end; -procedure TfrmKeymanWizard.LoadTouchLayout; // I4034 +function TfrmKeymanWizard.LoadTouchLayout: Boolean; // I4034 begin if pagesTouchLayout.ActivePage = pageTouchLayoutDesign then begin - if not frameTouchLayout.Load(FFeature[kfTouchLayout].Filename, False, False) then - begin - pagesTouchLayout.ActivePage := pageTouchLayoutCode; - frameTouchLayoutSource.LoadFromFile(FFeature[kfTouchLayout].Filename, tffUTF8); - end; + Result := frameTouchLayout.Load(FFeature[kfTouchLayout].Filename, False, False); end else + begin frameTouchLayoutSource.LoadFromFile(FFeature[kfTouchLayout].Filename, tffUTF8); + Result := True; + end; end; procedure TfrmKeymanWizard.SaveTouchLayout; // I3885 diff --git a/developer/src/tike/main/UfrmMain.pas b/developer/src/tike/main/UfrmMain.pas index 44b5873245..8fa7a45c9b 100644 --- a/developer/src/tike/main/UfrmMain.pas +++ b/developer/src/tike/main/UfrmMain.pas @@ -1477,8 +1477,12 @@ begin if n >= 0 then Result.ProjectFile := FGlobalProject.Files[n]; - (Result as frmClass).OpenFile(FFileName); LockWindowUpdate(0); + + if not (Result as frmClass).OpenFile(FFileName) then + begin + Result.Release; + end; end; procedure TfrmKeymanDeveloper.HelpTopic(s: string); diff --git a/developer/src/tike/oskbuilder/UframeTouchLayoutBuilder.pas b/developer/src/tike/oskbuilder/UframeTouchLayoutBuilder.pas index 4b0405a32f..68bff9fd60 100644 --- a/developer/src/tike/oskbuilder/UframeTouchLayoutBuilder.pas +++ b/developer/src/tike/oskbuilder/UframeTouchLayoutBuilder.pas @@ -90,7 +90,7 @@ type procedure cefCommand(Sender: TObject; const command: string; params: TStringList); procedure cefLoadEnd(Sender: TObject); - procedure RegisterSource; + procedure RegisterSources(const AState: string); procedure CharMapDragDrop(Sender, Source: TObject; X, Y: Integer); procedure CharMapDragOver(Sender, Source: TObject; X, Y: Integer; State: TDragState; var Accept: Boolean); @@ -238,10 +238,12 @@ begin modWebHttpServer.AppSource.UnregisterSource(FFilename+'#state'); end; -procedure TframeTouchLayoutBuilder.RegisterSource; +procedure TframeTouchLayoutBuilder.RegisterSources(const AState: string); begin if FFilename <> '' then modWebHttpServer.AppSource.RegisterSource(FFilename, FSavedLayoutJS); + if (FFileName <> '') and (AState <> '') then + modWebHttpServer.AppSource.RegisterSource(FFilename + '#state', AState, True); end; procedure TframeTouchLayoutBuilder.ImportFromKVK(const KVKFileName: string); // I3945 @@ -332,71 +334,85 @@ begin end; UnregisterSources; - try - if ALoadFromString then + if ALoadFromString then + begin + FNewLayoutJS := AFilename; + FFilename := GetNextFilename; + end + else + begin + if ALoadFromTemplate or (AFileName = '') or not FileExists(AFileName) then begin - FNewLayoutJS := AFilename; + FBaseFileName := FTemplateFileName; FFilename := GetNextFilename; end else begin - if ALoadFromTemplate or (AFileName = '') or not FileExists(AFileName) then - begin - FBaseFileName := FTemplateFileName; - FFilename := GetNextFilename; - end - else - begin - FBaseFileName := AFileName; - FFilename := AFileName; - end; - - with TStringList.Create do - try - LoadFromFile(FBaseFileName, TEncoding.UTF8); - FNewLayoutJS := Text; - finally - Free; - end; + FBaseFileName := AFileName; + FFilename := AFileName; end; - FTouchLayout := TTouchLayout.Create; // I3642 + with TStringList.Create do try - if not FTouchLayout.Load(FNewLayoutJS) then + LoadFromFile(FBaseFileName, TEncoding.UTF8); + FNewLayoutJS := Text; + finally + Free; + end; + end; + + FTouchLayout := TTouchLayout.Create; // I3642 + try + if not FTouchLayout.Load(FNewLayoutJS) then + begin + FLastError := FTouchLayout.LoadError; // I4083 + FLastErrorOffset := FTouchLayout.LoadErrorOffset; // I4083 + FFilename := FLastFilename; + RegisterSources(FState); + Exit(False); + end + else + begin + if (FSavedLayoutJS <> '') and ALoadFromTemplate then begin - FLastError := FTouchLayout.LoadError; // I4083 - FLastErrorOffset := FTouchLayout.LoadErrorOffset; // I4083 - FFilename := FLastFilename; - Exit(False); + FOldLayout := TTouchLayout.Create; + try + FOldLayout.Load(FSavedLayoutJS); + if FTouchLayout.Merge(FOldLayout) + then FSavedLayoutJS := FTouchLayout.Save(False) + else FSavedLayoutJS := FNewLayoutJS; + finally + FOldLayout.Free; + end; end else - begin - if (FSavedLayoutJS <> '') and ALoadFromTemplate then - begin - FOldLayout := TTouchLayout.Create; - try - FOldLayout.Load(FSavedLayoutJS); - if FTouchLayout.Merge(FOldLayout) - then FSavedLayoutJS := FTouchLayout.Save(False) - else FSavedLayoutJS := FNewLayoutJS; - finally - FOldLayout.Free; - end; - end - else - FSavedLayoutJS := FNewLayoutJS; - end; - finally - FTouchLayout.Free; + FSavedLayoutJS := FNewLayoutJS; end; - finally - RegisterSource; - if (FFileName <> '') and (FState <> '') then - modWebHttpServer.AppSource.RegisterSource(FFilename + '#state', FState, True); + FTouchLayout.Free; end; + if (FFileName <> '') and modWebHttpServer.AppSource.IsSourceRegistered(FFileName) then + begin + // If two .kmn files are loaded which both reference the same + // .keyman-touch-layout file, it's safest to just block it. This is a rare + // scenario, as most keyboard projects have a single .kmn, and it usually + // indicates a project may be in a bit of chaos anyway. + ShowMessage( + 'The touch layout is already opened for editing in another keyboard '+ + 'editor. Please close the other keyboard editor before opening this one '+ + 'again.'); + + // We want to prevent this window unregistering the sources it doesn't own + // when it is destroyed immediately after this, which we can do by blanking + // the filename. + FFileName := ''; + Exit(False); + end; + + RegisterSources(FState); + try DoLoad; except