Merge lp:~javierder/bzr-eclipse/add-ignore into lp:~verterok/bzr-eclipse/trunk

[Javier Derderyan]

[Javier Derderyan (javierder)]

Added support for "Ignore".

I'm working on a dialog to allow to manage .bzrignore in a "gui" way, but it would be nice to get this first part reviewed and merged.

[Martin Albisetti (beuno)]

> 153 + // FIXME: exception handling

Is this handled somewhere else?

[Javier Derderyan (javierder)]

> > 153 + // FIXME: exception handling
>
> Is this handled somewhere else?

That's all around BzrEclipse, probably needs to be checked all around

Also, I've pushed some changes, the "IgnoredDialog", that i hope doesn't get ignored...=P

[Guillermo Gonzalez (verterok)]

> Added support for "Ignore".
>
> I'm working on a dialog to allow to manage .bzrignore in a "gui" way, but it
> would be nice to get this first part reviewed and merged.
Great!
I didn't got any spare time to review this (deadlines and that kind stuff ;), apologize the delay. I'll try to digg into this tonight.
In the meantime, please update the headers to reflect the correct license and copyright.

Cheers,

[Guillermo Gonzalez (verterok)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi!

On Mon, May 4, 2009 at 5:40 PM, Javier Der Derian wro=
te:
> Javier Der Derian has proposed merging lp:~javierder/bzr-eclipse/add-igno=
re into lp:bzr-eclipse.
>
> Requested reviews:
> =C2=A0 =C2=A0Guillermo Gonzalez (verterok)
>
> Added support for "Ignore".
>
> I'm working on a dialog to allow to manage .bzrignore in a "gui" way, but=
 it would be nice to get this first part reviewed and merged.

The UI looks good! keep the great work.

A few comments on some issues I'ld like to get fixed before merging:

1) Please remove the commented code if is not used, e.g:
 - org.vcs.bazaar.eclipse.core.commands.IgnoreCommand:50
 - org.vcs.bazaar.eclipse.ui.dialogs.IgnoredDialog:148

2) I'm not entirely sure that both IgnoreCommand and AddIgnoredCommand
classes should extend ResourceChangeCommand as there is no resource
change involved in the operations. (looks like a branch level change,
or maybe it's a new type?)

3) In org.vcs.bazaar.eclipse.ui.dialogs.IgnoredDialog there is a
private variable that's never used:
private List selection;
(and getSelection() method.)

4) Please externalize the strings to UITexts/Coretext accordingly.

I'm still need to run some tests.
I'll let you work on this minor issues first :)

>
>
> --
> https://code.launchpad.net/~javierder/bzr-eclipse/add-ignore/+merge/6185
> You are requested to review the proposed merge of lp:~javierder/bzr-eclip=
se/add-ignore into lp:bzr-eclipse.
>

Regards,

 review needs-fixing

- --
Guillermo Gonzalez

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Use GnuPG with Firefox : http://getfiregpg.org (Version: 0.7.5)

iEYEARECAAYFAkoBN/UACgkQaYrZfi+VMsib3wCfVgFYSTQUQ8NcBZ4S/glfHXbP
OhQAn0HNxzVM5bISasI0hIzzbX2lrWYd
=KPPT
-----END PGP SIGNATURE-----

review: Needs Fixing

