Skip to content
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

[workbench layout restore] Detect failure, reset to default layout #1075

Closed
marcdumais-work opened this issue Jan 17, 2018 · 6 comments · Fixed by #1603
Closed

[workbench layout restore] Detect failure, reset to default layout #1075

marcdumais-work opened this issue Jan 17, 2018 · 6 comments · Fixed by #1603
Assignees

Comments

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Jan 17, 2018

once in a while, we encounter workbench layout restoration issues, e.g. when the underlying storage format is updated. The usual symptom is that some expected widgets will be missing. Then the easy way to restore a sane layout is to use the "Reset Workbench Layout" command.

It would be nice if we could detect this type of problem and reset the layout automatically to default.

@svenefftinge svenefftinge self-assigned this Jan 19, 2018
svenefftinge added a commit that referenced this issue Jan 19, 2018
…ixes #1075)

Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
@marcdumais-work
Copy link
Contributor Author

We have user reports that they still sometimes get an incomplete workbench (e.g. missing file navigator) when starting Theia. Triggering the "Reset Workspace Layout" commands clears the issue.

@marcdumais-work
Copy link
Contributor Author

Hi @svenefftinge

Would it be ok to re-assign this issue to someone on our side?

@elaihau
Copy link
Contributor

elaihau commented Mar 27, 2018

Thanks to @marcdumais-work , I am able to reproduce the problem as many times as I want from my local environment.

As far as I can tell, in our current code base, the messed up layout on re-startup is caused by 2 things:

  1. The factory id of the file navigator widget was changed from navigator to files, which causes the widgetManager being unable to re-create the file navigator.

  2. All property names other than statusBar in interface LayoutData were updated, which causes ApplicationShell.setLayoutData() ignoring the mainPanel, leftPanel, rightPanel, and bottomPanel in the original view.

Old LayoutData interface:

export interface LayoutData {
    mainArea?: DockLayoutData;
    leftBar?: SideBarData;
    rightBar?: SideBarData;
    statusBar?: StatusBarLayoutData;
}

New LayoutData interface:

export interface LayoutData {
    mainPanel?: DockPanel.ILayoutConfig;
    bottomPanel?: BottomPanelLayoutData;
    leftPanel?: SidePanel.LayoutData;
    rightPanel?: SidePanel.LayoutData;
    statusBar?: StatusBarLayoutData;
}

Making a hot fix is easy: elaihau@1f6b54d
but I am not happy with the solution. Because it is tied to the two versions of the interface we have, and not good enough to handle the potential breakages like this in future.
What do you think? @marcdumais-work

@spoenemann
Copy link
Contributor

We could introduce explicit version numbers in the layout data and implement conversions.

Or simply never change the data format again 😂

@marcdumais-work
Copy link
Contributor Author

@elaihau agreed

I do not think it's necessary to be able to perfectly restore a workbench that was saved under and old version of the format. But it would be very good to detect those cases and reset the workbench to default. Do you think it's possible to catch those cases you found above in a generic way?

Else @spoenemann 's suggestion of introducing a version number might be the way to go. The version number might tell us if the data we are trying to restore is expected to be compatible with current version, and, if not, we skip restoration and instead reset the workbench to default.

WDYT?

@elaihau
Copy link
Contributor

elaihau commented Mar 28, 2018

I mentioned 2 problems with the current code.
Introducing a version number is helpful to the problem #2 (layout interface change), while we still need to catch and handle the error from the problem #1 (factory id change).

2 options if the widgetManager is unable to re-create a widget from the layout string:
Option 1: reset the workbench
Option 2: ignore that widget, and only create the widgets that widgetManager is able to re-create.

I will try both options and see which one works better.

@spoenemann spoenemann assigned elaihau and unassigned svenefftinge Mar 29, 2018
elaihau pushed a commit to elaihau/theia that referenced this issue Mar 29, 2018
- The initial step that resolves eclipse-theia#1075

Signed-off-by: elaihau <liang.huang@ericsson.com>
elaihau pushed a commit that referenced this issue Mar 29, 2018
- The initial step that resolves #1075

Signed-off-by: elaihau <liang.huang@ericsson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants