initial download-folder-selector interface #890
Conversation
mcmonkey4eva
added
New Browser Test Expectations
There was a problem hiding this comment.
Can you add at least a playwright test to verify the UI is shown correctly? You can add code to https://github.com/Comfy-Org/ComfyUI_devtools to perform test setup.
(for testing purposes only basically, but does no harm in deployed envs)
mcmonkey4eva
added
New Browser Test Expectations
mcmonkey4eva
added
New Browser Test Expectations
mcmonkey4eva
added
New Browser Test Expectations
mcmonkey4eva
added
the
New Browser Test Expectations
since the checkbox covers that better anyway
mcmonkey4eva
added
New Browser Test Expectations
mcmonkey4eva
added
New Browser Test Expectations
|
alright, now it has playwright testing on it, screenshots all look correctly. (Note the folder selector shows 'models' and 'output' in the screenshot, visually similar but not wrong) |
The general rule of thumb for screenshot test is that we only do that do check litegraph canvas. For things that can be checked via inspecting DOM / evaluating JS, we do that instead to make test less fragile to subtle style changes and running locally. |
|
Kinda hurts not being able to do the thorough validation of screenshots but I get it, too much changeweight any time minor stylistic stuff is altered. I rewrote the tests to exclude screenshots and instead check visibility and classnames to ensure they are reacting appropriately. There's probably room to be more thorough than that? But that covers the key predictable functional parts, short of writing a full unit test that mocks the folder list and download apis to test end-to-end. |
| 'Comfy.Workflow.ModelDownload.AllowedSuffixes' | ||
| ) | ||
| const showFolderSelect = ref(false) | ||
|
|
There was a problem hiding this comment.
Why this becomes hard coded again? We do want to expose this configurability to cloud deployment platforms to download from custom endpoints.
|
They need to not be in user settings, and they should ideally not be something that gets corrupted by an accidental run of the playwright test. If we want to expose server configurability, it should not be through user settings. As noted earlier, the most ideal place imo would be an internal API on the Comfy core side that returns its actual accepted value list, and then the UI simply reads and works with that. (The UI is simply trying to preview what the server will determine, and atm serverside it's hardcoded) |
I think the playwright test side issue can be fixed by having extra logic backup server side user files and restore them after running all tests. Just for testability, they should not be hard-coded. |
|
|
Do you really want to expose localhost to default model download whitelist origin? |
|
|
* initial download-folder-selector interface * use primevue select * add a folder select visibility checkbox * slightly reduce indirection * fix up select box updating * revert bad upstream changes * cleanup * allow localhost sourced models in ui side (for testing purposes only basically, but does no harm in deployed envs) * add screenshot expectations to test * Update test expectations [skip ci] * add testing of folder select * fix test * don't exclude folder selector when there's only 1 since the checkbox covers that better anyway * oo - fix checkbox * Update test expectations [skip ci] * testing - don't expect screenshots :( * experimental new test code * toHaveClass is silly * add // comments documenting intent of allowedSources --------- Co-authored-by: github-actions <[email protected]>
* initial download-folder-selector interface * use primevue select * add a folder select visibility checkbox * slightly reduce indirection * fix up select box updating * revert bad upstream changes * cleanup * allow localhost sourced models in ui side (for testing purposes only basically, but does no harm in deployed envs) * add screenshot expectations to test * Update test expectations [skip ci] * add testing of folder select * fix test * don't exclude folder selector when there's only 1 since the checkbox covers that better anyway * oo - fix checkbox * Update test expectations [skip ci] * testing - don't expect screenshots :( * experimental new test code * toHaveClass is silly * add // comments documenting intent of allowedSources --------- Co-authored-by: github-actions <[email protected]>
* initial download-folder-selector interface * use primevue select * add a folder select visibility checkbox * slightly reduce indirection * fix up select box updating * revert bad upstream changes * cleanup * allow localhost sourced models in ui side (for testing purposes only basically, but does no harm in deployed envs) * add screenshot expectations to test * Update test expectations [skip ci] * add testing of folder select * fix test * don't exclude folder selector when there's only 1 since the checkbox covers that better anyway * oo - fix checkbox * Update test expectations [skip ci] * testing - don't expect screenshots :( * experimental new test code * toHaveClass is silly * add // comments documenting intent of allowedSources --------- Co-authored-by: github-actions <[email protected]>
Depends on comfyanonymous/ComfyUI#4981 and comfyanonymous/ComfyUI#4980
This is a followup from earlier PR #569
Adds a model download folder selector
If I use vanilla HTML Select it's naturally quite ugly
PrimeVue looks nicer:
But it's weirdly slow about updating when you select things and idk what's up with that. I think it's something to do withfixedv-modeland the indirection towards the actual folder path, but I'm not entirely sure.It's also chonky and ugly regardless so I added a visibility checkbox
(Most users won't want to change from default in theory)
TODO
fix the select interface being all wonkyfixedmake the tests pass lolthe merge fixed it? lol