Skip to content
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

Don't retreive pants options in Event Dispatch Thread #476

Merged
merged 6 commits into from
Jan 14, 2020
Merged

Don't retreive pants options in Event Dispatch Thread #476

merged 6 commits into from
Jan 14, 2020

Conversation

tpasternak
Copy link
Contributor

FileChangeTracker methods are always called in Event Dispatch Thread.
Pants Plugin implementation sometimes needs to retrieve pants options
in order to decide, whether to mark project as dirty. Unfortunatly,
Pants Options are retrieved via an external process call, what is
forbidden in the Event Dispatch Thread.

In this commit, the bahvior of the listener is changed in that way,
the pants options are retrieved before the listener is initialized,
on a thread from thread pool, and they're passed to listener's
constructor.

Unfortunately, pants options may be read only when a project
contains any modules. Therefore, it we can't just read them at
projectOpened stage, because the project may be initially empty.
That's why we register a ModuleListener in case when the project is
empty.

Fixes #463

FileChangeTracker methods are always called in Event Dispatch Thread.
Pants Plugin implementation sometimes needs to retrieve pants options
in order to decide, whether to mark project as dirty. Unfortunatly,
Pants Options are retrieved via an external process call, what is
forbidden in the Event Dispatch Thread.

In this commit, the bahvior of the listener is changed in that way,
the pants options are retrieved before the listener is initialized,
on a thread from thread pool, and they're passed to listener's
constructor.

Unfortunately, pants options may be read only when a project
contains any modules. Therefore, it we can't just read them at
`projectOpened` stage, because the project may be initially empty.
That's why we register a ModuleListener in case when the project is
empty.

Fixes #463
return new VirtualFileListener() {

void markDirty(VirtualFile file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
Copy link
Contributor

Choose a reason for hiding this comment

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

All these new imports are suspicious as there are no further changes in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -104,10 +104,10 @@ public FileChangeTracker getInstance() {
return instance;
}

private static void markDirty(@NotNull VirtualFile file, @NotNull VirtualFileListener listener) {
private static void markDirty(@NotNull VirtualFile file,final PantsOptions pantsOptions, @NotNull VirtualFileListener listener) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can run formatter on this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


/**
* VSF tracker implementation requires PantsOptions object, that cannot be constructed in
* the Event Dispatch Thread. What is more, it can't be constructed if the project contain
Copy link
Contributor

Choose a reason for hiding this comment

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

contains

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* needed:
* 1. When the Pants project is opened and it already contains modules - then we can register tracker immediately
* 2. When the Pants project is opened, but modules are not loaded yet - then we have to wait until a module is loaded
* @param project
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* @param project
*/
private void registerVfsListener(Project project) {
if(ModuleManager.getInstance(project).getModules().length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also run formatter on this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Tomasz Pasternak added 2 commits January 13, 2020 14:38
1. makeDirty marked private
2. removed unused import
3. formatter run some files
Copy link
Collaborator

@wisechengyi wisechengyi left a comment

Choose a reason for hiding this comment

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

Thanks! modulo same formatting and nits.

@@ -3,6 +3,7 @@

package com.twitter.intellij.pants.service.project.metadata;

import com.intellij.openapi.application.ApplicationManager;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no change in this file.

@wisechengyi wisechengyi merged commit c979d5d into pantsbuild:master Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synchronous execution on EDT
3 participants