Conversation
adkozlov
force-pushed
the
adkozlov/ALT-11006
branch
from
2e78d8b to
651bd9d
Compare
meanmail
left a comment
meanmail
left a comment
There was a problem hiding this comment.
Overall the fix looks good — try/finally is the right approach to guarantee combo box re-enablement. A few minor comments below.
| // Check if we found any JDK at all (either suitable or any) | ||
| if (sdksModel.sdks.all { it.sdkType != JavaSdk.getInstance() } | ||
| && ProjectJdkTable.getInstance().getSdksOfType(JavaSdk.getInstance()).isEmpty()) { | ||
| throw RuntimeException(EduJVMBundle.message("error.no.jdk.available")) |
There was a problem hiding this comment.
nit: Throwing RuntimeException for a normal business scenario (no JDKs installed) to unify error handling in the outer catch works, but reads a bit odd. An alternative would be to set FAILED state directly and skip to finally via return@runInBackground. Not a blocker — current approach is functional.
| error.no.jdk.need.at.least=JDK is not selected. In the settings section, choose or download some JDK with a version at least {0} | ||
| error.no.jdk.available=No JDK found on your system. Please <a href="{0}">configure JDK</a> in IDE settings | ||
| error.jdk.loading.failed=Failed to load JDK list. Please check your IDE settings or <a href="{0}">configure JDK manually</a> | ||
| error.no.jdk.available=No JDK found on your system. Please configure JDK in IDE settings |
There was a problem hiding this comment.
question: The <a href="{0}">...</a> links were removed from these messages. Were they previously rendered as clickable hyperlinks anywhere in the UI (e.g. in a notification or validation panel)? If so, users lose a quick shortcut to JDK settings. If they were never actually rendered as hyperlinks, the cleanup makes sense.
|
nit: |
2 participants
Summary
preselectJdkbackground task in try/finally to guarantee the JDK combo box is always re-enabledFAILEDloading state so validation shows a meaningful error instead of staying inPendingforeverTest plan