INSP: add attach file to module quick fix

[Kobzol]

This PR adds an action to DetachedFileNotificationProvider that adds the file to some module so that it will be present in the module tree. The current implementation is not yet complete because I'm not sure about the UI.

The directory of the file is checked for mod.rs, lib.rs or main.rs files and if they are valid and exist, they will be used as candidates for insertion. Currently if there is only one available module, I just insert the file into it, if there are more, some UI/dialog could show up - a select box with available modules?

Should it offer modules recursively, i.e. in grandparent modules? That would be pretty complicated if the parent module would already exist, but it would allow to include very deeply nested files (a/b/c/foo.rs, where neither a, b nor c are modules yet).

Is it possible to test this? I tried to access the intention action from the notification provider and trigger it, but that seems pretty hacky and I don't have access to an editor in the current notification provider test suite.

Fixes: #5488

[Undin]

Should it offer modules recursively, i.e. in grandparent modules? That would be pretty complicated if the parent module would already exist, but it would allow to include very deeply nested files (a/b/c/foo.rs, where neither a, b nor c are modules yet).

I think no. At least for now.

Is it possible to test this? I tried to access the intention action from the notification provider and trigger it, but that seems pretty hacky and I don't have access to an editor in the current notification provider test suite.

Let's convert it into inspection that produces file-level warning. See #5481 as example

[Kobzol]

Just to be sure, you want me to convert the whole DetachedFileNotificationProvider to an inspection, not create an additional inspection just for adding the file to a module, right? Otherwise there would be two notification bars for each file without a module tree.

[vlad20012]

Looks cool!

Personally I think that the "Add file to a module" message is not enough informative. I'd want to see a filename where the module declaration will be inserted. Maybe "Add the file to mod.rs"? Omm, sounds too confusing. "Declare the module in mod.rs"? Hmmm

Instead of hardcoding main.rs/lib.rs you could use

project.cargoProjects.findPackageForFile(virtualFIle) // CargoProjectsService.findPackageForFile

and then iterate over Package.targets crateRoots. If Package/Target terminology is confusing for you, you can read our ARCHITECTURE.md (I think our explanation is even better then Cargo's official docs =D)

If a package for a file is not found, we probably should not try to find any module for inclusion.

Also, if mod.rs is not found, we can check edition of the package, and if it is 2018, we can try to find a file in the upper directory with the same name as the directory (see no more mod.rs).

[Kobzol]

Updated module search and created a dialog for choosing the module. If there is a single module, the action button will say "Attach file to $modname", otherwise it says "Attach to a module".

Should I convert the whole RsNotificationProvider to a file-level inspection for easier testing?

[Undin]

Should I convert the whole RsNotificationProvider to a file-level inspection for easier testing?

@Kobzol yep. It will be more consistent now

[Undin]

@Kobzol Oops, you are about RsNotificationProvider. Sorry I didn't notice it. No, let's convert only DetachedFileNotificationProvider

[Kobzol]

Yeah, that's what I meant, sorry.

[Kobzol]

I converted the provider to an inspection with a quick fix, however if I try to use a GUI dialog inside the quickfix, it complaints:
java.lang.Throwable: AWT events are not allowed inside write action: java.awt.event.InvocationEvent[INVOCATION_DEFAULT,runnable=com.intellij.openapi.application.impl.FlushQueue$FlushNow@6b832df7,notifier=null,catchExceptions=false,when=1591344167928] on sun.awt.X11.XToolkit@152bb963

How should I handle module selection if there are multiple modules to select?

[Undin]

I converted the provider to an inspection with a quick fix, however if I try to use a GUI dialog inside the quickfix, it complaints:

Wow, it's a great warning!
You shouldn't show dialogs in write action because it's unpredictable how long this dialog will be shown. And all time it's shown, it blocks all other operations that require even read action.
By default, a quick fix is started in write action to avoid boilerplate in most of the cases. But if you need to show UI, you should control it yourself. Just override startInWriteAction method and start write action only when you need to apply the corresponding changes to PSI (or any other platform model)

[Kobzol]

Ah-ha! That was the missing piece, thanks. Indeed the exception message was very informative, I just forgot that it's possible to "turn off" write action in quick fixes.

[Kobzol]

The inspection doesn't offer any disabling action:

But it can still be disabled in Inspection settings. Is that ok or should I add an explicit disabling action somehow?

[vlad20012]

But it can still be disabled in Inspection settings.

Well, yes, but DetachedFileNotificationProvider can be disabled for a particular VirtualFile. See DetachedFileNotificationProvider.disablingKey

Is that ok or should I add an explicit disabling action somehow?

I think we should have the ability to disable the inspection in a particular file. It's because we still often can't find a parent module because of macros or something. So, the inspection is useful in general, but in some files, it can be very annoying. Including external library files.
We could leave the same behavior for the inspections - use PropertiesComponent, check the property before running the inspection

[vlad20012]

Could we show a relative path related to a package root for example (pkg.contentRoot or pkg.rootDirectory (they are the same but has different types))

[Kobzol]

Is there a way to modify the notification bar without creating an additional quick fix just for disabling the inspection? I tried overriding createOptionsPanel, but that does not seem to do anything. I also looked at RsSuppressQuickFix, should I modify/clone it for this inspection?

[vlad20012]

Hmm, I'm not sure how to do this better. But it seems that a new quick fix is the easiest way. It can be an inline class btw.

[Undin]

I tried overriding createOptionsPanel, but that does not seem to do anything.

@Kobzol JFYI it creates panel for inspection settings. See Preferences | Editor | Inspections | Rust | Unresolved Reference as example

[vlad20012]

Hmm. Also, maybe we should just navigate a user to the inserted mod foo; declaration after applying the fix?

[Kobzol]

I wanted to do that, but wasn't sure how to get an Editor instance. Just inherit from LocalQuickFixAndIntentionActionOnPsiElement?

[vlad20012]

Hmm, why do you need an editor instance?

[Kobzol]

Good question :D I wanted to do something like editor.caretModel.moveToOffset(inserted.textOffset). But then I realized that the editor of the inspection will be for the wrong file. What API should I use to navigate the user to the inserted mod?

[vlad20012]

modItem.navigate(true), I guess :)

