Skip to content

Deleting current workspace switches to another instead of closing [ backend implementation ] #1623

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Dec 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions emain/emain-window.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { delay, ensureBoundsAreVisible, waveKeyToElectronKey } from "./emain-uti
import { log } from "./log";
import { getElectronAppBasePath, unamePlatform } from "./platform";
import { updater } from "./updater";

export type WindowOpts = {
unamePlatform: string;
};
Expand Down Expand Up @@ -303,7 +304,8 @@ export class WaveBrowserWindow extends BaseWindow {
const workspaceList = await WorkspaceService.ListWorkspaces();
if (!workspaceList?.find((wse) => wse.workspaceid === workspaceId)?.windowid) {
const curWorkspace = await WorkspaceService.GetWorkspace(this.workspaceId);
if (isNonEmptyUnsavedWorkspace(curWorkspace)) {

if (curWorkspace && isNonEmptyUnsavedWorkspace(curWorkspace)) {
console.log(
`existing unsaved workspace ${this.workspaceId} has content, opening workspace ${workspaceId} in new window`
);
Expand Down Expand Up @@ -693,17 +695,22 @@ ipcMain.on("delete-workspace", (event, workspaceId) => {
type: "question",
buttons: ["Cancel", "Delete Workspace"],
title: "Confirm",
message: `Deleting workspace will also delete its contents.${workspaceHasWindow ? "\nWorkspace is open in a window, which will be closed." : ""}\n\nContinue?`,
message: `Deleting workspace will also delete its contents.\n\nContinue?`,
});
if (choice === 0) {
console.log("user cancelled workspace delete", workspaceId, ww?.waveWindowId);
return;
}
await WorkspaceService.DeleteWorkspace(workspaceId);

const newWorkspaceId = await WorkspaceService.DeleteWorkspace(workspaceId);
console.log("delete-workspace done", workspaceId, ww?.waveWindowId);
if (ww?.workspaceId == workspaceId) {
console.log("delete-workspace closing window", workspaceId, ww?.waveWindowId);
ww.destroy();
if (newWorkspaceId) {
await ww.switchWorkspace(newWorkspaceId);
} else {
console.log("delete-workspace closing window", workspaceId, ww?.waveWindowId);
ww.destroy();
}
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion frontend/app/store/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ class WorkspaceServiceType {
}

// @returns object updates
DeleteWorkspace(workspaceId: string): Promise<void> {
DeleteWorkspace(workspaceId: string): Promise<string> {
return WOS.callBackendService("workspace", "DeleteWorkspace", Array.from(arguments))
}

Expand Down
16 changes: 10 additions & 6 deletions pkg/service/workspaceservice/workspaceservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ func (svc *WorkspaceService) UpdateWorkspace(ctx context.Context, workspaceId st
}

wps.Broker.Publish(wps.WaveEvent{
Event: wps.Event_WorkspaceUpdate})
Event: wps.Event_WorkspaceUpdate,
})

updates := waveobj.ContextGetUpdatesRtn(ctx)
go func() {
Expand Down Expand Up @@ -87,23 +88,26 @@ func (svc *WorkspaceService) DeleteWorkspace_Meta() tsgenmeta.MethodMeta {
}
}

func (svc *WorkspaceService) DeleteWorkspace(workspaceId string) (waveobj.UpdatesRtnType, error) {
func (svc *WorkspaceService) DeleteWorkspace(workspaceId string) (waveobj.UpdatesRtnType, string, error) {
ctx, cancelFn := context.WithTimeout(context.Background(), DefaultTimeout)
defer cancelFn()
ctx = waveobj.ContextWithUpdates(ctx)
deleted, err := wcore.DeleteWorkspace(ctx, workspaceId, true)
deleted, claimableWorkspace, err := wcore.DeleteWorkspace(ctx, workspaceId, true)
if claimableWorkspace != "" {
return nil, claimableWorkspace, nil
}
if err != nil {
return nil, fmt.Errorf("error deleting workspace: %w", err)
return nil, claimableWorkspace, fmt.Errorf("error deleting workspace: %w", err)
}
if !deleted {
return nil, nil
return nil, claimableWorkspace, nil
}
updates := waveobj.ContextGetUpdatesRtn(ctx)
go func() {
defer panichandler.PanicHandler("WorkspaceService:DeleteWorkspace:SendUpdateEvents")
wps.Broker.SendUpdateEvents(updates)
}()
return updates, nil
return updates, claimableWorkspace, nil
}

func (svc *WorkspaceService) ListWorkspaces() (waveobj.WorkspaceList, error) {
Expand Down
11 changes: 7 additions & 4 deletions pkg/wcore/window.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,13 @@ func SwitchWorkspace(ctx context.Context, windowId string, workspaceId string) (
return nil, fmt.Errorf("error updating window: %w", err)
}

deleted, err := DeleteWorkspace(ctx, curWsId, false)
if err != nil {
return nil, fmt.Errorf("error deleting current workspace: %w", err)
deleted, _, err := DeleteWorkspace(ctx, curWsId, false)
if err != nil && deleted {
print(err.Error()) // @jalileh isolated the error for now, curwId/workspace was deleted when this occurs.
} else if err != nil {
return nil, fmt.Errorf("error deleting workspace: %w", err)
}

if !deleted {
log.Printf("current workspace %s was not deleted\n", curWsId)
} else {
Expand Down Expand Up @@ -131,7 +134,7 @@ func CloseWindow(ctx context.Context, windowId string, fromElectron bool) error
window, err := GetWindow(ctx, windowId)
if err == nil {
log.Printf("got window %s\n", windowId)
deleted, err := DeleteWorkspace(ctx, window.WorkspaceId, false)
deleted, _, err := DeleteWorkspace(ctx, window.WorkspaceId, false)
if err != nil {
log.Printf("error deleting workspace: %v\n", err)
}
Expand Down
55 changes: 45 additions & 10 deletions pkg/wcore/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ func CreateWorkspace(ctx context.Context, name string, icon string, color string
}

wps.Broker.Publish(wps.WaveEvent{
Event: wps.Event_WorkspaceUpdate})
Event: wps.Event_WorkspaceUpdate,
})

ws, _, err = UpdateWorkspace(ctx, ws.OID, name, icon, color, applyDefaults)
return ws, err
Expand Down Expand Up @@ -114,40 +115,74 @@ func UpdateWorkspace(ctx context.Context, workspaceId string, name string, icon
// 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) {
func DeleteWorkspace(ctx context.Context, workspaceId string, force bool) (bool, string, error) {
log.Printf("DeleteWorkspace %s\n", workspaceId)
workspace, err := wstore.DBMustGet[*waveobj.Workspace](ctx, workspaceId)
if err != nil && wstore.ErrNotFound == err {
return true, "", fmt.Errorf("workspace already deleted %w", err)
}
// @jalileh list needs to be saved early on i assume
workspaces, err := ListWorkspaces(ctx)
if err != nil {
return false, "", fmt.Errorf("error retrieving workspaceList: %w", err)
}

if err != nil {
return false, fmt.Errorf("error getting workspace: %w", err)
return false, "", fmt.Errorf("error getting workspace: %w", err)
}
if workspace.Name != "" && workspace.Icon != "" && !force && (len(workspace.TabIds) > 0 || len(workspace.PinnedTabIds) > 0) {
log.Printf("Ignoring DeleteWorkspace for workspace %s as it is named\n", workspaceId)
return false, nil
return false, "", nil
}

// delete all pinned and unpinned tabs
for _, tabId := range append(workspace.TabIds, workspace.PinnedTabIds...) {
log.Printf("deleting tab %s\n", tabId)
_, err := DeleteTab(ctx, workspaceId, tabId, false)
if err != nil {
return false, fmt.Errorf("error closing tab: %w", err)
return false, "", fmt.Errorf("error closing tab: %w", err)
}
}
windowId, err := wstore.DBFindWindowForWorkspaceId(ctx, workspaceId)
err = wstore.DBDelete(ctx, waveobj.OType_Workspace, workspaceId)
if err != nil {
return false, fmt.Errorf("error deleting workspace: %w", err)
return false, "", fmt.Errorf("error deleting workspace: %w", err)
}
log.Printf("deleted workspace %s\n", workspaceId)
wps.Broker.Publish(wps.WaveEvent{
Event: wps.Event_WorkspaceUpdate})
Event: wps.Event_WorkspaceUpdate,
})

if windowId != "" {
err = CloseWindow(ctx, windowId, false)

UnclaimedWorkspace, findAfter := "", false
for _, ws := range workspaces {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked your old logic which would attempt to find the workspace at index closest to the one that was deleted, could you incorporate that here?

I think you can do this in one pass. Create three variables, curWsIndex, freeWsIndexBeforeCurrent and freeWsIndexAfterCurrent. Then, you iterate through the workspace list, setting freeWsIndexBeforeCurrent to the index in the workspace list where you find a free workspace (not claimed by a window). Once you find the current index, you set curWsIndex and from there you continue iterating until you find another free workspace, where you set freeWsIndexAfterCurrent. Then, you just take whichever one of the two indices is closer to curWsIndex (or whichever isn't null).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can implement that if we can ignore this return error, because im relying on getting any claimable workspace to not upset this code because deleteworkspace also runs in switchworkspace for some reason i dont know
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it runs in switch workspace to clean up unsaved workspaces. For saved ones, delete without a force flag is a no-op

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do I actually retrieve the error string in the error data structure? if I can get the string to reliably know if 'not found' happened I wont have to limit the claimworkspace implementation, I only saw msg inside of it but couldn't access it, sorry I'm new to go.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can return empty string if nothing is deleted. This should never happen if you call deleteworkspace from the workspace service since that passes the force flag as true to the wcore implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK ill implement the old logic tomorrow and notify you, ty!

if ws.WorkspaceId == workspaceId {
if UnclaimedWorkspace != "" {
break
}
findAfter = true
continue
}
if findAfter && ws.WindowId == "" {
UnclaimedWorkspace = ws.WorkspaceId
break
} else if ws.WindowId == "" {
UnclaimedWorkspace = ws.WorkspaceId
}
}

if UnclaimedWorkspace != "" {
return true, UnclaimedWorkspace, nil
} else {
err = CloseWindow(ctx, windowId, false)
}

if err != nil {
return false, fmt.Errorf("error closing window: %w", err)
return false, "", fmt.Errorf("error closing window: %w", err)
}
}
return true, nil
return true, "", nil
}

func GetWorkspace(ctx context.Context, wsID string) (*waveobj.Workspace, error) {
Expand Down
Loading