Fix Issue 390: Better generation process monitor #393
Conversation
Replace isRunning flag with CustomProgressIndicator to better manage the test generation process.
pderakhshanfar
added
bug
| @@ -11,4 +11,5 @@ interface CustomProgressIndicator { | |||
| fun isCanceled(): Boolean | |||
| fun start() | |||
| fun stop() | |||
| fun isRunning(): Boolean | |||
There was a problem hiding this comment.
Note: It'll require publishing a new core version since the interface changes.
CC: @pderakhshanfar
| if (indicator != null && | ||
| indicator!!.isRunning() | ||
| ) { |
There was a problem hiding this comment.
the expression is quite short, so making it as this is fine:
if ((indicator != null) && indicator!!.isRunning()) {| @@ -26,6 +27,7 @@ class TestGenerationController { | |||
| private fun showGenerationRunningNotification(project: Project) { | |||
| val terminateButton: AnAction = object : AnAction("Terminate") { | |||
| override fun actionPerformed(e: AnActionEvent) { | |||
| indicator?.stop() | |||
There was a problem hiding this comment.
[Only my thoughts below; no request to any action]:
I genuinely do not like that we bring a dependency on CustomProgressIndicator further in the control flow.
Valich suggested us to transition to coroutines and reduce our CustomProgressIndicator interface to a viewer of the indicator's text (i.e. it should merely allow setting the indicator's display text).
Note: Calling stop method on the IntelliJ's ProgressIndicator is usually discouraged, as I saw in its Javadoc.
I suggest a major refactoring somewhere in the future after the release to tackle this long-lasting issue with CustomProgressIndicator. There was an issue somewhere in this regard.
| // If indicator is null, we have never initiated an indicator before and there is no running test generation | ||
| if (indicator == null) { | ||
| return false | ||
| } | ||
|
|
||
| if (indicator!!.isRunning()) { |
There was a problem hiding this comment.
how about:
fun isGeneratorRunning(project: Project): Boolean {
// If indicator is null, we have never initiated an indicator before and there is no running test generation
return indicator?.let { it.isRunning() } ?: false;
}
// OR:
fun isGeneratorRunning(project: Project): Boolean = indicator?. let { it.isRunning() } ?: false;| try { | ||
| toolWindow?.hide() | ||
| } catch (_: AlreadyDisposedException) {} // Make sure the process continues if the tool window is already closed |
There was a problem hiding this comment.
It's better to log the error at least; silencing it potentially may bring problems in the future.
| generatedTestsTabData.contentManager?.removeContent(generatedTestsTabData.content!!, true) | ||
| ToolWindowManager.getInstance(project).getToolWindow("TestSpark")?.hide() | ||
| coverageVisualisationTabBuilder.closeToolWindowTab() | ||
| } catch (_: AlreadyDisposedException) {} // Make sure the process continues if the tool window is already closed |
| val terminateButton: AnAction = object : AnAction("Terminate") { | ||
| override fun actionPerformed(e: AnActionEvent) { | ||
| indicator?.stop() |
There was a problem hiding this comment.
It would be nice to close this popup modal after a user clicks "Terminate." Otherwise, I can click as many times as I like (it does not cause errors, though).
…eration-process-monitor
arksap2002
deleted the
pderakhshanfar/bug-fix/390-better-generation-process-monitor
branch
Description of changes made
This PR fixes #390 issue and provides a better process monitoring procedure
Why is merge request needed
Fixes #390
What is missing?
We need a major refactoring by removing controller totally and using indicator instead to track the status of test generation processes