Bug 264389

Comment 1 Ian Bull 2009-02-12 17:09:09 EST

Looking at this code, it appears that you can create a RemoteUpdateSiteAction with either a source, or UpdateSite.  However, createActions assumes that you always have an UpdateSite.  I will remove the ability to create this with a "source", as you really should be using a LocalUpdateSiteAction for this.

Comment 2 Ian Bull 2009-02-12 19:27:17 EST

Andrew,

I have approached this problem as follows: 

In the SiteXMLAction look to see if a categoryQualifier was passed in (probably from the command line, but this can also be done via API).  If it has, we use it, otherwise:
return URIUtil.toUnencodedString(updateSite.getLocation()) + "."; //$NON-NLS-1$

Updatesite should not be null at this point (either it is a remote update site, or one has been created from a local site.xml file).

Then when I create the category ID I do the following:
String categoryId = categoryQualifier + category.getName();

Does this make sense?  Do we want to see if the names "look unique" first?  If this looks ok, I will post the patch.

Comment 3 Ian Bull 2009-02-17 15:36:49 EST

Created attachment 125938 [details]
Qualifier patch

This patch uses the update site as the qualifier for categories.

Andrew, can you take a look at this and let me know if you think it will work?  I still have to add a command line argument for the publisher application, but if this patch looks good, it can be released.

Comment 4 Ian Bull 2009-02-17 15:37:02 EST

Created attachment 125939 [details]
mylyn/context/zip

Comment 5 Ian Bull 2009-02-18 13:47:01 EST

trying to write test cases, but I'm blocked by the fact that I *must* publish artifacts, otherwise I get an NPE.

Comment 6 Ian Bull 2009-02-18 16:42:36 EST

Created attachment 126082 [details]
Upated patch

This now includes the command line argument in the UpdateSiteApplication.  The argument is -categoryQualifier.

the patch also includes another test case (testing the application)

Comment 7 Ian Bull 2009-02-18 16:42:46 EST

Created attachment 126083 [details]
mylyn/context/zip

Comment 8 Jeff McAffer 2009-02-18 21:56:03 EST

comments on the latest patch
- "source" field is no longer used in RemoteUpdateSiteAction. can be removed


SiteXMLAction
- Style thing 
	public void setCategoryQualifier(String categoryQualifier) {
		this.categoryQualifier = categoryQualifier;
should be 
	public void setCategoryQualifier(String value) {
		categoryQualifier = value;

- createQualifier() seems to imply that qualifiers should end in ".". why?  Seems like if people want to 

- naming consistency.  some places it is "qualifier" others it is "categoryQualifier"

- why use a setter for setQualifierName? Most other actions seem to take everything in the constructor AFAIK.

LocalUpdateSiteAction/SiteXMLAction
- RemoteUpdateSiteAction was modified to remove the String-based constructor-- you have to pass in an update site.  I could go either way on that but it does leave LocalUpdateSiteAction and SiteXMLAction inconsistent. They both have String/URI based constructors but now Remote... does not.  Should be consistent.

- How do you supply a qualifier if using LocalUpdateSiteAction(UpdateSite)?  If you use LocalUpdateSiteAction(String, String) you can provide one.  Creating an UpdateSite does not allow for qualifiers (and should not) so there seems to be a missing arg.

Comment 9 Ian Bull 2009-02-19 13:55:44 EST

Created attachment 126197 [details]
Updated patch

This patch takes into consideration Jeff's suggestions.  It also changes the name of UpdatesitePublisherApplication to UpdateSitePublisherApplication (notice the S in UpdateSite)

Comment 10 Ian Bull 2009-02-19 13:59:29 EST

Created attachment 126200 [details]
Updated patch

opps.. sorry, the last one included a change to AbstractPublisherApplication that should not be included in this patch.

Note:  this patch depends on Bug #204060.  Also, there is a small change to PDE build in here.

Comment 11 Jeff McAffer 2009-02-24 23:03:46 EST

Created attachment 126667 [details]
updated SiteXMLAction

Some comments and an updated patch.
- the new patch has some changes in SiteXMLAction around how categoryIds are created

Comments:
- Is there a bug to capture the disconnect in the API for local and remote sites?  With this patch the ability to create a remote site action with just a location is removed but you can do it with a local site action.  We discussed it but I don't remember the logic but do remember that we agreed there would be a bug to capture that for later fix up.

- There is another inconsistency in that you can create a local site action with a qualifier but not a remote site action.  Why is that?

Comment 12 Ian Bull 2009-02-24 23:23:56 EST

Thanks for the updated patch.  Yep, your approach for creating categories is much better.

I haven't opened the bug to capture the disconnect in the API, because the disconnect hasn't happened yet (it won't happen until this patch is in).  Although, so I don't forget let do that now.

As for the reason for not adding the qualifier to remote update site, it stems from the description of the bug report:  For update sites on the web, we should transform the URL, and for local sites, we should allow it to be specified. Although I see no reason why we could not also specify it for remote sites.  

Comment 13 Ian Bull 2009-02-24 23:51:12 EST

Created attachment 126670 [details]
Updated Patch

This updates the previous patch so you can pass a qualifier to a remote update site now.

Comment 14 Ian Bull 2009-02-24 23:51:17 EST

Created attachment 126671 [details]
mylyn/context/zip

Comment 15 Ian Bull 2009-02-24 23:54:13 EST

I have opened Bug 266070 to track the API issue.

Andrew, do you think you could review and hopefully release this patch?  (There is a small change to PDE build in here).

Comment 16 Andrew Niefer 2009-03-04 08:48:00 EST

Added minor change to use the name as a label is the label is null.