[vlad20012]

It's perfect now! Could you please squash the commits?

@Undin, would you like to take a look?

[Undin]

Hmm. Also, maybe we should just navigate a user to the inserted mod foo; declaration after applying the fix?

@vlad20012 @Kobzol I have some doubts about it. Looks like it may be quite annoying. I imagine the following user case: I created a new rust module to write some new code in it but forgot to create mod declaration for it (or did it intentionally because of existence of the corresponding quick fix). I apply quick fix to get proper code insight in the file and IDE moves me into another file. Why? What should a user get from this navigation? I wanted to write new code in new file. But after navigation, I have to move back.
Also, it's inconsistent with other actions. For example, we already have quick fix to make an item public. It may be located in any place in the project (i.e. it does a similar thing - makes some changes in an arbitrary place in the project). But we don't navigate a user to the place where quick fix was applied
What do you think?

[Kobzol]

Now that I think about it, the navigation was mainly useful before there was the UI dialog, for the user to see where was the mod created.

However, inserting a mod is a bit different from the make public quick fix, because that one simply prepends a visibility restriction to an already existing item, while this quick fix creates a new line and in most cases I think that the mod ... line will be in a weird location, so the user might want to fix it. We could possibly try to find some existing mod ... item in the target file and put the new mod item below it?

Regarding new file creation, maybe we should offer a similar dialog when the user creates a new Rust file, so that it will be added to some module right away?

[vlad20012]

@Undin I know a lot of people (mostly newbies in Rust) who asked "what does this message mean" ("File is not included in module tree") because they don't understand the Rust module system. I think it's necessary to show an inserted mod declaration because otherwise, they will not understand what's happened.
Another point - there are multiple options for how a module can be introduced, and I think this quick fix doesn't cover all of them, so it can place a module to a wrong place. Navigating a user to the inserted declaration is the best way to let the user check and fix it. Also, it's simple to go back by clicking Ctrl+B, if everything looks ok

[vlad20012]

@Undin One more point - I think that this quick fix is mostly for newbies because professional users should write module declarations prior to creating files (then files can be created with another provided quick fix). So this quick fix should be rather obvious and transparent than handy for everyday usage.

[Undin]

I know a lot of people (mostly newbies in Rust) who asked "what does this message mean" ("File is not included in module tree") because they don't understand the Rust module system. I think it's necessary to show an inserted mod declaration because otherwise, they will not understand what's happened.

But what's about a situation when I've already known how the module system works?

One more point - I think that this quick fix is mostly for newbies because professional users should write module declarations prior to creating files (then files can be created with another provided quick fix)

You don't know because currently, this quick fix doesn't exist and no one can use it in development workflow.
But I don't know too. So let's try current implementation and fix it in future if there are some user complaints

[Undin]

LGHM
Thanks!
Could you squash commit please?

[vlad20012]

I just came up with another option: we can show a balloon as before, but with a button that navigates to an inserted module declaration. But ok, let's leave it as is for now

[vlad20012]

bors r+

[bors]

Build succeeded:

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push