Skip to content

Correct ranges and enums for various preference values#10

Merged
confluence merged 11 commits into
mainfrom
confluence/correct-definition-wcstype-percentile
Jul 22, 2025
Merged

Correct ranges and enums for various preference values#10
confluence merged 11 commits into
mainfrom
confluence/correct-definition-wcstype-percentile

Conversation

@confluence
Copy link
Copy Markdown
Contributor

@confluence confluence commented Jul 7, 2025

Fixes #8.

This PR corrects several restrictions on preference values which are out of sync with the frontend and cause unexpected behaviour because of schema validation errors.

These changes mostly affect the frontend, so I will describe the details in the related frontend issue.

I have removed the types from preferences which have values restricted to enums -- per the JSON schema docs, including them is considered bad practice, as the enum already implicitly determines the type(s).

Some constraints have been relaxed, so that limitations in the frontend input GUI don't restrict what valid values can be entered, and so that we don't make it unnecessarily difficult to change these input selections.

Companion PRs in the backend, frontend, and controller.

@confluence confluence self-assigned this Jul 7, 2025
@confluence confluence changed the title Confluence/correct definition wcstype percentile Correct ranges and enums for various preference values Jul 7, 2025
@loveluthien
Copy link
Copy Markdown

loveluthien commented Jul 8, 2025

Do we need to consider the case that users modify the value directly in preferences.json? If yes, users giving any unvalidated value will cause preferences to be reset. I guess we need to handle the incorrect value in preferences.json to be able to upgrade those values.

@confluence
Copy link
Copy Markdown
Contributor Author

@loveluthien only the individual preferences which are invalid will be reset. I think this is to be expected if users manually edit the file and make a mistake; we don't need to add any special handling. In any case, this is an issue for the frontend, not for the schemas (here we only define what the schema should be).

Comment thread preferences_schema_2.json
Comment thread preferences_schema_2.json
Copy link
Copy Markdown

@loveluthien loveluthien left a comment

Choose a reason for hiding this comment

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

Looks good. Feel free to merge if you want to merge tonight, I will update schemas in the frontend in my morning.

Comment thread preferences_schema_2.json
"minimum": 0.5,
"maximum": 10
},
"pvPreviewCubeSizeLimit": {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please modify this to "minimum": 0 and "maximum": 2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minimum 0.1 GB maybe?

Comment thread preferences_schema_2.json Outdated
},
"pvPreviewCubeSizeLimitUnit": {
"type": "string",
"enum": ["TB", "GB", "MB", "kB", "B"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only "GB" here.

@loveluthien
Copy link
Copy Markdown

loveluthien commented Jul 17, 2025

@confluence, ksw found an asynchronous problem when changing the pvPreviewCubeSizeLimit and pvPreviewCubeSizeLimitUnit. Our solution is that store the pvPreviewCubeSizeLimit only in GB in preferences.json, and the frontend UI does the MB conversion (for display only), so pvPreviewCubeSizeLimitUnit in schemas is limited to GB. I also realized that the frontend gives a boundary for pvPreviewCubeSizeLimit between 1e-12 and 2 GB.

@confluence
Copy link
Copy Markdown
Contributor Author

confluence commented Jul 17, 2025

@kswang1029 @loveluthien Eliminating the different units simplifies a lot of things -- this is one of the changes I suggested in this refactoring request. If we're doing this now, I suggest that we deprecate pvPreviewCubeSizeLimitUnit entirely (i.e. remove it from the schema and frontend code completely -- old prefs won't break anything because extra prefs are allowed; to clean up we could clear it explicitly in the upgrade code if it exists). I realise now that we can't just ignore it -- we need to convert values saved with other units. This can be done in the upgrade function, and then the unit preference can be cleared. We definitely can't just change the schema to allow GB only -- then the frontend will delete the pref if it has an invalid value, and the limit value will be interpreted as GB when it isn't. I suggest marking this as deprecated in the schema, and not using it anywhere in the frontend except the upgrade function.

Regarding the constraints: pvPreviewCubeSizeLimit is one of the size preferences that I discussed with @veggiesaurus -- the strategy that we agreed on was minimal constraints in the schema (so that they can easily be changed in the frontend input). So I don't want to add a maximum here, but I agree that there should be a minimum (for consistency with other limits) -- the reason there wasn't one previously was the unit complication. This should be the real usable minimum (not a default), even if it's not currently selectable in the GUI, so a fraction of a GB seems like a good choice. Is ± 100M a usable limit? I would suggest 128M or 256M (so 0.125 or 0.25) I see that these are treated as metric, not binary, so actually 0.128 or 0.256.

The current minimum in the frontend should to be changed so that values lower than a sensible minimum aren't allowed: if the unit is GB, 1e-12 is less than a byte! It would be about a byte if the unit were TB, which still seems ridiculous. If you're allowing the user to select input units, the bounds should change with the unit.

@loveluthien
Copy link
Copy Markdown

I will make pvPreviewCubeSizeLimitUnit deprecated.
Do we really need such a small number for the minimum? Is it good to use 128 MB for the minimum, like ksw said?

@confluence
Copy link
Copy Markdown
Contributor Author

Do we really need such a small number for the minimum? Is it good to use 128 MB for the minimum, like ksw said?

I am suggesting that we use something like 0.1, as @kswang1029 suggested. I will make more comments in the frontend PR.

@confluence confluence merged commit 73592b2 into main Jul 22, 2025
@confluence confluence deleted the confluence/correct-definition-wcstype-percentile branch July 22, 2025 08:42
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.

Correct definitions in preferences schema

4 participants