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

Settings store #362

Merged
merged 10 commits into from
Dec 3, 2024
Merged

Settings store #362

merged 10 commits into from
Dec 3, 2024

Conversation

webfiltered
Copy link
Contributor

@webfiltered webfiltered commented Nov 26, 2024

Adds an electron-store config store to house desktop-specific installation state / config. Includes fairly(?) complete error handling and pre-window interaction.

config.json

{
  "basePath": "C:\\Users\\User\\Documents\\ComfyUI",
  "installState": "installed"
}
  • Install state is undefined until an install has started, 'started', or 'installed'.
    • 'upgraded' status is temporarily used when upgrading from a version prior to this PR, and changed to 'installed' when successful
  • Base path is set immediately before the install process starts
    • The base_path in extra_models_config.yaml is still the source of truth
    • basePath in config.json is used as a fallback only

Failures

If the settings store is invalid (file is present, but can't be parsed as json), the user is prompted to resolve the issue:

image

Confirmation if they choose to reset:

image

Resolves #360

@webfiltered webfiltered mentioned this pull request Nov 26, 2024
@webfiltered webfiltered force-pushed the settings-store branch 3 times, most recently from e1a0eaa to a5469b8 Compare November 27, 2024 15:23
Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

In general, the changes to store basePath in a separate store looks good, while the installState probably needs a rename or better documentation on explaining it's scope.


export type DesktopSettings = {
basePath?: string;
installState?: 'started' | 'installed' | 'upgraded';
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some documentation explaining what each state represents? Do we have a plan to expand this install state to track more stages during installation so that we can resume at certain state on install retry?

@huchenlei huchenlei merged commit 54f1a96 into main Dec 3, 2024
6 checks passed
@huchenlei huchenlei deleted the settings-store branch December 3, 2024 23:44
robinjhuang added a commit that referenced this pull request Dec 5, 2024
robinjhuang added a commit that referenced this pull request Dec 5, 2024
webfiltered added a commit that referenced this pull request Dec 6, 2024
huchenlei pushed a commit that referenced this pull request Dec 6, 2024
* Reapply "Settings store (#362)" (#440)

This reverts commit 81320e2.

* Allow user to see result of YAML config read

- Nature of the failure is returned
- Invalid values read are included if available
- Retains inferred types

* Fix cannot start app if invalid install state

Current requirement is that a missing YAML file triggers the install screen.

* Detect YAML parse exceptions

* Update test expectations
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 this pull request may close these issues.

Decouple desktop install location and extra model config
2 participants