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

Hi,

Please comment on the merge proposal the next time you address an issue that needs to be fixed, so I can take a look to it more quickly, as I'm not subscribed to the branches.

Please take a look to the issues detailed below:
1) You are adding a new command: 'AddIgnoredCommand' that it's based on AddCommand and isn't adding anything or doing something different from the plain AddCommand. please remove the command as isn't adding any value and it's almost a copy/paste of the AddCommand.

2) In the IgnoredAction you are directly using IBazaarClient, but the UI shouldn't know anything about the client, that's the reason to have the commands in the core module, it might look like a bit of overhead, but the idea is to handle all the File's (java.io.File) and low level stuff in the core, so we can change the backend and keep the UI intact, which should use the interfaces provided by eclipse (IResource, etc) and by bzr-eclipse core module, e.g: IBzrResource, etc.
You should create a IgnoredCommand class in order to get that data from the UI module.

3) In the IgnoreAction you could use Job as is a lot nicer than executing a runnable, the IgnoreAction.execute method could be changed to something like this:
 protected void execute(IAction action) throws BazaarException, InvocationTargetException, InterruptedException {
  final IResource[] resources = getSelectedResources();
  Job ignoreJob = new WorkspaceJob("adding to ignored...") {
   @Override
   public IStatus runInWorkspace(IProgressMonitor monitor)
     throws CoreException {
    BazaarTeamProvider provider = (BazaarTeamProvider) RepositoryProvider.getProvider(resources[0].getProject(), EclipseBazaarCore.getProviderID());
    IgnoreCommand op = new IgnoreCommand(provider.getBzrWorkspaceRoot(), Arrays.asList(resources));
    op.run(EclipseBazaarCore.subProgressMonitorFor(monitor));
    return Status.OK_STATUS;
   }
  };
  if (resources.length == 1) {
   ignoreJob.setRule(resources[0]);
   ignoreJob.setPriority(Job.LONG);
   ignoreJob.setUser(true);
   ignoreJob.schedule();
  }
 }

Regards,

 review needs-fixing

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