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

feat(desktop): save and restore window state #1852

Merged
merged 6 commits into from
Feb 10, 2025
Merged

Conversation

ni00
Copy link
Contributor

@ni00 ni00 commented Jan 3, 2025

PR Checklist

What is the current behavior?

Won't save and restore window position and size.

Issue Number

#1849

What is the new behavior?

Save and restore window position and size.

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Other information

reference: electron/electron#526

@Red-Asuka Red-Asuka added feature This pr is a feature desktop MQTTX Desktop labels Jan 13, 2025
src/background.ts Outdated Show resolved Hide resolved
@ysfscream
Copy link
Member

ysfscream commented Jan 13, 2025

So many thanks for implementing the window state persistence feature!

The implementation looks thoughtful and handles the multi-monitor scenario well. Here are some thoughts that might be helpful:

  1. We might want to consider adding maximized state handling?
// Example approach:
electronStore.set('windowState', {
  bounds: win.getBounds(),
  isMaximized: win.isMaximized()
})
  1. Would it make sense to add some error handling? Something like:
try {
  win.setBounds(savedBounds)
} catch (error) {
  // log some error here
  console.error('Failed to restore window bounds:', error)
  // Handle fallback
}
  1. Can we sync the latest window size values to the database for better persistence?
image

Only when we open it for the first time do we use 1025 * 749 as the default value?

const defaultBounds = {
  width: 1024,
  height: 768,
  x: 0,
  y: 0
}

@ysfscream ysfscream closed this Jan 13, 2025
@ysfscream ysfscream reopened this Jan 13, 2025
@ni00
Copy link
Contributor Author

ni00 commented Jan 20, 2025

图片

I'm considering removing the height and width fields from SettingEntity

Reasons:

  1. These two fields currently only provide default values and cannot be explicitly modified, which is inconsistent with other configurable properties in SettingEntity that can be adjusted in the settings page

  2. Window size, position, and other state properties are better suited for internal application state management, and don't need to be included in user-configurable settings

  3. Following practices from other Electron applications, window states are typically managed using lightweight storage solutions like electron-store, rather than being written to the main database that requires backup

  4. These window state properties are not noticeable to users and don't need to be synchronized in backups

@ysfscream ysfscream added this to the v1.12.0 milestone Jan 21, 2025
@Red-Asuka
Copy link
Member

@ni00 Thank you very much for your contribution to the project! I have optimized the existing code and resolved the building error. I completely agree with your views on storing window width and height data. In version v1.x, we will no longer use the width and height from the database table. These have already been removed in version v2.x. For more details on the changes, please see: #1868

@Red-Asuka Red-Asuka requested a review from ysfscream February 10, 2025 09:00
@ysfscream ysfscream merged commit 9b5275f into emqx:main Feb 10, 2025
2 checks passed
@ysfscream
Copy link
Member

Thanks to all @ni00 @Red-Asuka

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop MQTTX Desktop feature This pr is a feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants