Action analytics #2233
Action analytics #2233
Conversation
…llij into action_analytics # Conflicts: # src/io/flutter/view/FlutterView.java
|
Oh I see a build break... I'll fix that in a sec. |
|
Ok, weird, I pulled in a conflicting FlutterView during a rebase. Should be fixed now. |
|
Sorry for the noise. This is green now. |
| * Template method. Implement @performAction. | ||
| */ | ||
| @Override | ||
| public final void actionPerformed(AnActionEvent e) { |
There was a problem hiding this comment.
Final methods often end up causing unexpected problems. It would be better to omit that modifier.
There was a problem hiding this comment.
👍 In general I hear you. In this case though it's a template method and I don't want it accidentally overridden since the analytics call won't happen unless they think to call super.
There was a problem hiding this comment.
@OverridingMethodsMustInvokeSuper will do what you want and won't do what I don't want.
You'll need to add a library dependency on jsr305 to enable that annotation, but I think it's worth doing. (Remember to add it to all relevant module definitions. I think there are two.)
In its current form, this method appears to contradict the instructions from JetBrains:
https://www.jetbrains.org/intellij/sdk/docs/tutorials/action_system/working_with_custom_actions.html
There was a problem hiding this comment.
I don't have an opinion here, but we may want to move discussion out of a PR; perhaps when you're in the office Monday Steve?
There was a problem hiding this comment.
Sure. We can discuss and rejigger.
# Conflicts: # flutter-studio/src/io/flutter/actions/OpenAndroidModule.java
|
Ok, I've rebased and this PR is clean again. What do you guys think? |
| @NotNull | ||
| @Override | ||
| public String getAnalyticsId() { | ||
| return "openModuleInAndroidStudio"; |
There was a problem hiding this comment.
openModuleInAndroidStudio ==> OpenModuleInAndroidStudio (TitleCamelCase)
| @Override | ||
| public void actionPerformed(AnActionEvent e) { | ||
| public String getAnalyticsId() { | ||
| return "openInAndroidStudio"; |
| @Override | ||
| public void actionPerformed(AnActionEvent e) { | ||
| public String getAnalyticsId() { | ||
| return "openInXcode"; |
| @NotNull | ||
| @Override | ||
| public String getAnalyticsId() { | ||
| return "openInXcodeFromBanner"; |
There was a problem hiding this comment.
We don't have an existing pattern for this; perhaps OpenInXcode.banner?
| @NotNull | ||
| @Override | ||
| public String getAnalyticsId() { | ||
| return "openInAndroidStudioFromBanner"; |
| * Template method. Implement @performAction. | ||
| */ | ||
| @Override | ||
| public final void actionPerformed(AnActionEvent e) { |
There was a problem hiding this comment.
I don't have an opinion here, but we may want to move discussion out of a PR; perhaps when you're in the office Monday Steve?
# Conflicts: # src/io/flutter/editor/NativeEditorNotificationProvider.java
| @NotNull | ||
| @Override | ||
| public String getAnalyticsId() { | ||
| return "openInXcodeFromBanner"; |
| * Template method. Implement @performAction. | ||
| */ | ||
| @Override | ||
| public final void actionPerformed(AnActionEvent e) { |
There was a problem hiding this comment.
Sure. We can discuss and rejigger.
An approach to getting unique analytics for the 2 flavors of "open in..." native editor actions.
I'm not married to the analytics keys ("openInXcodeFromBanner", etc. are long-winded but not inconsistent with what we have already). Needless to say, very open to suggestions.
/cc @devoncarew @stevemessick