From a72d3e5c7aa81e7cee5c086a1c57cf76be27ffcd Mon Sep 17 00:00:00 2001 From: Evan Simkowitz Date: Tue, 3 Dec 2024 21:39:06 -0500 Subject: [PATCH] Add recursive flag to prevent unwanted deletion of parents while they're already being deleted (#1378) When a tab was being deleted, for instance, the last block that got deleted would cascade to delete its parent tab, even though the `DeleteTab` function was already going to do this. This would produce DB collisions that would put the app into a bad state. Now we pass a `recursive` flag that is used to determine whether to perform the recursive close of the parents. Also updates `DeleteWorkspace` so that named workspaces will always be deleted if they're empty, even if `force` is false --- pkg/service/objectservice/objectservice.go | 2 +- pkg/service/workspaceservice/workspaceservice.go | 2 +- pkg/wcore/block.go | 12 ++++++++---- pkg/wcore/window.go | 2 ++ pkg/wcore/workspace.go | 14 +++++++++----- pkg/wshrpc/wshserver/wshserver.go | 4 ++-- 6 files changed, 23 insertions(+), 13 deletions(-) diff --git a/pkg/service/objectservice/objectservice.go b/pkg/service/objectservice/objectservice.go index 6b1bc99d1..40baca645 100644 --- a/pkg/service/objectservice/objectservice.go +++ b/pkg/service/objectservice/objectservice.go @@ -121,7 +121,7 @@ func (svc *ObjectService) DeleteBlock(uiContext waveobj.UIContext, blockId strin ctx, cancelFn := context.WithTimeout(context.Background(), DefaultTimeout) defer cancelFn() ctx = waveobj.ContextWithUpdates(ctx) - err := wcore.DeleteBlock(ctx, blockId) + err := wcore.DeleteBlock(ctx, blockId, true) if err != nil { return nil, fmt.Errorf("error deleting block: %w", err) } diff --git a/pkg/service/workspaceservice/workspaceservice.go b/pkg/service/workspaceservice/workspaceservice.go index 5c634684e..f3198f727 100644 --- a/pkg/service/workspaceservice/workspaceservice.go +++ b/pkg/service/workspaceservice/workspaceservice.go @@ -169,7 +169,7 @@ func (svc *WorkspaceService) CloseTab(ctx context.Context, workspaceId string, t blockcontroller.StopBlockController(blockId) } }() - newActiveTabId, err := wcore.DeleteTab(ctx, workspaceId, tabId) + newActiveTabId, err := wcore.DeleteTab(ctx, workspaceId, tabId, true) if err != nil { return nil, nil, fmt.Errorf("error closing tab: %w", err) } diff --git a/pkg/wcore/block.go b/pkg/wcore/block.go index 28559a032..7a23e30e3 100644 --- a/pkg/wcore/block.go +++ b/pkg/wcore/block.go @@ -99,7 +99,11 @@ func createBlockObj(ctx context.Context, tabId string, blockDef *waveobj.BlockDe }) } -func DeleteBlock(ctx context.Context, blockId string) error { +// Must delete all blocks individually first. +// Also deletes LayoutState. +// recursive: if true, will recursively close parent tab, window, workspace, if they are empty. +// Returns new active tab id, error. +func DeleteBlock(ctx context.Context, blockId string, recursive bool) error { block, err := wstore.DBMustGet[*waveobj.Block](ctx, blockId) if err != nil { return fmt.Errorf("error getting block: %w", err) @@ -109,7 +113,7 @@ func DeleteBlock(ctx context.Context, blockId string) error { } if len(block.SubBlockIds) > 0 { for _, subBlockId := range block.SubBlockIds { - err := DeleteBlock(ctx, subBlockId) + err := DeleteBlock(ctx, subBlockId, recursive) if err != nil { return fmt.Errorf("error deleting subblock %s: %w", subBlockId, err) } @@ -123,14 +127,14 @@ func DeleteBlock(ctx context.Context, blockId string) error { parentORef := waveobj.ParseORefNoErr(block.ParentORef) if parentORef.OType == waveobj.OType_Tab { - if parentBlockCount == 0 { + if parentBlockCount == 0 && recursive { // if parent tab has no blocks, delete the tab log.Printf("DeleteBlock: parent tab has no blocks, deleting tab %s", parentORef.OID) parentWorkspaceId, err := wstore.DBFindWorkspaceForTabId(ctx, parentORef.OID) if err != nil { return fmt.Errorf("error finding workspace for tab to delete %s: %w", parentORef.OID, err) } - newActiveTabId, err := DeleteTab(ctx, parentWorkspaceId, parentORef.OID) + newActiveTabId, err := DeleteTab(ctx, parentWorkspaceId, parentORef.OID, true) if err != nil { return fmt.Errorf("error deleting tab %s: %w", parentORef.OID, err) } diff --git a/pkg/wcore/window.go b/pkg/wcore/window.go index 08ec9d8dd..b989eeeb9 100644 --- a/pkg/wcore/window.go +++ b/pkg/wcore/window.go @@ -120,6 +120,8 @@ func CreateWindow(ctx context.Context, winSize *waveobj.WinSize, workspaceId str return GetWindow(ctx, windowId) } +// CloseWindow closes a window and deletes its workspace if it is empty and not named. +// If fromElectron is true, it does not send an event to Electron. func CloseWindow(ctx context.Context, windowId string, fromElectron bool) error { log.Printf("CloseWindow %s\n", windowId) window, err := GetWindow(ctx, windowId) diff --git a/pkg/wcore/workspace.go b/pkg/wcore/workspace.go index 930f20762..717ff41e6 100644 --- a/pkg/wcore/workspace.go +++ b/pkg/wcore/workspace.go @@ -28,19 +28,22 @@ func CreateWorkspace(ctx context.Context, name string, icon string, color string return ws, nil } +// If force is true, it will delete even if workspace is named. +// If workspace is empty, it will be deleted, even if it is named. +// Returns true if workspace was deleted, false if it was not deleted. func DeleteWorkspace(ctx context.Context, workspaceId string, force bool) (bool, error) { log.Printf("DeleteWorkspace %s\n", workspaceId) workspace, err := wstore.DBMustGet[*waveobj.Workspace](ctx, workspaceId) if err != nil { return false, fmt.Errorf("error getting workspace: %w", err) } - if workspace.Name != "" && workspace.Icon != "" && !force { + if workspace.Name != "" && workspace.Icon != "" && !force && len(workspace.TabIds) > 0 { log.Printf("Ignoring DeleteWorkspace for workspace %s as it is named\n", workspaceId) return false, nil } for _, tabId := range workspace.TabIds { log.Printf("deleting tab %s\n", tabId) - _, err := DeleteTab(ctx, workspaceId, tabId) + _, err := DeleteTab(ctx, workspaceId, tabId, false) if err != nil { return false, fmt.Errorf("error closing tab: %w", err) } @@ -104,8 +107,9 @@ func CreateTab(ctx context.Context, workspaceId string, tabName string, activate // Must delete all blocks individually first. // Also deletes LayoutState. +// recursive: if true, will recursively close parent window, workspace, if they are empty. // Returns new active tab id, error. -func DeleteTab(ctx context.Context, workspaceId string, tabId string) (string, error) { +func DeleteTab(ctx context.Context, workspaceId string, tabId string, recursive bool) (string, error) { ws, _ := wstore.DBGet[*waveobj.Workspace](ctx, workspaceId) if ws == nil { return "", fmt.Errorf("workspace not found: %q", workspaceId) @@ -117,7 +121,7 @@ func DeleteTab(ctx context.Context, workspaceId string, tabId string) (string, e // close blocks (sends events + stops block controllers) for _, blockId := range tab.BlockIds { - err := DeleteBlock(ctx, blockId) + err := DeleteBlock(ctx, blockId, false) if err != nil { return "", fmt.Errorf("error deleting block %s: %w", blockId, err) } @@ -141,7 +145,7 @@ func DeleteTab(ctx context.Context, workspaceId string, tabId string) (string, e wstore.DBDelete(ctx, waveobj.OType_Tab, tabId) wstore.DBDelete(ctx, waveobj.OType_LayoutState, tab.LayoutState) - if newActiveTabId == "" { + if newActiveTabId == "" && recursive { windowId, err := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId) if err != nil { return newActiveTabId, fmt.Errorf("unable to find window for workspace id %v: %w", workspaceId, err) diff --git a/pkg/wshrpc/wshserver/wshserver.go b/pkg/wshrpc/wshserver/wshserver.go index c23e813eb..7255e93ba 100644 --- a/pkg/wshrpc/wshserver/wshserver.go +++ b/pkg/wshrpc/wshserver/wshserver.go @@ -489,7 +489,7 @@ func (ws *WshServer) FileAppendIJsonCommand(ctx context.Context, data wshrpc.Com } func (ws *WshServer) DeleteSubBlockCommand(ctx context.Context, data wshrpc.CommandDeleteBlockData) error { - err := wcore.DeleteBlock(ctx, data.BlockId) + err := wcore.DeleteBlock(ctx, data.BlockId, false) if err != nil { return fmt.Errorf("error deleting block: %w", err) } @@ -505,7 +505,7 @@ func (ws *WshServer) DeleteBlockCommand(ctx context.Context, data wshrpc.Command if tabId == "" { return fmt.Errorf("no tab found for block") } - err = wcore.DeleteBlock(ctx, data.BlockId) + err = wcore.DeleteBlock(ctx, data.BlockId, true) if err != nil { return fmt.Errorf("error deleting block: %w", err) }