Skip to content

IntelliJ-enable the plugin template #10429

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.

Sign up for GitHub

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

Jump to bottom
Merged
mit-mit merged 1 commit into from
Jun 6, 2017
Merged

IntelliJ-enable the plugin template #10429

mit-mit merged 1 commit into from
Jun 6, 2017

Conversation

@mit-mit
Copy link
Member

@mit-mit mit-mit commented Jun 1, 2017 •
edited
Loading

Fixes #10115

@mit-mit
Copy link
Member Author

mit-mit commented Jun 1, 2017

Tested with:

flutter create --plugin test
cd test
idea .

Everything passes analysis + hitting Run starts the example app displaying the OS of the phone.

@mit-mit mit-mit requested review from devoncarew and skybrian June 1, 2017 13:00
@branflake2267
Copy link

branflake2267 commented Jun 1, 2017

idea did you create that command? That's clever.

@eseidelGoogle
Copy link
Contributor

eseidelGoogle commented Jun 1, 2017 •
edited
Loading

https://www.jetbrains.com/help/idea/2017.1/using-command-line-tools.html

@devoncarew
Copy link
Contributor

devoncarew commented Jun 1, 2017

lgtm!

skybrian
skybrian approved these changes Jun 1, 2017
View reviewed changes
Copy link
Contributor

@skybrian skybrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

packages/flutter_tools/templates/plugin/.idea/runConfigurations/main_dart.xml.tmpl Outdated
Copy link
Contributor

@skybrian skybrian Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline

packages/flutter_tools/templates/plugin/.idea/workspace.xml.tmpl Outdated
Copy link
Contributor

@skybrian skybrian Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what is this for?

Copy link
Member Author

@mit-mit mit-mit Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this causes the listed file to be open by-default in the code editor. It's a great way of indicating to the developer which of the many, many files in the project they should start with.

Hixie
Hixie reviewed Jun 1, 2017
View reviewed changes
packages/flutter_tools/templates/plugin/.idea/libraries/Dart_SDK.xml.tmpl Outdated
Copy link
Contributor

@Hixie Hixie Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is scary. For example, why is web_sql in this list? Shouldn't dart:ui be there instead? When would the user edit this file?

Copy link
Contributor

@skybrian skybrian Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a start:

This is how the Dart plugin writes out the location of the Dart SDK in an IDEA project. It was changed recently - this used to be a global setting and it's now a per-project setting. It lists all core libraries because the same SDK can be used to run both Dart and Flutter code. I don't know how often the Dart plugin updates the list from the Dart SDK itself.

We don't expect users to edit this file. That's why it's in the .idea directory.

It's also used by the Flutter plugin; we derive the location of the Flutter SDK from the Dart SDK.

We've been discussing other ways to determine the location of the Flutter SDK. See discussion in #10096.

Copy link
Contributor

@Hixie Hixie Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't expect users to edit this file, should we somehow keep it in the flutter repo? How will they get updates?

Copy link
Contributor

@skybrian skybrian Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be in the flutter repo because it's how the IDE locates the flutter repo. (Also, everything in the .idea directory is part of IntelliJ's world so we can't just change the file location or format.)

An alternate approach would be to record the path to the Flutter SDK somewhere else, and the Flutter plugin can set the location of the Dart SDK on project open, which will cause the Dart plugin to generate this file. That way it doesn't have to be in the template.

So far the best idea seems to be to derive it from the .packages file. That would work fine for flutter create but maybe not so great after doing a git clone on a Flutter app's repo, since flutter packages get hasn't been run yet. Then again, the location of the flutter repo is machine-specific, so making the user choose it might not be so bad?

(I think @pq has some plans to work on the getting started experience.)

@Hixie
Copy link
Contributor

Hixie commented Jun 1, 2017

I really think we should fix #9884 before we add anything more to the template.

@Hixie
Copy link
Contributor

Hixie commented Jun 1, 2017

I'm really concerned about hard-coding references to the path of the SDK in files we expect the user to check in to their repo. I think we should find better solutions to solve that problem as per the discussion in #10096.

I'm really concerned about generating files that the user is not expected to edit, and that we do expect to edit, since that will lead to the user getting out of date.

I'm somewhat concerned about hard-coding IDE-specific information in the template. Will this scale once we support a dozen IDEs? If Joe's Fancy IDE wanted to support Flutter, would we accept that they add their configuration files to our templates? Just like the Flutter framework does not have a special place in the Engine's worldview, and just like the Material layer does not have a special place in the Widgets layer's worldview, I don't think any particular IDE should have a special place in the Flutter's worldview. We are building a layered system.

I'm concerned that our template generates a lot of files that users don't understand, and I really think we should document all these files as per issue #9884 before adding more such files.

For all these reasons, I think rather than fixing #10115 in this way, we should take a step back and attempt to solve these general problems first, in a long-term sustainable way.

Please let me know if there's a reason why we should have a short-term fix (e.g. if we're planning a demo or something that is broken by #10115).

@mit-mit
Copy link
Member Author

mit-mit commented Jun 2, 2017

This fix makes the plugin template consistent with the application template -- both for good (you can now run a plugin in IntelliJ just like you can run an app), and for bad (we get more files).

But note that the issues you point out are identical to the existing application template. I literally copied the files you noted issues with over from the application template.

So I want to land this, and then track the potential architectural change to how we handle template files as a consistent change across all of our templates.

@Hixie
Copy link
Contributor

Hixie commented Jun 2, 2017

Fair enough, in the interests of consistency.

@skybrian skybrian mentioned this pull request Jun 5, 2017
Closed
@mit-mit mit-mit merged commit 0f277fc into flutter:master Jun 6, 2017
@mit-mit mit-mit deleted the pluginlaunch branch June 6, 2017 15:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Hixie Hixie Hixie left review comments

@devoncarew devoncarew Awaiting requested review from devoncarew

+1 more reviewer

@skybrian skybrian skybrian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cannot run the example app of a plugin from IDE

7 participants

@mit-mit @branflake2267 @eseidelGoogle @devoncarew @Hixie @skybrian @googlebot