[Javier Derderyan (javierder)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi!
>
> On Mon, May 4, 2009 at 5:40 PM, Javier Der Derian wro=
> te:
> > Javier Der Derian has proposed merging lp:~javierder/bzr-eclipse/add-igno=
> re into lp:bzr-eclipse.
> >
> > Requested reviews:
> > =C2=A0 =C2=A0Guillermo Gonzalez (verterok)
> >
> > Added support for "Ignore".
> >
> > I'm working on a dialog to allow to manage .bzrignore in a "gui" way, but=
> it would be nice to get this first part reviewed and merged.
>
> The UI looks good! keep the great work.
>
> A few comments on some issues I'ld like to get fixed before merging:
>
> 1) Please remove the commented code if is not used, e.g:
> - org.vcs.bazaar.eclipse.core.commands.IgnoreCommand:50
> - org.vcs.bazaar.eclipse.ui.dialogs.IgnoredDialog:148
>
> 2) I'm not entirely sure that both IgnoreCommand and AddIgnoredCommand
> classes should extend ResourceChangeCommand as there is no resource
> change involved in the operations. (looks like a branch level change,
> or maybe it's a new type?)
>
> 3) In org.vcs.bazaar.eclipse.ui.dialogs.IgnoredDialog there is a
> private variable that's never used:
> private List selection;
> (and getSelection() method.)
>
> 4) Please externalize the strings to UITexts/Coretext accordingly.
>
> I'm still need to run some tests.
> I'll let you work on this minor issues first :)
>
> >
> >
> > --
> > https://code.launchpad.net/~javierder/bzr-eclipse/add-ignore/+merge/6185
> > You are requested to review the proposed merge of lp:~javierder/bzr-eclip=
> se/add-ignore into lp:bzr-eclipse.
> >
>
> Regards,
>
> review needs-fixing
>
> - --
> Guillermo Gonzalez
>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Use GnuPG with Firefox : http://getfiregpg.org (Version: 0.7.5)
>
> iEYEARECAAYFAkoBN/UACgkQaYrZfi+VMsib3wCfVgFYSTQUQ8NcBZ4S/glfHXbP
> OhQAn0HNxzVM5bISasI0hIzzbX2lrWYd
> =KPPT
> -----END PGP SIGNATURE-----

Hi, thanks for the review!

About the "ResourceChangeCommand".
I don't know if it applies, but resources are changed everytime any of those commands is executed.
If ignore is executed, .bzrignore is changed.
If addignored is executed, files are added, just like in AddCommand

[Guillermo Gonzalez (verterok)]

>
> Hi, thanks for the review!
>

Hi, apologize the delay.

> About the "ResourceChangeCommand".
> I don't know if it applies, but resources are changed everytime any of those
> commands is executed.
sometimes what change is the the metadata associated with the branch, we could treat the ignored files as such, or as a resource modification, the difference is how the refresh is executed.

> If ignore is executed, .bzrignore is changed.
Actually .bzrignore might be changed, but ins't tracked so, isn't changing, let me digg a bit more about this. maybe we can improve this in a later branch.

> If addignored is executed, files are added, just like in AddCommand

Testing the ingored files dialog, I get NullPointer exceptions when unchecking one ignored file and clicking in ok.

java.lang.NullPointerException
at org.vcs.bazaar.eclipse.core.commands.AddIgnoredCommand.getFiles(AddIgnoredCommand.java:56)
at org.vcs.bazaar.eclipse.core.commands.AddIgnoredCommand.execute(AddIgnoredCommand.java:45)
at org.vcs.bazaar.eclipse.core.commands.MetaModifyCommand.run(MetaModifyCommand.java:29)
at org.vcs.bazaar.eclipse.ui.actions.IgnoredAction$1.execute(IgnoredAction.java:83)
at org.eclipse.ui.actions.WorkspaceModifyOperation$1.run(WorkspaceModifyOperation.java:104)
at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1800)
at org.eclipse.ui.actions.WorkspaceModifyOperation.run(WorkspaceModifyOperation.java:116)
at org.vcs.bazaar.eclipse.ui.actions.BzrAction$1.run(BzrAction.java:120)
at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121)

looks like the problem is in IgnoredDialog.readUnchecked method.
fwiw, there is a list "uncheckedfiles" that's isn't used.
but the problem is that unchecked is populated with one null element.
possibly the error is in project.findMember((String)ignored[i].get(0)) as (String)ignored[i].get(0) is "<project>/<file>" and findMemeber looks for files/folders inside <project>

maybe using IBzrResource classes to show the files might simplify this, as all the code is already there. If you go with this approach, the Map<String, String> returned by the ignored command should be preprocessed before passing it to the dialog, in order to pass a Map<IBzrResource, String> to the dialog.

Regards.

[lp:~javierder/bzr-eclipse/add-ignore]

211. By Guillermo Gonzalez on 2009-05-09

 merge lp:~javierder/bzr-eclipse/send_command, externalize strings in send dialog and fix Bug #373311

[Guillermo Gonzalez (verterok)]

I missed to add the vote :p

review: Needs Fixing

[Guillermo Gonzalez (verterok)]

