-
Notifications
You must be signed in to change notification settings - Fork 330
Some initial work for FlutterWeb apps #3315
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,8 @@ public class FlutterApp { | |
private final @NotNull Project myProject; | ||
private final @Nullable Module myModule; | ||
private final @NotNull RunMode myMode; | ||
private final @NotNull FlutterDevice myDevice; | ||
// TODO(github.com/flutter/flutter-intellij/issues/3293) myDevice is not-null for all run configurations except flutter web configurations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bug listed doesn't cover the issue. e.g. whether Device should be null for FlutterWeb. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, the bug URL however is the searchable text that can be used to identify all locations in source that might want to be visited if this expectation switches (with a mocked Chrome device for instance.) |
||
private final @Nullable FlutterDevice myDevice; | ||
private final @NotNull ProcessHandler myProcessHandler; | ||
private final @NotNull ExecutionEnvironment myExecutionEnvironment; | ||
private final @NotNull DaemonApi myDaemonApi; | ||
|
@@ -122,7 +123,7 @@ public static FlutterApp fromEnv(@NotNull ExecutionEnvironment env) { | |
FlutterApp(@NotNull Project project, | ||
@Nullable Module module, | ||
@NotNull RunMode mode, | ||
@NotNull FlutterDevice device, | ||
@Nullable FlutterDevice device, | ||
@NotNull ProcessHandler processHandler, | ||
@NotNull ExecutionEnvironment executionEnvironment, | ||
@NotNull DaemonApi daemonApi, | ||
|
@@ -237,7 +238,7 @@ public static FlutterApp start(@NotNull ExecutionEnvironment env, | |
@NotNull Project project, | ||
@Nullable Module module, | ||
@NotNull RunMode mode, | ||
@NotNull FlutterDevice device, | ||
@Nullable FlutterDevice device, | ||
@NotNull GeneralCommandLine command, | ||
@Nullable String analyticsStart, | ||
@Nullable String analyticsStop) | ||
|
@@ -593,7 +594,12 @@ public FlutterDevice device() { | |
} | ||
|
||
public String deviceId() { | ||
return myDevice.deviceId(); | ||
return myDevice != null ? myDevice.deviceId() : ""; | ||
} | ||
|
||
// TODO this should be a temporary hack until there is some mocked out "browser" device | ||
public boolean isWebDev() { | ||
return myDevice == null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add a TODO that this is a temp hack. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO added |
||
} | ||
|
||
public void setFlutterDebugProcess(FlutterDebugProcess flutterDebugProcess) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,13 +48,28 @@ public class FlutterCommand { | |
@NotNull | ||
private final List<String> args; | ||
|
||
// TODO(github.com/flutter/flutter-intellij/issues/3293) This is a temporary "<pub-cache>/bin/webdev" that can be provided to test some | ||
// webdev directly. | ||
@Nullable | ||
private final String localWebDevExe = "webdev"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be removed before checkin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is up to you and Devon. I'd like to land it to allow you to verify that things work as expected on someone elses' machine. If your question is: should this be removed before users use it, then yes. This string only exists for line 267 below to change the generation of the command line from FlutterSdk (below) from |
||
|
||
private final boolean isFlutterWeb; | ||
|
||
/** | ||
* @see FlutterSdk for methods to create specific commands. | ||
*/ | ||
FlutterCommand(@NotNull FlutterSdk sdk, @Nullable VirtualFile workDir, @NotNull Type type, String... args) { | ||
this(sdk, workDir, type, false, args); | ||
} | ||
|
||
/** | ||
* @see FlutterSdk for methods to create specific commands. | ||
*/ | ||
FlutterCommand(@NotNull FlutterSdk sdk, @Nullable VirtualFile workDir, @NotNull Type type, boolean isFlutterWeb, String... args) { | ||
this.sdk = sdk; | ||
this.workDir = workDir; | ||
this.type = type; | ||
this.isFlutterWeb = isFlutterWeb; | ||
this.args = ImmutableList.copyOf(args); | ||
} | ||
|
||
|
@@ -245,23 +260,59 @@ public OSProcessHandler startProcessOrShowError(@Nullable Project project) { | |
public GeneralCommandLine createGeneralCommandLine(@Nullable Project project) { | ||
final GeneralCommandLine line = new GeneralCommandLine(); | ||
line.setCharset(CharsetToolkit.UTF8_CHARSET); | ||
if (isFlutterWeb) { | ||
if (workDir != null) { | ||
line.setWorkDirectory(workDir.getPath()); | ||
} | ||
if(localWebDevExe != null || !localWebDevExe.isEmpty()) { | ||
line.setExePath(localWebDevExe); | ||
line.addParameters("daemon"); | ||
if(args.contains("--debug")) { | ||
line.addParameters("--debug"); | ||
} | ||
if(args.contains("--hot-reload")) { | ||
line.addParameters("--hot-reload"); | ||
} | ||
} else { | ||
line.setExePath(FileUtil.toSystemDependentName(sdk.getHomePath() + "/bin/" + FlutterSdkUtil.flutterScriptName())); | ||
line.addParameters(args); | ||
} | ||
} | ||
else { | ||
line.withEnvironment(FlutterSdkUtil.FLUTTER_HOST_ENV, FlutterSdkUtil.getFlutterHostEnvValue()); | ||
|
||
line.withEnvironment(FlutterSdkUtil.FLUTTER_HOST_ENV, FlutterSdkUtil.getFlutterHostEnvValue()); | ||
final String androidHome = IntelliJAndroidSdk.chooseAndroidHome(project, false); | ||
if (androidHome != null) { | ||
line.withEnvironment("ANDROID_HOME", androidHome); | ||
} | ||
|
||
final String androidHome = IntelliJAndroidSdk.chooseAndroidHome(project, false); | ||
if (androidHome != null) { | ||
line.withEnvironment("ANDROID_HOME", androidHome); | ||
line.setExePath(FileUtil.toSystemDependentName(sdk.getHomePath() + "/bin/" + FlutterSdkUtil.flutterScriptName())); | ||
if (workDir != null) { | ||
line.setWorkDirectory(workDir.getPath()); | ||
} | ||
if (!isDoctorCommand()) { | ||
line.addParameter("--no-color"); | ||
} | ||
line.addParameters(type.subCommand); | ||
line.addParameters(args); | ||
} | ||
return line; | ||
} | ||
|
||
/** | ||
* Creates a FlutterWeb command line to run. | ||
*/ | ||
@NotNull | ||
public GeneralCommandLine createFlutterWebCommandLine(@Nullable Project project) { | ||
final GeneralCommandLine line = new GeneralCommandLine(); | ||
line.setCharset(CharsetToolkit.UTF8_CHARSET); | ||
|
||
line.setExePath(FileUtil.toSystemDependentName(sdk.getHomePath() + "/bin/" + FlutterSdkUtil.flutterScriptName())); | ||
if (workDir != null) { | ||
line.setWorkDirectory(workDir.getPath()); | ||
} | ||
if (!isDoctorCommand()) { | ||
line.addParameter("--no-color"); | ||
} | ||
line.addParameters(type.subCommand); | ||
line.addParameters(args); | ||
|
||
return line; | ||
} | ||
|
||
|
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.
Does removing this check cause any problems for regular flutter apps?
An alternate that might be more robust is to create a mock ChromeBrowser Device to use for FlutterWeb.
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.
No, it does not as it can't be null unless it is a FlutterWeb execution.
Adding a comment to this effect.