Comment 1 Mike Wilson 2007-02-23 09:32:27 EST

Typo in constant name: 
ATTRIBUTE_NON_MODIFABLE shold be ATTRIBUTE_NON_MODIFIABLE


(I might have picked ATTRIBUTE_NOT_MODIFIABLE instead, but that's just personal preference.)

Comment 2 Frederic Fusier 2007-02-23 10:42:38 EST

I agree with the name change for the constant. I also fixed some other typos and rephrased some sentences in the javadoc comment:

/**
 * Return whether attributes of a given classpath container classpath entry
 * can be modified or not.
 * <p>
 * The returned status is {@link IStatus#isOK()} if the attributes
 * are supported, modifiable and optionally have a valid value.<br>
 * Otherwise (i.e. if {@link IStatus#ERROR} is returned), then the
 * {@link IStatus#getCode()} can have one of the following value:
 * <ul>
 * <li>{@link #ATTRIBUTE_NOT_SUPPORTED}: at least one attribute
 * 	is not supported</li>
 * <li>{@link #ATTRIBUTE_NOT_MODIFIABLE}: all attributes are supported
 * 	but at least one is not modifiable</li>
 * <li>{@link #INVALID_VALUE}: all attributes are supported and modifiable
 * 	but at least one has an invalid value</li>
 * </ul>
 * The status message can contain more information.
 * </p><p>
 * The attributes to be tested are the attributes of the class path entry
 * that are not set to their default value and all extra classpath attributes 
 * returned by the entries {@link IClasspathEntry#getExtraAttributes()}.
 * </p><p>
 * Default values for the built in attributes are:
 * <ul><li>{@link IClasspathEntry#getSourceAttachmentPath()
 *               source attachment path}: <code>null</code></li>
 * <li>{@link IClasspathEntry#getSourceAttachmentRootPath()
 *                source attachment root path}: <code>null</code></li>
 * <li>{@link IClasspathEntry#getAccessRules() access rules}:
 *                 <code>empty array of IAccessRule</code></li>
 * <li>{@link IClasspathEntry#combineAccessRules() 
 *                combineAccessRules}: <code>true</code></li>
 * </ul>
 * If <code>testValue</code> is set, the value is also tested for validity.
 * If not set, all values are accepted.
 * </p>
 *    
 * @param containerPath the path of the container which requires to be
 * updated
 * @param project the project for which the container is to be updated
 * @param entry an entry of the classpath container with all attributes set
 * that need to be tested. Set means a value that is not the attributes
 * default value.
 * @param testValue if set the value is also tested
 * @return returns the resulting status
 * 
 * @since 3.3
 */
public IStatus canUpdateChildAttributes(IPath containerPath,
	IJavaProject project, IClasspathEntry entry, boolean testValue) {

	if (canUpdateClasspathContainer(containerPath, project)) {
		return Status.OK_STATUS;
	}
	return new JavaModelStatus(ATTRIBUTE_NOT_MODIFIABLE);
}

Comment 3 Frederic Fusier 2007-02-23 10:46:53 EST

Created attachment 59665 [details]
Here's the corresponding patch...

Comment 4 Philipe Mulet 2007-02-23 12:31:40 EST

+1

Comment 5 Philipe Mulet 2007-02-23 12:39:42 EST

Typo "built in" --> "built-in"

Comment 6 Philipe Mulet 2007-02-23 12:43:58 EST

Two points:
1. default value is already a special value, why prevent user from using it ? Let say, I want my initializer to refuse resetting the source attachment under me... if null value is passed, then I should be able to veto it.
Suggest instead to consider that any value is considered if distinct from existing one.

2. should rename 'test' into 'validate/check'. e.g. 'testValue' --> 'requestValidation' ?

Comment 7 Martin Aeschlimann 2007-02-26 05:00:40 EST

Yes, you are correct. The API doesn't allow you to communicate that you want to allow modification of one of the built-in attributes, but not to null (or empty array).
I thought about some solutions to that, but decided to accept that weekness, as I couldn't come up with a nice solution that doesn't involve too much new API.

Here's my evaluation:
It would probably mean to introduce some constants, like 'defaultPath', 'defaultPattern (IPath[])' 'defaultRule'.
(suggestion b.)
- First problem is to find good values. To avoid confusion, it should not only have a own identity but also not be 'equals' to an existing Path
- When the UI creates a IClasspathEntry to test, it must create the entry with the defaults at all places possible:
JavaCore.newLibraryEntry(defaultPath, defaultPath, defaultPath, defaultRule, myAttributes, false)
That means it must use the factory method with the most arguments. If it forgets one, this can be interpreted as a testing request of that attribute. This might happen if we would decide to add a built-in attribute API in a later release.

For that reason it would make more sense to define 'nullValues'
'nullPath', 'nullPattern (IPath[])' 'nullRule'
(suggestion c.)
But that might be confusing and users will use them at other places.

Then there are plenty of possibilities if you accept adding more APIs: For example offering a bitMask that specifies which built-in attribites are to be tested: This would add a API constant per built-in attribute. but doesn't play together with the existing API's.
(suggestion d.)

So maybe we should not support value testing after all, just offer 'is modifiable, is supported': That would simplify the API a bit, and avoids to find a solution of the discussed problem. For UI porposes that would still be good enough.

canUpdateChildAttributes(IPath containerPath, IJavaProject project, IClasspathEntry entry)
as before: all non-default built-in attributes are tested.

Comment 8 Philipe Mulet 2007-02-28 06:20:54 EST

Why not simply considering built-in attributes set to values different than current ones ? Thus you allow them to be resetted (and passing along the same value is treated as a NO-OP, which it is anyway).

Comment 9 Martin Aeschlimann 2007-02-28 06:55:03 EST

- when we get the entries, we get them as an array: JavaCore.getClasspathContainer(..).getClasspathEntries()
So if you want to test against a existing entry, you would also have to give the index
or maybe give the whole IClasspathContainer.
IStatus canUpdateClasspathContainer(IPath containerPath, IJavaProject project, IClasspathContainer container, boolean testValues) {

API wise this would be consistent, but would mean quite some work to do in the ClasspathInitializer to compare old and new to find the difference. The UI would call that method for each container entry' attribute. So over 100 times for the JRE container.

Note that we need a way to just find ont if an attribute (built-in or extra) is supported (without knowing the valid values). That's why I added the 'textValues' again.

So I still think let's keep it minimal for now and skip the value testing, but just is_supported/ is_modifiable

Comment 10 Frederic Fusier 2007-03-01 12:56:28 EST

I've talked with philippe about this and he proposed an interesting middle term solution: pass 2 classpath entries (old and new values). The method would answer one global IStatus for *all* attributes which have different values between the two given entries.

This would avoid to make any assumption on attribute values for the verification and allow *all* values to be verified (even the default ones). This would also be the clearest way for user to understand how to specify this value (avoiding any additional constants).

It would finally allow to keep the support for value testing as it become 100% clear how to use it... I'll soon put the new proposed method and its javadoc comment if you do not veto it until then...

Comment 11 Frederic Fusier 2007-03-01 13:35:18 EST

Here's the new proposal:
/**
 * Return status on modified attributes between two given classpath container
 * classpath entries.
 * <p>
 * The attributes to be tested are those which have a different
 * value between the two given classpath entries.
 * </p><p>
 * The returned status is {@link IStatus#OK} if the attributes
 * are supported, modifiable and optionally have a valid value.<br>
 * Otherwise (i.e. if {@link IStatus#ERROR} is returned), then the
 * {@link IStatus#getCode()} can have one of the following value:
 * <ul>
 * <li>{@link #ATTRIBUTE_NOT_SUPPORTED}:
 * 	at least one attribute is not supported</li>
 * <li>{@link #ATTRIBUTE_NOT_MODIFIABLE}:
 * 	all attributes are supported but at least one is not modifiable</li>
 * <li>{@link #INVALID_VALUE}:
 * 	all attributes are supported and modifiable but at least one
 * 	has an invalid value</li>
 * </ul>
 * The status message can contain more information.
 * </p><p>
 * If the subclass does not override this method, then default behavior is
 * to return {@link IStatus#OK} if the classpath container can be updated
 * (see {@link #canUpdateClasspathContainer(IPath, IJavaProject)}).
 * Otherwise it returns {@link IStatus#ERROR} with
 * {@link #ATTRIBUTE_NOT_MODIFIABLE} code.
 * </p>
 *    
 * @param containerPath the path of the container which requires to be
 * 	updated
 * @param project the project for which the container is to be updated
 * @param currentEntry a classpath container entry with current values
 * @param testedEntry a classpath container entry with attributes to be tested.
 * 	All attributes having a different value from the current entry should
 * 	be tested by this method.
 * @param validate check if the values to be tested are valide or not.
 * @return returns the resulting status for all tested attributes
 * 
 * @since 3.3
 */
public IStatus canUpdateChildAttributes(IPath containerPath,
	IJavaProject project,
	IClasspathEntry currentEntry,
	IClasspathEntry testedEntry,
	boolean validate) {

	if (canUpdateClasspathContainer(containerPath, project)) {
		return Status.OK_STATUS;
	}
	return new JavaModelStatus(ATTRIBUTE_NOT_MODIFIABLE);
}

Comment 12 Frederic Fusier 2007-03-01 13:49:31 EST

I think it could be also interesting to validate that the two given classpath entries have the same path and then return a specific error status in case not...

Comment 13 Martin Aeschlimann 2007-03-02 06:27:09 EST

I see a problem with the 'current entry' parameter: It sounds as it be one of the classpath entries that the container returned. Or can it any kind of entry, for example an entry that contains all default values? Then we should rather call it 'default entry'.

If you really want to test on the current entries, we become much more powerful (specific validation for each single classpath entry is now possible), but also very expensive, as this requires the UI to make the tests for each entry and each attribute (see comment 9).

I would like to stay simple and have the same 'is_supported/is_modifiable' rule on all children. Therefor I still think we should go for something like:

/**
 * Returns whether a given classpath container supports certain classpath attributes on
 * its class path entries.
 * <p>
 * The returned status is {@link IStatus#OK}, {@link IStatus#WARNING} or {@link IStatus#INFO}
 * if modifying the specified attributes are supported and are modifiable<br>
 * If the returned status is {@link IStatus#ERROR}, then the
 * {@link IStatus#getCode()} can have one of the following values:
 * <ul>
 * <li>{@link #ATTRIBUTE_NOT_SUPPORTED}: at least one attribute
 *      is not supported</li>
 * <li>{@link #ATTRIBUTE_NOT_MODIFIABLE}: all attributes are supported
 *      but at least one is not modifiable</li>
 * <li>{@link #INVALID_VALUE}: all attributes are supported and modifiable
 *      but at least one has an invalid value</li>
 * </ul>
 * The status message can contain more information.
 * </p><p>
 * The attributes to be tested are defined by the class path entry passed as parameter:
 * All attributes of the class path entry
 * that are not set to their default value and all extra classpath attributes 
 * returned by the entries {@link IClasspathEntry#getExtraAttributes()} are tested.
 * </p><p>
 * Default values for the built in attributes are:
 * <ul><li>{@link IClasspathEntry#getSourceAttachmentPath()
 *               source attachment path}: <code>null</code></li>
 * <li>{@link IClasspathEntry#getSourceAttachmentRootPath()
 *                source attachment root path}: <code>null</code></li>
 * <li>{@link IClasspathEntry#getAccessRules() access rules}:
 *                <code>empty array of IAccessRule</code></li>
 * <li>{@link IClasspathEntry#getExclusionPatterns() exclusion pattern}:
 *                <code>empty array of IPath</code></li>
 * <li>{@link IClasspathEntry#getInclusionPatterns() inclusion pattern}:
 *                <code>empty array of IPath</code></li>
 * <li>{@link IClasspathEntry#combineAccessRules() combineAccessRules}
 *                <code>false</code></li>
 * <li>{@link IClasspathEntry#isExported() is exported}
 *                <code>false</code></li></ul>
 * </p>
 *    
 * @param containerPath the path of the container which requires to be
 * updated
 * @param project the project for which the container is to be updated
 * @param entry an entry of the classpath container with all attributes set
 * that need to be tested. Set means a value that is not the attributes
 * default value as spec'ed above
 * @return returns the resulting status
 * 
 * @since 3.3
 */
public IStatus canUpdateChildAttributes(IPath containerPath,
        IJavaProject project, IClasspathEntry entry) {

        if (canUpdateClasspathContainer(containerPath, project)) {
                return Status.OK_STATUS;
        }
        return new JavaModelStatus(ATTRIBUTE_NOT_MODIFIABLE);
}

Comment 14 Frederic Fusier 2007-03-06 07:45:26 EST

Created attachment 60315 [details]
New proposed patch

Talking with Martin about this API, we agreed that only have one method will be confusing whatever the solution we use to specify which attribute needs to be updated...

Jerome observed that only source attachment, access rules and extra attributes may be modified from Java Build Path tab in the properties dialog.

So, the new proposal for this API is to provide methods validating updates only on these specific attributes...

Comment 15 Jerome Lanneluc 2007-03-13 07:50:53 EDT

Need to clarify what a valid value is for an access rule in validateAccessRules(...). Also shouldn't we have a multi-status for each access rule ? Or should the method be validateAccessRule(..., IAccessRule, ...) ?

Comment 16 Jerome Lanneluc 2007-03-13 07:52:35 EDT

Also the constants need Javadoc comments.

Comment 17 Frederic Fusier 2007-03-14 06:14:20 EDT

(In reply to comment #15)
> Need to clarify what a valid value is for an access rule in
> validateAccessRules(...).
>
Check value option has been removed from API.

> Also shouldn't we have a multi-status for each access rule ?
>
Clients may return a multi-status, it will depend on their specific implementation. This would be possible with this API as the return value is an IStatus which can be multiple or not.

> Or should the method be validateAccessRule(..., IAccessRule, ...) ?
> 
Martin specified that he does not want several attributes for the validation but each verification should verify the entire attribute => for access rule, this is an array of IAccessRule...

Comment 18 Frederic Fusier 2007-03-14 06:15:51 EDT

Created attachment 60773 [details]
3 API methods with no check for values

(In reply to comment #16)
> Also the constants need Javadoc comments.
> 
Done in this new patch

Comment 19 Martin Aeschlimann 2007-03-14 06:26:44 EDT

I we don't do value checking (which I think is best) there's no need to pass in any value and the API's would look like that:

IStatus canUpdateSourceAttachmentOnChildren(IPath containerPath, IJavaProject project) 
IStatus canUpdateAccessRulesOnChildren(IPath containerPath, IJavaProject project)
IStatus canUpdateExtraAttributeOnChildren(IPath containerPath, IJavaProject project, String extraAttributeKey) 

Comment 20 Frederic Fusier 2007-03-14 10:44:51 EDT

(In reply to comment #19)
> I we don't do value checking (which I think is best) there's no need to pass
> in any value and the API's would look like that:
> 
> IStatus canUpdateSourceAttachmentOnChildren(IPath containerPath, IJavaProject
> project) 
> IStatus canUpdateAccessRulesOnChildren(IPath containerPath, IJavaProject
> project)
> IStatus canUpdateExtraAttributeOnChildren(IPath containerPath, IJavaProject
> project, String extraAttributeKey) 
> 
OK to remove the attribute value, but I do not think canUpdate* is better than validate*. IMO, 'canUpdate' suggests that method returns a boolean. Looking at our code, when returning a IStatus, this is the 'validate' prefix which is used in our API methods...

Comment 21 Martin Aeschlimann 2007-03-14 10:53:51 EDT

But nothing is 'validated' here.
Instead of an IStatus we could directly return the int constant.
'ATTRIBUTE_NOT_SUPPORTED'
'ATTRIBUTE_READ_ONLY'
'ATTRIBUTE_MODIFIABLE'
 

Comment 22 Frederic Fusier 2007-03-14 11:33:56 EDT

(In reply to comment #21)
> But nothing is 'validated' here.
> Instead of an IStatus we could directly return the int constant.
> 'ATTRIBUTE_NOT_SUPPORTED'
> 'ATTRIBUTE_READ_ONLY'
> 'ATTRIBUTE_MODIFIABLE'
> 
> 
For extra attribute there's a validation: is it supported or not?

Reduce the return to a boolean or to an int would make client unable to add more information although you suggested it was posisble in your inital proposal:
"... The status message can contain more information".

I think that possibility to add more information is really interesting as it gives an opportunity for the sub-class implementor to explain why client cannot modify an attribute or, for the extra attribute, why it is not supported...

Comment 23 Martin Aeschlimann 2007-03-14 11:47:09 EDT

In the UI we will use this API to filter elements (if not supported) or enable/disable the 'Edit...' button.
So we actually won't be able to show any explanation.

When I initially added the IStatus, this was for giving better messages on value testing.

Note that SUPPORTED == READ_ONLY || MODIFIABLE

So all 3 states are required also for the extra attributes. 

Comment 24 Jerome Lanneluc 2007-03-14 11:55:40 EDT

You could take advantage of the status message to show it on the hover of a disabled button, so that a user knows why it is disabled (from experience it is very frustrating to not be able to press a button when you saw it enabled (in another context) 10 minutes ago).

Comment 25 Martin Aeschlimann 2007-03-14 12:07:15 EDT

Interesting idea, but it's not common that buttons have tooltips. No plans for adding this.

Comment 26 Jerome Lanneluc 2007-03-15 06:54:36 EDT

Martin, in what scenario can a classpath container return a classpath entry with a 'non supported' attribute ? Is this going to ever happen ?

Comment 27 Martin Aeschlimann 2007-03-15 07:33:44 EDT

The JRE container for example doesn't want that its children can be configured with the 'native lib' attribute (it doesn't make sense for a JRE).
We want to filter out an many attributes as possible, so that the UI looks cleaned up.

Comment 28 Jerome Lanneluc 2007-03-15 07:54:51 EDT

To get some context, talking with Martin it appears that UI has a list of attributes (that don't necessarily come from the container initializer) and they want to know if each attribute is supported by the initializer. 

Comment 29 Jerome Lanneluc 2007-03-15 08:05:20 EDT

Suggest the 3 APIs:
- boolean canUpdateSourceAttachment(IPath containerPath, IJavaProject
project) 
- boolean canUpdateAccessRules(IPath containerPath, IJavaProject
project)
- IStatus getAttributeStatus(IPath containerPath, IJavaProject
project, String extraAttributeKey)
where the possible status codes would be:
  ATTRIBUTE_NOT_SUPPORTED
  ATTRIBUTE_READ_ONLY
  ATTRIBUTE_MODIFIABLE

Comment 30 Frederic Fusier 2007-03-15 12:53:46 EDT

Created attachment 60979 [details]
New proposed patch

Hopefully the last one?

Comment 31 Frederic Fusier 2007-03-16 05:49:17 EDT

Last patch released for 3.3 M6 in HEAD stream.

Comment 32 Jerome Lanneluc 2007-03-16 07:06:28 EDT

Entered bug 177732 for the UI to take advantage of the status message.

Comment 33 Olivier Thomann 2007-03-20 09:23:21 EDT

Verified for 3.3 M6 using build I20070320-0010