Now that your send_command fixes are in trunk I got conflicts in org.vcs.bazaar.eclipse.ui/plugin.xml :(

[Guillermo Gonzalez (verterok)]

> > If ignore is executed, .bzrignore is changed.
> Actually .bzrignore might be changed, but ins't tracked so, isn't changing,
> let me digg a bit more about this. maybe we can improve this in a later
> branch.
>
Please ignore this ^
I meant that besides the change in .bzrignore other status are changed, but if you only return the affected resources, but not .bzrignore it might not be refreshed.
I'll double check this.

Thanks,

Regards.

[Guillermo Gonzalez (verterok)]

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>

review: Needs Fixing

[Javier Derderyan (javierder)]

Ok, the 3 issues have been resolved!

I also added some comments, let me know if that's what you need or if you need some kind of special hing.

Thanks!

[Guillermo Gonzalez (verterok)]

Hi,

On Thu, May 28, 2009 at 9:22 PM, Javier Der Derian<email address hidden> wrote:
> Ok, the 3 issues have been resolved!
>

Great thanks!

> I also added some comments, let me know if that's what you need or if you need some kind of special hing.

Regarding the comment about if you need the: run(WorkspaceModifyOperation(...)) to run a commad:
you could also use a plain Runnable or a Job to do it if the command isn't going to modify the workspace, if you are going to modify the workspace there is WorkspaceJob.Hi,

On Thu, May 28, 2009 at 9:22 PM, Javier Der Derian<email address hidden> wrote:
> Ok, the 3 issues have been resolved!
>

Great thanks!

> I also added some comments, let me know if that's what you need or if you need some kind of special hing.

Regarding the comment about if you need the: run(WorkspaceModifyOperation(...)) to run a commad, this call to run, executes a Runnable in the same thread, there are some actions the are already using the Job API, that should be used for long running tasks.
you could also use a plain Runnable or a Job to do it if the command isn't going to modify the workspace, if you are going to modify the workspace there is WorkspaceJob, and UIJob for jobs that modify the UI.

Have in mind that all this are executed asynchronously (the jobs).
Let me explain this a bit more :)
Java is multi threaded, Eclipse use several threads in order to be snappy. The model is basically: View/UI threads and Worker threads.
If you execute blocking code, e.g: a Command, in the UI Thread it 'll block the whole UI...and that's bad.
So you have to split the work into workers, this way you can update the UI in the UI Thread and do the heavy lifting in non-UI-workers. Eclipse provides abstractions so we don't have to work with low level threads directly, those are: Jobs, facilities to execute a Runnable, etc. The Job API is the recommended way of doing this.

If you need to update the UI from a Job or a WorkspaceJob you can't do that directly because it's executed in a different thread, so you can wait until the Job is done and do something to get the value and pass it to the code that's executed in the UI Thread, e.g: using a Listener, (there are plenty of examples in bzr-eclipse source), the other option is to schedule a UIJob or WorkbenchJob (that's executed in the UI Thread, you can find some examples of this approach too), or call Display.asyncExec but is not recommended (some old code still use this approach).

Let me know if I missed a command.

Now this is a bit more clear, please take a look for example, to the IgnoredAction class, in this case you'r running a pontential long running task in the same thread, at this particular task don't take too much time the UI freeze isn't noticeable.
I know there a lot of code that needs update, but one thing at time.

I know that a lot of code to run this Jobs is almost the same with minimal changes, but there some helper classes to simplify the boring tasks of working with them.

Take a look to this article for a detailed description of the Job API and it's benefits: http://www.eclipse.org/articles/Article-Concurrency/jobs-api.html

Th...

review: Needs Fixing

[lp:~javierder/bzr-eclipse/add-ignore]

212. By Javier Derderyan on 2009-06-01

Internal Merge. Conclicts resolved

213. By Javier Derderyan on 2009-06-01

Merged with lp:~javierder/bzr-eclipse/add-ignore

[Javier Derderyan (javierder)]

Hi,

Thanks for your long reply! It will be very helpfull!!

I've made some internal merges (with the send_command branch) to resolve all conflicts.

Now it merges OK in my config.

[Guillermo Gonzalez (verterok)]

Hi,
Apologize the delay I was in offline mode, due to my vacations.

