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

Add ability for windows to save custom state as part of the persisted window layout #197

Merged
merged 3 commits into from
Nov 5, 2018

Conversation

bingenito
Copy link
Member

@bingenito bingenito commented Oct 26, 2018

State will be provided back to the window upon layout load. If the underlying native js window has getState/setState defined they will be invoked.

… window layout and be provided that state back on load of the layout. If the underlying native js window has getState/setState defined they will be invoked.
@bingenito bingenito requested a review from a team October 26, 2018 16:52
@bingenito bingenito changed the title Add ability for windows to save custom state as part of the persisted window layout and be provided that state back on load of the layout. If the underlying native js window has getState/setState defined they will be invoked. Add ability for windows to save custom state as part of the persisted window layout Oct 26, 2018
@codecov
Copy link

codecov bot commented Oct 26, 2018

Codecov Report

Merging #197 into master will increase coverage by 0.03%.
The diff coverage is 95.71%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   93.45%   93.49%   +0.03%     
==========================================
  Files          15       15              
  Lines        1405     1444      +39     
  Branches      237      244       +7     
==========================================
+ Hits         1313     1350      +37     
- Misses         92       94       +2
Impacted Files Coverage Δ
src/container.ts 96.15% <100%> (+0.06%) ⬆️
src/window.ts 88.98% <100%> (+0.19%) ⬆️
src/Electron/electron.ts 96.55% <100%> (+0.08%) ⬆️
src/Default/default.ts 96.91% <90.9%> (-0.74%) ⬇️
src/OpenFin/openfin.ts 86.29% <96.66%> (+0.44%) ⬆️


public setState(state: any): Promise<void> {
if (this.innerWindow && this.innerWindow.webContents) {
return this.innerWindow.webContents.executeJavaScript(`if (setState) setState(JSON.parse(\`${JSON.stringify(state)}\`))`);
Copy link
Member

Choose a reason for hiding this comment

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

We sure there is no better expression of the intent here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@psmulovics What do you mean by intent?

Copy link
Member Author

@bingenito bingenito Oct 26, 2018

Choose a reason for hiding this comment

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

It's certainly not pretty but there is no access to the underlying native window (cross process) and doing it via ipc would require having two way send but if the other side doesn't have any code in place to handle it the caller would be waiting for a reply that isn't coming.

Copy link

Choose a reason for hiding this comment

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

if (setState) would throw an error if setState isn't defined; should this be if (typeof setState !== "undefined") or if (window.setState) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@1j01 I assumed since it was in the global that window.setState is equivalent to setState. Testing it is a bit muddied since it goes through executeJavaScript which can be masking this from me. I'll update it if not for a fix but definitely for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

@1j01 @psmulovics Updated to add explicit use of methods on window global and formatting with braces where appropriate.

psmulovics
psmulovics previously approved these changes Oct 26, 2018
psmulovics
psmulovics previously approved these changes Oct 26, 2018
@bingenito bingenito merged commit f0f7741 into morganstanley:master Nov 5, 2018
@bingenito bingenito deleted the window-state branch March 30, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants