lppedd / idea-conventional-commit

Context and template-based completion for conventional/semantic commits.
https://plugins.jetbrains.com/plugin/13389-conventional-commit
MIT License
327 stars 19 forks source link

Replacing internal API used by `CCApplicationLoadListener` #118

Closed bric3 closed 1 year ago

bric3 commented 1 year ago

Plugin verification is now forbidding Internal APIs, this commit replaces the internal mechanism by combining StartupActivity and RunOnceUtil.

'com.intellij.ide.ApplicationLoadListener' is marked unstable with @ApiStatus.Internal
Overridden method 'beforeApplicationLoaded(com.intellij.openapi.application.@org.jetbrains.annotations.NotNull Application, java.lang.@org.jetbrains.annotations.NotNull String)' is declared in unstable interface 'com.intellij.ide.ApplicationLoadListener' marked with @ApiStatus.Internal
lppedd commented 1 year ago

Just be aware that ApplicationLoadListener is called further up in the call stack, so a lot before startup activities. In this case I think it's ok removing it, but in general don't worry too much about using these internal APIs. I've done way worse lol

bric3 commented 1 year ago

Yes but plugins using internal API will be rejected by default.

image

Just be aware that ApplicationLoadListener is called further up in the call stack, so a lot before startup activities.

I'm aware of that. That said, the doc indicates this extension/listener shouldn't be used by plugin developers. Otherwise there's also the AppLifecycleListener, something along the lines

<applicationListeners>
  <listener class="plugin.OnAppStartup" topic="com.intellij.ide.AppLifecycleListener"/>
</applicationListeners>
class OnAppStartup : AppLifecycleListener {
  override fun appStarted() { 
     // do something
  }
}
lppedd commented 1 year ago

Yes but plugins using internal API will be rejected by default.

That's a bit harsh tbh. I think that's just theoretical.
Otherwise how do you get rid of these workarounds, as they're both reported as internal API usages.

https://github.com/lppedd/idea-conventional-commit/blob/6fa98d20b8b6d2af2c40140ef97d2132b1acff29/src/main/kotlin/com/github/lppedd/cc/completion/ConventionalCommitTextCompletionContributor.kt#L259-L290

https://github.com/lppedd/idea-conventional-commit/blob/6fa98d20b8b6d2af2c40140ef97d2132b1acff29/src/main/kotlin/com/github/lppedd/cc/ui/NoContentTabbedPaneWrapper.kt#L51-L58

But this change in particular is definitely ok, it makes no sense using ApplicationLoadListener anymore.

bric3 commented 1 year ago

I understand their point of view regarding API support. We'll see how this is implemented in the coming months. But I bet they will tighten their process here.

I'm not sure about the API quoted above. I can certainly see why they wouldn't want plugin developers to forbid flushQueue().

bric3 commented 1 year ago

Seems like BundleBase is also internal

https://github.com/lppedd/idea-conventional-commit/blob/d6e4310a8414cdbb8d9f355515fa4d7d8f3ca547/src/main/kotlin/com/github/lppedd/cc/CCBundle.kt#L28-L28

And there other usage of CompletionProgressIndicator there

https://github.com/lppedd/idea-conventional-commit/blob/d6e4310a8414cdbb8d9f355515fa4d7d8f3ca547/src/main/kotlin/com/github/lppedd/cc/completion/providers/FooterValueCompletionProvider.kt#L73-L76

lppedd commented 1 year ago

Let's not worry about it at all for now. We'll get back at it if they push back updates.

lppedd commented 1 year ago

Merged, thanks!

lppedd commented 1 year ago

@bric3 I will temporarily revert to the ApplicationLoadListener. See DM on Slack.