cloudfoundry-attic / eclipse-integration-cloudfoundry

Cloud Foundry Integration for Eclipse
Apache License 2.0
41 stars 47 forks source link

Fix a bug when dragging and dropping a local project into the applicat... #22

Closed liuhewei closed 10 years ago

liuhewei commented 10 years ago

...ionViewer.

If there is already a cloud application with the same name as the local project, the cloud application will be deleted and replaced by the local project, which is "not yet deployed".

Fig1 drag and drop local project with the same name drag&drop

Fig2 existed cloud application is replaced replaced

There should better be a warning message and not delete the cloud application which already exists.

Fig3 better jump out a warning message warning

cfdreddbot commented 10 years ago

Hey liuhewei!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

nierajsingh commented 10 years ago

Hi,

Thanks very much for submitting this Pull Request. We are currently at the end of the release cycle for Cloud Foundry Eclipse version 1.7.1 and in testing stage for the release candidate, so we'll be reviewing and accepting any open Pull Requests starting next week for the next release cycle once 1.7.1 is released.

We will follow up on the PR next week once we review and test it, and if the fix is fine, it will be be merged for our next release 1.7.2 which is scheduled approximately at the end of September.

Thank you.

nierajsingh commented 10 years ago

There were some merge conflicts with the moduleName. Fixed merge conflicts, tested the fix, and merged PR. Closing.

nierajsingh commented 10 years ago

Thanks for submitting this fix. As a further note, we will be making changes to this behaviour later. Eventually, we want to support replacing of an existing module with a workspace project via drag/drop, so a future change to this PR will have the dialogue showing a warning, asking a user whether to replace the existing deployed application, and then deploying the workspace project after deleting the existing deployed application. If the user clicks "Cancel", then the effect will be the same as this PR's error dialogue indicating that module with same project name already exists and canceling the deployment.

liuhewei commented 10 years ago

@nierajsingh Yes. Actually, what you said, to support replacing is what we're doing now. If it's not conflicted with your plan, we can submit another PR for it with pleasure.

nierajsingh commented 10 years ago

Hi,

Thanks for letting us know about your plans to support application replacement. We've actually already just started working on this as well. Since our work may be overlapping, we should coordinate development.

The work that we've started is primarily with the ApplicationMasterPart, where in the current implementation, we always force an application publish via:

cloudServer.getBehaviour().publishAdd(moduleName, monitor);

in the performDrop(final Object data) method.

However, we want to replace this, and rely on a modifyModules call on the CF server instance, and pass in a list of applications that we are adding and removing. This is how we plan on "replacing" an existing application. We determine the list of applications to replace (i.e. to remove) from:

ApplicationViewersDropAdapter#determineTarget(...)

In addition, we are also going to re-enable publishing preferences for a CF server, as at the moment we have the "Publishing" section in the CF editor "Overview" tab disabled. With publishing preferences, we will no longer be automatically publishing an application on each drag/drop,but instead have it behave more like the Add/Remove application wizard in the CF editor, where a user can select whether to publish immediately, or just create the modules, and publish later.

Also, to support accessibility use-cases, we'll be adding UI like context menus to the Servers view that will perform the same replacement behaviour as drag and drop via a wizard, where you right-click on an existing application in the Servers view, and then select which workspace project to replace it with.

We've just started to work on this, so we haven't gotten too far.

We do want to have this in for our next release, 1.7.2, which is scheduled by the end of September.

If you already have a PR to submit soon, we can hold off on further development on our end and see what approach you've taken before we move forward.

Thank you for doing this work, and we look forward to seeing your PR.

-Nieraj

liuhewei commented 10 years ago

@nierajsingh Actually, the backend part for this app replacement feature is almost finished, but our frontend part is a little different from your description, especially on the context menu. We'll submit a PR in this week with some screeshots, and then we can discuss on the implementation which I think would be more effective.

Thanks.