Home Page Model Selection and Multiple Tab Support

[JEYuhas]

First Draft of MultiTabs in HARP. Add tab button is in top right corner. Will be moved.

[netlify]

✅ Deploy Preview for harp-plugin canceled.

Name	Link
🔨 Latest commit	cea85ba
🔍 Latest deploy log	https://app.netlify.com/projects/harp-plugin/deploys/69f22b8adaeab30008512c4d

[cwitkowitz]

I've not thoroughly reviewed this PR, but here are some minor issues I noticed:

• If model loading is unsuccessful, the corresponding tab should not be opened.
• The sizing of the categorization tags is off.
• Leftover from the previous dropdown feature, if model loading is unsuccessful, a failure tag (e.g., [SLEEPING]) is appended to the URL and causes all subsequent load attempts to fail.
• I wonder if there is a better way to read in the list of valid models. See the model validation PR (Model Validation Infrastructure, Script, and Workflow #432) for ideas. We also may be able to use the same infrastructure to only display models that will load successfully.

[2cylu2]

Thanks for the review! I changed the failed loading behavior, tag sizing, and URL state. I also agree that using the validation infrastructure to determine valid models is a good idea, so I can connect this to the validation infrastructure once #432 is merged.

[ChughVansh]

Hello!

I think it looks great! Went through the code and tried some models and everything seems to work! Here are some notes for in ModelSelectionWidget.h; all optional and would love to hear your thoughts.

• Since model selection/loading is now handled by HomeTab, there looks to be some dead code. modelSelectionWidget inside ModelTab is never addAndMakeVisible'd (it's pinned to 0,0,0,0 in resized()), and I think it's the only place ModelSelectionWidget is instantiated in the codebase. I tried removing loadModelButton, modelPathComboBox, and the associated pieces of code (initializeLoadModelButton(), initializeModelPathComboBox(), resetModelPathComboBox(), the HoverHandler, and the ChangeListener relationship to sharedChoices) and the app still builds and runs fine i.e. nothing else in the codebase depends on them. May be worth cleaning up? Unless I'm missing something.

• Also, it looks like lastSelectedPathIndex is read as soon as it's assigned (fed straight into modelPathComboBox.setSelectedId(-1) in resetState(), or into a debug log line in onChange). Since it's never used elsewhere for actual logic, maybe we could remove the variable from the file? That's unrelated to this PR, but since we're updating the file, I just thought that might be worth cleaning up too?

[ChughVansh]

sorry accidentally closed it

[ChughVansh]

callbacks :)

[2cylu2]

Hello!

I think it looks great! Went through the code and tried some models and everything seems to work! Here are some notes for in ModelSelectionWidget.h; all optional and would love to hear your thoughts.

• Since model selection/loading is now handled by HomeTab, there looks to be some dead code. modelSelectionWidget inside ModelTab is never addAndMakeVisible'd (it's pinned to 0,0,0,0 in resized()), and I think it's the only place ModelSelectionWidget is instantiated in the codebase. I tried removing loadModelButton, modelPathComboBox, and the associated pieces of code (initializeLoadModelButton(), initializeModelPathComboBox(), resetModelPathComboBox(), the HoverHandler, and the ChangeListener relationship to sharedChoices) and the app still builds and runs fine i.e. nothing else in the codebase depends on them. May be worth cleaning up? Unless I'm missing something.
• Also, it looks like lastSelectedPathIndex is read as soon as it's assigned (fed straight into modelPathComboBox.setSelectedId(-1) in resetState(), or into a debug log line in onChange). Since it's never used elsewhere for actual logic, maybe we could remove the variable from the file? That's unrelated to this PR, but since we're updating the file, I just thought that might be worth cleaning up too?

Thanks for the review! Yes, those cleanups sound good!

[VedMistry42]

Hey!
Left a request changes after building and testing locally. I found a few issues, most notably 2 crash/hang scenarios which I think need to be sorted before merging. Details are below:

1. Crashes when retrying a failed Process
   Repro:
   -Load a model that errors out during Process (I did Demucs while the space was asleep)
   -Hit Process and dismiss the error popup
   -Hit Process again and dismiss the error popup
   -App crashes

2. App hangs after closing a tab mid-request
   Repro:

• Start processing any model
• Close tab out while processing
• Loading a new model gets stuck on "Model: Querying Controls" permanently (sometime the 1 after that too)

3. Tutorial UI cases

• steps that are supposed to switch tabs don't, so the UI looks frozen
• Media Clipboard step doesn't highlight anything even though it says so
• Interface Summary highlights the wrong region
• Spam clicking Next creates multiple duplicate model tabs
• finishing/closing the tutorial while on the model tab leaves that tab blank

[VedMistry42]

Opened & merged #440 into this branch addressing the bugs stated previously (Crash on retrying a failed Process, App hang/crash on closing a tab or quitting mid-request, & Tutorial UI cases)