Show error when Dart SDK is not set correctly (#636) #637

skybrian · 2017-01-18T03:26:28Z

No description provided.

alexander-doroshko · 2017-01-18T07:08:12Z

Flutter users do not need to use the Dart page in Preferences and to know much about Dart SDK. I'd suggest rephrasing: it is Flutter SDK that is not set, and it is Flutter page in Settings (Linux) or Preferences (Mac) where user should set Flutter SDK.
Yes, under the hood IDE will automatically change Dart SDK as well (to bin/cache/dart-sdk), but this is not something user should care about.
P.S. I believe this check has already been there at some point, @pq should remember better.

skybrian · 2017-01-18T07:48:05Z

I tested the error message by changing the Dart SDK, so that does work. I guess what you're saying is that modifying the Flutter SDK also works?

If the user sets the Flutter SDK and then the Dart SDK, it seems like that might lead to confusion, if they don't know they're linked? I wonder if there is anything we can do to make this more obvious. Perhaps in a Flutter module, the Dart SDK should appear to be locked down (no edit allowed).

alexander-doroshko · 2017-01-18T08:53:00Z

You are right, there's a field for the Dart SDK in the Dart page and for the Flutter SDK in the Flutter page. And inside the model used by the IDE it is the same setting. Changing one SDK path affects another SDK path and vice versa. It may be not obvious for users.

Indeed, you may be confused if you change both SDKs in one shot. Only one of them wins and the next time you open Preferences you'll see it.

Also you may be confused if you change one SDK path and then move to another page. For example you change Flutter SDK path and open Dart page without closing the Preferences dialog. You'll see that Dart SDK path is not updated at this moment. Only when you press OK and then open Preferences again you'll see the new path in the 2nd page.

I'm afraid I do not know a good fix for this confusion at the moment.

What I'm saying is that Flutter users do not have to care about the Dart page and Dart SDK field. So all messages related to the Flutter settings could mention only Flutter page and Flutter SDK field.

Perhaps in a Flutter module, the Dart SDK should appear to be locked down (no edit allowed).

We may think about some solution like this. The problem is that in IntelliJ IDEA a project may contain a lot of modules of different nature. Three Java modules, five Dart modules, two Flutter modules, etc. Dart/Flutter SDK in 2016.3 is set per the whole IDE - once changed it affects all projects. In 2017.1 it is improved, and SDK is set per project. Different projects may have different SDKs configured. But still not per module.

pq · 2017-01-18T13:29:01Z

P.S. I believe this check has already been there at some point, @pq should remember better.

Right. It was in this commit:

d6b1cd1

Given all the nuances, how do you feel about restoring WrongDartSdkConfigurationNotificationProvider? It seems like the case we really want to flag is where an incompatible Dart SDK is set and this in inspection does just that.

pq · 2017-01-18T14:01:47Z

Ah, wait, it looks like that logic just got moved (see: fcbb090). It's now here:

io.flutter.inspections.SdkConfigurationNotificationProvider#createWrongSdkPanel

this should generate an editor notification in this case. If it's not we should fix that! (Or if that's not the approach we want to take we should remove it.)

alexander-doroshko · 2017-01-18T14:03:11Z

With the current model I wouldn't say 'wrong Dart SDK'. I would say 'Flutter SDK not specified'. Updating SdkConfigurationNotificationProvider looks good to me. That's exactly the TODO that you added in its createNoFlutterSdkPanel() that Brian suggests to implement. I only mentioned that you can avoid word 'Dart SDK' 'Dart page in Settings', 'bin/cache/dart-sdk'.

Link to open Flutter page would be great for sure as Brian mentioned in the new TODO.

BTW, createWrongSdkPanel() seems to be unreachable with the current model. As well as
// TODO(devoncarew): Recommend to set up with Flutter's dart sdk. - dead branch too.

alexander-doroshko · 2017-01-18T14:05:51Z

this should generate an editor notification in this case. If it's not we should fix that!

it's a dead branch. And the fix is the TODO that you left in createNoFlutterSdkPanel() - exactly what Brian is suggesting :)

pq · 2017-01-18T14:43:55Z

Clearly I need more coffee... and we have some dead code to cleanup!

skybrian · 2017-01-19T01:14:52Z

Trying this out, if the error tells the user to change the Flutter SDK, it will be confusing because it looks like the Flutter SDK is already set correctly in the UI.

It looks like we populate mySdkCombo in addKnownSDKPathsToCombo() and the first item is selected by default, but this doesn't actually have any effect on the rest of the system. It seems like we need to somehow represent 'Flutter SDK not set' in the preferences UI, so that the user will have something to do, and then when they save it, it will be fixed.

