Code review comment for lp:~javierder/bzr-eclipse/send_command

Hi Javier,

On Fri, Apr 17, 2009 at 8:45 PM, Javier Derderian <email address hidden> wrote:
> Javier Derderian has proposed merging lp:~javierder/bzr-eclipse/send_command into lp:bzr-eclipse.
>
> Send command implementation.
>
> The dialog, action and command are based on merge and uncommit.

The branch looks good, but I have some comments regarding minor issues
I'ld like to see solved before merging.

1) please revert the changes to
org.vcs.bazaar.eclipse.client/.classpath as this reflects your
configuration of the project, but not the current default in order to
be able build the update-site.

2) org.vcs.bazaar.eclipse.core/src/org/vcs/bazaar/eclipse/core/commands/SendCommand.java:

 + public void run(IProgressMonitor monitor) throws BazaarException {
 + try {
 + monitor.beginTask(CoreTexts.SendCommand_send, 1000);
 + IBazaarClient client = getClient(resource.getIResource().getProject());
 +
 + final BranchLocation branchLocation = new BranchLocation(remoteLocation);
 +
 + client.send(branchLocation, options);
 + //annotation =
client.annotate(resource.getIResource().getLocation().toFile());

    The remoteLocation variable is handled as a plain string, this is
really discouraged, as it's a value provided by the user, also the
overall idea is that the core don't know about locations as strings
but as instances of BranchLocation class.
    The SendAction or the Dialog should do some work to validate the
user input and pass a BranchLocation to SendCommand (see below).
    Also, please remove the commented code at line 48.

* org.vcs.bazaar.eclipse.ui/src/org/vcs/bazaar/eclipse/ui/actions/SendAction.java:
 - Please fix the header of the file.
 - in isEnabledForUnmodifiedResources:
 + protected boolean isEnabledForUnmodifiedResources() {
 + if(getSelectedProjects().length > 1) {
 + return true;
 + } else if(getSelectedProjects().length == 1) {
 + return true;
 + }
 + return false;
 + }
   here you'r checking for getSelectedProjects().length > 1 and
getSelectedProjects().length == 1 and returng true in both conditions.
Ir order to make clear the intention of the code there should be only
one check: >= 1. Also take into account the side effects of this, as
the action 'll be enabled with multiple projects selected. This Might
be valid in the case that two projects are in the same branch, but are
two different projects in bzr-eclipse (This feature is supported by
bzr-eclipse but not directly available to the regular user due to
limitations in the Eclipse Team integration framework)

 * in the execute method:
 + final IBzrLocalResource[] resourcesToRevert =
BzrWorkspaceRoot.getBzrResourcesFor(projects);
 +

 (line 50): you'r naming a variable resourcesToRevert when it should
be resourcesToSend maybe, or just resources?

 + final SendCommand cmd = new SendCommand(resourcesToRevert[0],
dialog.getOptions(), dialog.getValue());

  (line 52) only the first resource is used, but as in
isEnabledForUnmodifiedResources the action is enabled for multiple
projects.

 + run(new WorkspaceModifyOperation() {
 + public void execute(IProgressMonitor monitor) throws
InvocationTargetException {
 + try {
 + cmd.run(monitor);
 + } catch (BazaarException e) {
 + // FIXME: exception handling
 + throw new InvocationTargetException(e);
 + } finally {
 + monitor.done();
 + }
 + }
 + }, false /* no cancelable */, PROGRESS_DIALOG);
 +
  (line 53..) there is an issue in the way you'r calling the
SendCommand, it'll block the entire workspace, and don't have the
cancel button enabled. I know that having real cancelable action in
bzr-eclipse isn't possible or easy at this moment, but it 'ld be nice
if the command is executed as a Job in order to allow the user to keep
using the workspace

About the branch location handling I mentioned above, take a look at
other classes that hanle branch location input, e.g: MergeAction at
line 34 and 55 where getRemoteLocation method is defined. it pass a
IBzrBranch to the MergeCommand.

Finally a few comments on SendDialog:
There are two constants: MESSAGE and TITLE, those should be
externalized to uitexts.properties.
Also it would be nice that when "Save to location" is selected, one
could click on a "browse" button in order to select the file, instead
of writting or copy&paste the path to the file.

Thats all. fix those minor issues and it's ready to land.

Great work!

Regards,

--
Guillermo Gonzalez
<http://launchpad.net/~verterok>