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

[RELEASE FIX] Add workaround for project switching #417

Merged

Conversation

4 participants
@tobous
Copy link
Member

commented Feb 3, 2019

This PR is a workaround for the issue #394.

This is only meant as a temporary solution for the upcoming release. Afterwards, this issue will be solved by making Saros/I an application level plugin an explicitly handling the choice which project to share.

[RELEASE-FIX][I] Add workaround to replace old project object

THIS IS A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Makes the IntellijProjectLifecycle singleton.

Introduces a wrapper for the held project object as it is changeable
during the plugin lifecycle.

Adds a workaround to replace the old project object when a new project
is opened and the currently held project object is disposed. This is
needed as the internal Saros logic otherwise tries to work on the old
project object which is no longer valid, leading to an assertion error.

This is only a very rudimentary workaround as it still leaves users in a
headless state if they open two projects and then close the one opened
first. This state is not handled gracefully by Saros (it will cause
assertion errors shown to the user in the IDE when trying to start a
session). The headless state is resolved once the user opens another
project which will then be used for Saros.

[RELEASE-FIX][I] Stop session when current project is closed

THIS IS A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Stops the current session when the shared project is closed. This is
done as the session state is dropped at that point, meaning we can no
longer work with the old session.

Furthermore, dropping the old session should guarantee that we don't
have any references to resources of the closed project. This is
necessary as trying to access these resources would lead to an assertion
error.

[RELEASE-FIX][I] Adhere to Intellij plugin lifecycle setup

THIS IS PART OF A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Moves the plugin initialization in SarosComponent from the CTOR into
initComponent() in order to adhere to the Intellij plugin lifecycle
setup guidelines. This is important as getComponent() calls are not
allowed for Intellij object from within the CTOR.

Subsequently improves the javadoc of SarosComponent.

[RELEASE-FIX][I] Display warning message when entering headless state

THIS IS A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Displays a warning message to the user in all remaining open projects
when the shareable project is closed. Closing the shareable project
leaves Saros in a headless state where it does not have access to a
valid project object to load shareable modules from. This headless state
gets resolved when the user opens a new project. The newly opened
project will be used as the shareable project.

@tobous tobous self-assigned this Feb 3, 2019

@tobous tobous added this to In progress in Saros/I via automation Feb 3, 2019

@tobous tobous requested review from m273d15 and srossbach Feb 3, 2019

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2019

Hmm, that's weird.

For whatever reason, the branch build failed because of an error in the JS part of the whiteboard. The normal tests all ran without a problem locally.

And the PR build fails because of a DataTransferManagerTest, which is weird as I didn't modify the core.

Will look into it tomorrow.

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2019

The build-failure seems to be unrelated to this PR as it can be re-created on the current master. I created issue #418 for this.

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from 8a22bc5 to 63d09b8 Feb 4, 2019