I just found an error in the way the paths(or patters) are passed to the IBazaarClient.ignore method,
IgnoreCommand.java line: 48

instead of
BzrWorkspaceRoot.getBzrFileFor(resources.get(i)).toString()
should be:
BzrWorkspaceRoot.getBzrFileFor(resources.get(i)).getBranchRelativePath().toString()

as the current code returns the representation of the object that might not be the path, currentlt is the encoded path, so spaces are replaced with %20.

Let me know if you don't have the time to fix this (I know you'r working on some qbzr stuff), as I could branch from your branch and fix it.

review: Needs Fixing

=== added file 'org.vcs.bazaar.eclipse.core/src/org/vcs/bazaar/eclipse/core/commands/IgnoreCommand.java'
--- org.vcs.bazaar.eclipse.core/src/org/vcs/bazaar/eclipse/core/commands/IgnoreCommand.java	1970-01-01 00:00:00 +0000
+++ org.vcs.bazaar.eclipse.core/src/org/vcs/bazaar/eclipse/core/commands/IgnoreCommand.java	2009-04-28 18:33:15 +0000
@@ -0,0 +1,46 @@
+/**
+ * LICENSE + COPYRIGHT
+ */
+package org.vcs.bazaar.eclipse.core.commands;
+
+import java.io.File;
+import java.util.List;
+
+import org.eclipse.core.resources.IResource;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.vcs.bazaar.client.core.BazaarClientException;
+import org.vcs.bazaar.eclipse.BzrWorkspaceRoot;
+import org.vcs.bazaar.eclipse.internal.core.BazaarException;
+
+/**
+ * @author Guillermo Gonzalez <[e-mail]>
+ * 
+ */
+public class IgnoreCommand extends ResourceChangeCommand {
+
+	private final List<? extends IResource> resources;
+	private final BzrWorkspaceRoot root;
+
+	public IgnoreCommand(BzrWorkspaceRoot bzrWorkspaceRoot, List<? extends IResource> resources) {
+		this.root = bzrWorkspaceRoot;
+		this.resources = resources;
+	}
+
+	public IResource[] execute(IProgressMonitor monitor) throws BazaarException {
+		try {
+			root.getClient().add(getFiles(resources));
+			return resources.toArray(new IResource[0]);
+		} catch (BazaarClientException e) {
+			throw BazaarException.wrapException(e, root.getBranch().getProject());
+		}
+	}
+
+	private static File[] getFiles(List<? extends IResource> resources) {
+		File[] files = new File[resources.size()];
+		for (int i = 0; i < resources.size(); i++) {
+			files[i] = resources.get(i).getLocation().toFile();
+		}
+		return files;
+	}
+
+}

=== modified file 'org.vcs.bazaar.eclipse.ui/plugin.properties'
--- org.vcs.bazaar.eclipse.ui/plugin.properties	2009-04-15 16:36:58 +0000
+++ org.vcs.bazaar.eclipse.ui/plugin.properties	2009-04-28 18:33:15 +0000
@@ -47,6 +47,8 @@
 ActionNick_tooltip=Show or set the branch nickname.
 ActionRemove_label=R&emove...
 ActionRemove_tooltip=Make a file unversioned.
+ActionIgnore_label=&Ignore...
+ActionIgnore_tooltip=Make a file ignored by Bazaar
 ActionResolve_label=Resolv&e...
 ActionResolve_tooltip=Mark a conflict as resolved.
 ActionRevert_label=&Revert

=== modified file 'org.vcs.bazaar.eclipse.ui/plugin.xml'
--- org.vcs.bazaar.eclipse.ui/plugin.xml	2009-04-15 16:41:07 +0000
+++ org.vcs.bazaar.eclipse.ui/plugin.xml	2009-04-28 18:33:15 +0000
@@ -207,6 +207,14 @@
 				label="%ActionRemove_label" menubarPath="team.main/group4"
 				tooltip="%ActionRemove_tooltip">
 			</action>
+			<action
+				class="org.vcs.bazaar.eclipse.ui.actions.IgnoreAction"
+				definitionId="org.vcs.bazaar.eclipse.ui.actions.ActionIgnore"
+				icon="icons/bzrRemove.png"
+				id="org.vcs.bazaar.eclipse.ui.actions.ActionIgnore"
+				label="%ActionIgnore_label" menubarPath="team.main/group4"
+				tooltip="%ActionIgnore_tooltip">
+			</action>
 		</objectContribution>
 
 		<!-- Project only actions -->

