Comment 1 Pascal Rapicault 2011-06-09 20:08:59 EDT

make sure that the behavior of null in the repos parameter is defined.

Comment 2 Hugo Corbucci 2011-06-10 08:10:22 EDT

Since I was the one having issues with this lack of Javadoc, I thought I could provide a start to this work.

/**
 * Create an {@link UpdateOperation} that will update the elements specified.
 * @param toUpdate The elements to update. This can not be null. Note that you can pass the results of {@link OperationFactory.listInstalledElements} to this method if you wish to update all elements installed in the running instance of eclipse (those just need to be casted to {@link IVersionedId} for now).
 * @param repos the repositories to update the elements from. If null is passed, it will use all previously registered repositories.
 * @param monitor the progress monitor
 * @return an instance of {@link UpdateOperation}
 */

I noticed also that all create methods in this class have the same behavior regarding the repositories so adding the comment about null to all of them might be a good idea.

Comment 3 Hugo Corbucci 2011-06-10 08:25:35 EDT

(In reply to comment #2)
> Since I was the one having issues with this lack of Javadoc, I thought I could
> provide a start to this work.
> 
> /**
>  * Create an {@link UpdateOperation} that will update the elements specified.
>  * @param toUpdate The elements to update. This can not be null. Note that you
> can pass the results of {@link OperationFactory.listInstalledElements} to this
> method if you wish to update all elements installed in the running instance of
> eclipse (those just need to be casted to {@link IVersionedId} for now).
>  * @param repos the repositories to update the elements from. If null is
> passed, it will use all previously registered repositories.
>  * @param monitor the progress monitor
>  * @return an instance of {@link UpdateOperation}
>  */
> 
> I noticed also that all create methods in this class have the same behavior
> regarding the repositories so adding the comment about null to all of them
> might be a good idea.

On a closer look, the toUpdate collection can be null and will simply create an UpdateOperation with null as elements to update, which will, by itself, suggest all installed units.
So I would suggest the following Javadoc.

/**
  * Create an {@link UpdateOperation} that will update the elements specified.
  * @param toUpdate The elements to update. This can be null in which case all installed IUs will be queried for updates.
  * @param repos the repositories to update the elements from. If null is passed, it will use all previously registered repositories.
  * @param monitor the progress monitor
  * @return an instance of {@link UpdateOperation}
  */

Comment 4 Hugo Corbucci 2011-06-10 08:29:30 EDT

(In reply to comment #3)
> (In reply to comment #2)
> > Since I was the one having issues with this lack of Javadoc, I thought I could
> > provide a start to this work.
> > 
> > /**
> >  * Create an {@link UpdateOperation} that will update the elements specified.
> >  * @param toUpdate The elements to update. This can not be null. Note that you
> > can pass the results of {@link OperationFactory.listInstalledElements} to this
> > method if you wish to update all elements installed in the running instance of
> > eclipse (those just need to be casted to {@link IVersionedId} for now).
> >  * @param repos the repositories to update the elements from. If null is
> > passed, it will use all previously registered repositories.
> >  * @param monitor the progress monitor
> >  * @return an instance of {@link UpdateOperation}
> >  */
> > 
> > I noticed also that all create methods in this class have the same behavior
> > regarding the repositories so adding the comment about null to all of them
> > might be a good idea.
> 
> On a closer look, the toUpdate collection can be null and will simply create an
> UpdateOperation with null as elements to update, which will, by itself, suggest
> all installed units.
> So I would suggest the following Javadoc.
> 
> /**
>   * Create an {@link UpdateOperation} that will update the elements specified.
>   * @param toUpdate The elements to update. This can be null in which case all
> installed IUs will be queried for updates.
>   * @param repos the repositories to update the elements from. If null is
> passed, it will use all previously registered repositories.
>   * @param monitor the progress monitor
>   * @return an instance of {@link UpdateOperation}
>   */

Yet another correction but I don't know the impact of that one. Passing null to the UpdateOperation will make it query only through user visible roots. Which, by the way, is done by a method in UpdateOperation called getInstalledIUs with an outdated comment (shows params and explains behavior that do not match the code).

Comment 5 Pascal Rapicault 2011-06-10 21:37:13 EDT

Thx for the contrib! Fixed in 3.7.1 and HEAD.