@@ -94,6 +105,16 @@ public void projectOpened() {

@Override
public void projectClosed() {
// TODO: Update project
ISarosSessionManager sarosSessionManager =

This comment has been minimized.

Copy link
@srossbach

srossbach Feb 4, 2019

Contributor

I guess this needs a lot more of explanation. If you look at the code you are just wondering why the lifecycle is not started in projectOpened and is not stopped/disposed on projectClosed

This comment has been minimized.

Copy link
@tobous

tobous Feb 4, 2019

Author Member

This is just a WIP. It seems to work, but I wanted your opinion on what other pitfalls there are with this handling.

I was also wondering why we don't do anything in projectOpened. I will look into what the CTOR and what projectOpened should be used for.

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from 63d09b8 to d5a696a Feb 11, 2019

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

Rebased onto the current master without any interactions.

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from d5a696a to 3413f34 Feb 11, 2019

@tobous tobous changed the title [WIP] Add workaround for project switching Add workaround for project switching Feb 11, 2019

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

Adjusted SarosComponent to adhere to IntelliJ plugin lifecycle setup guidelines.

Improves javadoc of SarosComponent and ProjectWrapper.

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@srossbach Is the new documentation sufficient?

* #initComponent()}.
*/
@Override
public void projectOpened() {

This comment has been minimized.

Copy link
@srossbach

srossbach Feb 11, 2019

Contributor

Isnt this called multiple times. As far as I can remember you should not expect that projectClosed() is called before projectOpened() is called.

To keep the issue simple: Isn't there a option to shutdown Saros before a new project is either opened or the user switches the project ?

This comment has been minimized.

Copy link
@tobous

tobous Feb 11, 2019

Author Member

@srossbach Yes, this gets called multiple times and yes, this can get called before the last project was closed.

The problem is that each setup method is called for ever project that is opened (CTOR, initComponent(), and projectOpened()), even if there is already an open project.

I was not sure how to handle the case that a user opens a second project, so my previous inclination was to exclude the case through the release notes (which is not a very good handling of the case). But not mentioning the problem at all was a mistake. My bad.

But thinking about it now, my preferred handling of this case would be that Saros only acts on the project that was opened first. Any additional projects will be ignored as long as there is currently an open project. (Later on, we can deal with this by always using the project that is focused by the user when trying to start a session, but I don't think this should be implemented for the first alpha. This should be done when moving to Intellij application scope.)
This would mean that we would have to check whether there is currently already a project object in the context and only add the opened project if there isn't one (no possible with the current handling, but easily adjustable; would also mean null-checks, but violations here would have thrown disposed access assertions instead anyway).

This still leaves us with a couple of problems that I don't know how to handle gracefully:

  • What do we do when the user closes the shareable project? This gets worse when there are multiple open projects. Even if we store all project object, which one do we choose?
  • The NegotiationHandler still uses a deprecated method that chooses the project object by looking at the current focus. I am not sure if it would be possible to create a weird corner case with this that leads to problems.

Furthermore, we would have to adjust SarosFileShareGroup to check whether the focused project is shareable (or update the context).

In general, there is the potential for a lot of corner cases when allowing multiple open project and I don't think we can handle them well with the current setup. So I am not sure whether allowing multiple open projects is really a good idea before moving the plugin to application scope. (I know this somewhat contradicts my statement above, but I am pondering as I am writing 😉).

We can still release a second version not long after , but I would like to get something out there soon-ish so that we at least appear on the radar and to show that something is actually happening.

This comment has been minimized.

Copy link
@m273d15

m273d15 Feb 12, 2019

Contributor

I also think that the restriction of "one open project at a time" is acceptable for the first alpha version. This (change of plugin licecycle to application lifecycle) is also a point that should we document as "ongoing work".

Show resolved Hide resolved ...intellij/src/de/fu_berlin/inf/dpp/intellij/IntellijProjectLifecycle.java Outdated
@@ -47,7 +56,9 @@ public SarosComponent(final Project project) {
LogLog.error("could not load saros property file 'saros.properties'", e);

This comment has been minimized.

Copy link
@m273d15

m273d15 Feb 12, 2019

Contributor

Check whether LogLog is required

This comment has been minimized.

Copy link
@tobous

tobous Feb 25, 2019

Author Member

As it is currently not planned that this PR remains on the master, I would prefer not dealing with it here. It does not seem to cause problems.

* #initComponent()}.
*/
@Override
public void projectOpened() {

This comment has been minimized.

Copy link
@m273d15

m273d15 Feb 12, 2019

Contributor

I also think that the restriction of "one open project at a time" is acceptable for the first alpha version. This (change of plugin licecycle to application lifecycle) is also a point that should we document as "ongoing work".

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from 3413f34 to ceb3826 Feb 14, 2019

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Fixed merge conflict with master.

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

I am currently working on a proper solution for this problem that involves moving the project object into the session context and moving the plugin to the application level. This would make this workaround unnecessary.

As this workaround introduces unnecessary wrappers and does not contribute to the state after the real solution was implemented, I don't think it should be merged onto the master.
I would suggest that, in case we want to release a first alpha before the real solution is implemented, we create a release branch that this PR gets merged onto as a final step. I would still have to adjust it before that to implement a usable behavior (probably holding the first opened project and only loading the new one into the context if there currently isn't one; the current handling of always using the last opened project is not usable as it creates a lot of trouble if a new project is opened during a session).

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from ceb3826 to 1b5b4c6 Feb 25, 2019

@tobous

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

Rebased onto current master.

Added some logging and ensured that logging is initialized in time.

Adjusted IntellijProjectLifecycle to always use the project that was opened first and to only replace the held project object when it is disposed (meaning the project was closed). This now still can't handle closing the current project with another project open, as the project that is remaining open will not be recognized by IntelliJ and using the menu at this point will lead to issue.

Adjusted SarosComponent to only end the session when the shared project is closed.

@tobous tobous changed the title Add workaround for project switching [RELEASE FIX] Add workaround for project switching Feb 28, 2019

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from 1b5b4c6 to 12bb33b Apr 17, 2019

@tobous

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Rebased the PR on the current master. Resolved a lot of path conflicts caused by the project structure renaming.

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from 12bb33b to 0605456 Apr 18, 2019

@saros-project saros-project deleted a comment from codacy-bot Apr 18, 2019

tobous added some commits Feb 3, 2019

[RELEASE-FIX][I] Add workaround to replace old project object
THIS IS A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Makes the IntellijProjectLifecycle singleton.

Introduces a wrapper for the held project object as it is changeable
during the plugin lifecycle.

Adds a workaround to replace the old project object when a new project
is opened and the currently held project object is disposed. This is
needed as the internal Saros logic otherwise tries to work on the old
project object which is no longer valid, leading to an assertion error.

This is only a very rudimentary workaround as it still leaves users in a
headless state if they open two projects and then close the one opened
first. This state is not handled gracefully by Saros (it will cause
assertion errors shown to the user in the IDE when trying to start a
session). The headless state is resolved once the user opens another
project which will then be used for Saros.
[RELEASE-FIX][I] Stop session when current project is closed
THIS IS A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Stops the current session when the shared project is closed. This is
done as the session state is dropped at that point, meaning we can no
longer work with the old session.

Furthermore, dropping the old session should guarantee that we don't
have any references to resources of the closed project. This is
necessary as trying to access these resources would lead to an assertion
error.

tobous added some commits Feb 11, 2019

[RELEASE-FIX][I] Adhere to Intellij plugin lifecycle setup
THIS IS PART OF A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Moves the plugin initialization in SarosComponent from the CTOR into
initComponent() in order to adhere to the Intellij plugin lifecycle
setup guidelines. This is important as getComponent() calls are not
allowed for Intellij object from within the CTOR.

Subsequently improves the javadoc of SarosComponent.
[RELEASE-FIX][I] Display warning message when entering headless state
THIS IS A RELEASE FIX! DO NOT MERGE ONTO MASTER!

Displays a warning message to the user in all remaining open projects
when the shareable project is closed. Closing the shareable project
leaves Saros in a headless state where it does not have access to a
valid project object to load shareable modules from. This headless state
gets resolved when the user opens a new project. The newly opened
project will be used as the shareable project.

@tobous tobous changed the base branch from master to release/intellij/0.1.0 May 2, 2019

@tobous tobous force-pushed the pr/intellij/fix_project_assertion_error branch from 0605456 to fa55380 May 2, 2019

@codacy-bot

This comment has been minimized.

Copy link

commented May 2, 2019

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
- Added 1
           

Complexity increasing per file
==============================
- intellij/src/saros/intellij/SarosComponent.java  1
- intellij/src/saros/intellij/IntellijProjectLifecycle.java  1
         

See the complete overview on Codacy

@saros-project saros-project deleted a comment from codacy-bot May 2, 2019

@tobous

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Switched the target of the PR to the newly created release branch for Saros/I. Rebased the PR on the current release branch. Added a warning message informing the user when they are entering a headless state.

@m273d15 m273d15 merged commit c2d56f3 into release/intellij/0.1.0 May 17, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details

Saros/I automation moved this from In progress to Done May 17, 2019

@m273d15 m273d15 deleted the pr/intellij/fix_project_assertion_error branch May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.