=== added file 'org.vcs.bazaar.eclipse.ui/src/org/vcs/bazaar/eclipse/ui/actions/IgnoreAction.java'
--- org.vcs.bazaar.eclipse.ui/src/org/vcs/bazaar/eclipse/ui/actions/IgnoreAction.java	1970-01-01 00:00:00 +0000
+++ org.vcs.bazaar.eclipse.ui/src/org/vcs/bazaar/eclipse/ui/actions/IgnoreAction.java	2009-04-28 18:33:15 +0000
@@ -0,0 +1,86 @@
+/**
+ * LICENSE + COPYRIGHT
+ */
+package org.vcs.bazaar.eclipse.ui.actions;
+
+import java.lang.reflect.InvocationTargetException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.eclipse.core.resources.IProject;
+import org.eclipse.core.resources.IResource;
+import org.eclipse.core.runtime.CoreException;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.jface.action.IAction;
+import org.eclipse.jface.operation.IRunnableWithProgress;
+import org.eclipse.team.core.RepositoryProvider;
+import org.eclipse.ui.IWorkbenchWindowActionDelegate;
+import org.vcs.bazaar.eclipse.CoreTexts;
+import org.vcs.bazaar.eclipse.EclipseBazaarCore;
+import org.vcs.bazaar.eclipse.core.commands.AddCommand;
+import org.vcs.bazaar.eclipse.core.repository.BazaarTeamProvider;
+import org.vcs.bazaar.eclipse.internal.core.BazaarException;
+import org.vcs.bazaar.eclipse.ui.EclipseBazaarUI;
+
+/**
+ * @author Guillermo Gonzalez
+ * 
+ * TODO: Test me
+ */
+
+public class IgnoreAction extends WorkbenchAction {
+
+	/**
+	 * The action has been activated. The argument of the method represents the
+	 * 'real' action sitting in the workbench UI.
+	 * 
+	 * @see IWorkbenchWindowActionDelegate#run
+	 */
+	protected void execute(IAction action) throws BazaarException, InvocationTargetException, InterruptedException {
+		run(new IRunnableWithProgress() {
+			
+			public void run(IProgressMonitor monitor) throws InvocationTargetException, InterruptedException {
+				try {
+					IProject[] projects = getSelectedProjects();
+					if (projects.length == 1) {
+						IProject project = projects[0];
+						BazaarTeamProvider provider = (BazaarTeamProvider) RepositoryProvider.getProvider(project, EclipseBazaarCore.getProviderID());
+						AddCommand op = new AddCommand(provider.getBzrWorkspaceRoot(), Arrays.asList(projects));
+						op.run(EclipseBazaarCore.subProgressMonitorFor(monitor));
+					} else {
+						// associate the resources with their respective
+						// RepositoryProvider
+						Map<BazaarTeamProvider, List<IResource>> map = getProviderMapping(getSelectedResources());
+						Set<BazaarTeamProvider> keySet = map.keySet();
+						monitor.beginTask("", keySet.size() * 1000); //$NON-NLS-1$
+						monitor.setTaskName(CoreTexts.AddCommand_adding); //$NON-NLS-1$
+						for (BazaarTeamProvider provider : keySet) {
+							List<IResource> resources = map.get(provider);
+							AddCommand op = new AddCommand(provider.getBzrWorkspaceRoot(), resources);
+							op.run(EclipseBazaarCore.subProgressMonitorFor(monitor));
+						}
+					}
+				} catch (CoreException e) {
+					// FIXME: exception handling
+					EclipseBazaarUI.log(e.getMessage(), e);
+					throw new InvocationTargetException(e);
+				} finally {
+					monitor.done();
+				}
+			}
+		}, false /* no cancelable */, PROGRESS_DIALOG);
+	}
+
+	@Override
+	protected boolean isEnabledForManagedResources() {
+		return false;
+	}
+	@Override
+	protected boolean isEnabledForUnmanagedResources() {
+		return true;
+	}
+	
+
+}