Also, the 'Apply' button is weird. It apparently has no effect (the UI doesn't update right away) but in the debugger I see that it does set the Dart SDK. So we need to fix that too.

skybrian · 2017-01-19T01:55:09Z

Another problem: the device selector does not appear when it should. If you didn't have the Flutter SDK properly configured, and you configure it, doesn't show up.

It looks like it's listening to FlutterSdkManager which doesn't get an event when the Dart SDK changes.

I think FlutterSdkManager.Listener should fire events for this, but that's not what the comment says. It says it should be fired when the "Flutter global library is set".

(Also, I don't understand why FlutterSdkManager uses ApplicationLibraryTable. Is that table even used for Dart?)

Fix combo box to display correctly when no Flutter SDK is configured. Fire a Flutter SDK changed event when the user changes the Flutter SDK. Remove dead code.

skybrian · 2017-01-19T03:57:14Z

Please take another look. This doesn't handle everything, but I made enough changes so that it mostly looks right when the user changes the Flutter SDK in response to the error.

But the Apply button still doesn't work, and it's unclear why. Current behavior is that the Flutter Run Configuration gets greyed out when you click Apply, but the device menu doesn't appear until after you close the settings dialog.

pq

General question: I'm not seeing how the case where the Dart SDK is set to a non-Flutter SDK is handled. This was a BIG source of confusion with users early on and I'd hate to remove the safety net. (But maybe we're not and I'm just not seeing it?)

pq · 2017-01-19T16:58:20Z

src/io/flutter/inspections/SdkConfigurationNotificationProvider.java

@@ -47,30 +47,18 @@ public SdkConfigurationNotificationProvider(@NotNull Project project) {
    this.project = project;
  }

-  @Nullable
-  private static EditorNotificationPanel createWrongSdkPanel(@NotNull Project project, @Nullable Module module) {


How is this case being handled?

it is not possible

alexander-doroshko · 2017-01-19T21:24:29Z

I'm not seeing how the case where the Dart SDK is set to a non-Flutter SDK is handled.

It is not possible to configure both Dart SDK and Flutter SDK in such a way that they are in conflict. Because under the hood it is ONE setting.
If you change Dart SDK in Preferences to non-Flutter's and press OK - at this moment you loose Flutter SDK setting for this project. This is the case 'Flutter SDK is not specified'. You can fix it in 2 ways: either change Dart SDK in the Dart page or set Flutter SDK in the Flutter page. Under the hood these are absolutely equivalent actions.

skybrian · 2017-01-19T21:29:20Z

There is still an issue that no "Flutter SDK changed" event is sent when the user sets the Dart SDK. But, this error message doesn't rely on events - it does a query each time.

pq · 2017-01-19T21:57:18Z

It is not possible to configure both Dart SDK and Flutter SDK in such a way that they are in conflict. Because under the hood it is ONE setting.

Something is not right then... I just got into this state by setting my Dart SDK preferences.

skybrian · 2017-01-19T23:27:11Z

Yes, when no Flutter SDK is set, the combo box is displayed wrong. (It displays the first entry.) I fixed that too.

However, I just noticed that the "Version:" text is still wrong. Working on that.

alexander-doroshko · 2017-01-20T08:17:30Z

Something is not right then...

Notice a 'Reset...' link in the top-right of your 2nd screenshot :). It is a marker of a bug: the page is in the incorrect state right after opening it. 'Reset...' (as well as enabled Apply button) automatically appears when UI doesn't correspond to the stored model. On page opening there must be neither Reset nor Apply,

skybrian requested a review from pq January 18, 2017 03:26

googlebot added the cla: yes label Jan 18, 2017

skybrian mentioned this pull request Jan 18, 2017

No error message if Dart SDK is not set or is set incorrectly #636

Closed

Show error when Dart SDK is not set correctly (#636)

d85441d

Change message to 'No Flutter SDK configured'

d92fc7d

Fix combo box to display correctly when no Flutter SDK is configured. Fire a Flutter SDK changed event when the user changes the Flutter SDK. Remove dead code.

skybrian requested a review from alexander-doroshko January 19, 2017 03:53

pq reviewed Jan 19, 2017

View reviewed changes

skybrian closed this Jan 19, 2017

skybrian deleted the check-dart-sdk branch January 19, 2017 21:42

pq mentioned this pull request Jan 19, 2017

Show error when Dart SDK is not set correctly (#636) #646

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show error when Dart SDK is not set correctly (#636) #637

Show error when Dart SDK is not set correctly (#636) #637

skybrian commented Jan 18, 2017

alexander-doroshko commented Jan 18, 2017

skybrian commented Jan 18, 2017 •

edited

Loading

alexander-doroshko commented Jan 18, 2017 •

edited

Loading

pq commented Jan 18, 2017

pq commented Jan 18, 2017

alexander-doroshko commented Jan 18, 2017

alexander-doroshko commented Jan 18, 2017

pq commented Jan 18, 2017

skybrian commented Jan 19, 2017

skybrian commented Jan 19, 2017

skybrian commented Jan 19, 2017

pq left a comment

pq Jan 19, 2017

alexander-doroshko Jan 19, 2017

alexander-doroshko commented Jan 19, 2017

skybrian commented Jan 19, 2017

pq commented Jan 19, 2017

skybrian commented Jan 19, 2017

alexander-doroshko commented Jan 20, 2017

Show error when Dart SDK is not set correctly (#636) #637

Show error when Dart SDK is not set correctly (#636) #637

Conversation

skybrian commented Jan 18, 2017

alexander-doroshko commented Jan 18, 2017

skybrian commented Jan 18, 2017 • edited Loading

alexander-doroshko commented Jan 18, 2017 • edited Loading

pq commented Jan 18, 2017

pq commented Jan 18, 2017

alexander-doroshko commented Jan 18, 2017

alexander-doroshko commented Jan 18, 2017

pq commented Jan 18, 2017

skybrian commented Jan 19, 2017

skybrian commented Jan 19, 2017

skybrian commented Jan 19, 2017

pq left a comment

Choose a reason for hiding this comment

pq Jan 19, 2017

Choose a reason for hiding this comment

alexander-doroshko Jan 19, 2017

Choose a reason for hiding this comment

alexander-doroshko commented Jan 19, 2017

skybrian commented Jan 19, 2017

pq commented Jan 19, 2017

skybrian commented Jan 19, 2017

alexander-doroshko commented Jan 20, 2017

skybrian commented Jan 18, 2017 •

edited

Loading

alexander-doroshko commented Jan 18, 2017 •

edited

Loading