-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix internal error on quick project close & open #447
Conversation
.orTimeout(4, TimeUnit.SECONDS) | ||
.getNow(false) == true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen the error coming from this place. I managed to get it twice but it's a race (hard to reproduce). This approach will not throw anything.
.orTimeout(4, TimeUnit.SECONDS) | ||
.getNow(false) == true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. I did the same thing in other places where it seemed relevant.
val notify = didStatusChanged(project) | ||
if (notify) { | ||
updateCodyStatusBarIcons() | ||
if (!project.isDisposed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another issue - the same scenario (a quick open & close project) - this one is (almost) deterministic. Should not be a case anymore.
ApplicationManager.getApplication().invokeLater { | ||
UpgradeToCodyProNotification(title, content, shouldShowUpgradeOption).notify(project) | ||
} | ||
UpgradeToCodyProNotification(title, content, shouldShowUpgradeOption).notify(project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrel: redundant invokeLater
- notify
handles the threading already.
@@ -159,8 +159,8 @@ private static String doReplacements( | |||
.thenCompose( | |||
agent -> | |||
agent.getServer().convertGitCloneURLToCodebaseName(new CloneURL(cloneURL))) | |||
.completeOnTimeout(/* value= */ null, /* timeout= */ 4, TimeUnit.SECONDS) | |||
.get(); | |||
.orTimeout(4, TimeUnit.SECONDS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why that change?
I do not understand how .orTimeout(...).getNow(null)
is supposed to work, orTimeout
throws exception after a given time, but getNow
returns immediately. That orTimeout
will do nothing there. I think .completeOnTimeout(...).get()
was in fact proper solution.
Please elaborate :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I did think about it. Probably getnow. Ignores the timeout 🤔 anyway. My point is I want to ensure that the timeout applies and get will not throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one comment but it applies to a few places. Please do not merge before claryfying.
4e10c98
to
44cc35d
Compare
44cc35d
to
7f933ea
Compare
…ect close; similar fixes
…ect close; similar fixes - V2
7f933ea
to
894a095
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Test plan