Community
Participate
Working Groups
When viewing the property pages for a connection and clicking okay, without changing any of the values, a saving profile job is started. This happens for all 3 property pages: Connector Service, Host, and Subsystems.
Appears to happen on Subsystems for the Service property page. Filters when looking at the filter property page. Shells and the Environment Variables.
This is important for us, too.
Created attachment 117141 [details] patch to check whether there are changes before committing This patch contains changes for the service pages, the connection page, the environment variables and the filter property page to only do a commit if there is an actual change. Could you try this out to make sure this handles all the cases you've seen this with?
The subsystem property page on the files subsystem is still causing a save. Also on the Host property page we hide the default encoding, so this line is now giving a NPE: if (!isRemoteEncoding && !encoding.equals(currentEncoding)) {
(In reply to comment #4) > The subsystem property page on the files subsystem is still causing a save. > > Also on the Host property page we hide the default encoding, so this line is > now giving a NPE: > > if (!isRemoteEncoding && !encoding.equals(currentEncoding)) { > I can add a check for the null encoding on the host property page. For the subsystem property page, the user id is always being seen as changed because the default displayed one is based on Subsystem.getLocalUserId() while it's being compared with IConnectorService.getUserId() and these aren't the same. I need a better understanding as to why we have both of these and how they should be used in this scenario before committing to a solution for this. Could you open a separate defect for this?
(In reply to comment #5) > I can add a check for the null encoding on the host property page. Let's add the check then we can resolve this bug. > For the subsystem property page, the user id is always being seen as changed > because the default displayed one is based on Subsystem.getLocalUserId() while > it's being compared with IConnectorService.getUserId() and these aren't the > same. I need a better understanding as to why we have both of these and how > they should be used in this scenario before committing to a solution for this. > Could you open a separate defect for this? Bug 254548 has been opened for this.
(In reply to comment #6) > (In reply to comment #5) > > I can add a check for the null encoding on the host property page. > > Let's add the check then we can resolve this bug. > I've committed the changes and the check for the null encoding. I will address the subsystem property page issue in bug 254548.
hasConnectionChanged() looks suspicious because IHost.getDescription() can be null (and actually is null by default). I'd assume that getDefaultUserId() and getHostName() can also be null for connection types that don't have a concept of user/password, or connect by means other than TCP/IP (such as serial). I'd recommend introducing a compareStrings(s1,s2) method which is robust against null Strings.
Also, ServiceElement# protected boolean _childChanged is new API. Please add an @since tag, and enable API Tooling in order to see such errors in your workspace.
Also, in RemoteCmdSubSystem#areVariablesTheSame() you'll likely need to sort the keys before you compare them (not 100%sure, but Arrays.sort() is simple to call, don't forget to clone the input argument before sorting). PropertySets are HashMaps, and the order produced by the keySet() is undefined (even on a clone, I think).
(In reply to comment #8) > hasConnectionChanged() looks suspicious because IHost.getDescription() can be > null (and actually is null by default). I'd assume that getDefaultUserId() and > getHostName() can also be null for connection types that don't have a concept > of user/password, or connect by means other than TCP/IP (such as serial). > > I'd recommend introducing a compareStrings(s1,s2) method which is robust > against null Strings. > For the compare strings are you thinking something like this? private boolean hasConnectionChanged(IHost conn){ if (!compareStrings(conn.getName(), form.getConnectionName()) || !compareStrings(conn.getHostName(), form.getHostName()) || !compareStrings(conn.getDescription(), form.getConnectionDescription()) || !compareStrings(conn.getDefaultUserId(), form.getDefaultUserId())){ return true; } return false; } private boolean compareStrings(String str1, String str2){ if (str1 == null) return (str2 == null); else return (str2 == null) ? false: str1.equals(str2); } In the case of the description it's stored as "" rather than null but I guess null could come up.
(In reply to comment #9) > Also, ServiceElement# protected boolean _childChanged is new API. Please add > an @since tag, and enable API Tooling in order to see such errors in your > workspace. > I've added the @since tag.
DummyHost has description==null, and class Host initialized description==null. Persistence providers are pluggable, so a "null" description seems likely IMHO. I think that your compareStrings() method should likely treat "" and null as equal, since the Form will likely not accept null in any of its SWT fields but replace null by "" when initializing the fields.
(In reply to comment #10) > Also, in RemoteCmdSubSystem#areVariablesTheSame() you'll likely need to sort > the keys before you compare them (not 100%sure, but Arrays.sort() is simple to > call, don't forget to clone the input argument before sorting). PropertySets > are HashMaps, and the order produced by the keySet() is undefined (even on a > clone, I think). > Hmm... I didn't want to sort anything because one change a user can make to the environment variables page is moving variables up and down in the list - so if the order changes, a save is needed. From my tests the current code seems to work okay but I guess I should check again to make sure this is predictable and consistent.
(In reply to comment #13) > DummyHost has description==null, and class Host initialized description==null. > Persistence providers are pluggable, so a "null" description seems likely IMHO. > > I think that your compareStrings() method should likely treat "" and null as > equal, since the Form will likely not accept null in any of its SWT fields but > replace null by "" when initializing the fields. > I've update cvs with the new compareStrings method.
(In reply to comment #14) > (In reply to comment #10) > > Also, in RemoteCmdSubSystem#areVariablesTheSame() you'll likely need to sort > > the keys before you compare them (not 100%sure, but Arrays.sort() is simple to > > call, don't forget to clone the input argument before sorting). PropertySets > > are HashMaps, and the order produced by the keySet() is undefined (even on a > > clone, I think). > > > > Hmm... I didn't want to sort anything because one change a user can make to the > environment variables page is moving variables up and down in the list - so if > the order changes, a save is needed. From my tests the current code seems to > work okay but I guess I should check again to make sure this is predictable and > consistent. > I looked at how the environment variables get loaded into the form and I see that we call this method from RemoteCmdSubSystem: public IRemoteSystemEnvVar[] getEnvironmentVariableList() { IPropertySet environmentVariables = getEnvironmentVariables(); String[] names = environmentVariables.getPropertyKeys(); IRemoteSystemEnvVar[] result = new IRemoteSystemEnvVar[names.length]; for (int i = 0; i < names.length; i++) { String name = names[i]; String value = environmentVariables.getPropertyValue(name); IRemoteSystemEnvVar v = new RemoteSystemEnvVar(); v.setName(name); v.setValue(value); result[i] = v; } return result; } Because the order is determined by the order of the names returned in environmentVariables.getPropertyKeys() and we use the same method to get the keys in areEnvironmentVariables() I think the current method is okay.
Since we have bug 254548 for the subsystem property page and keys should not be sorted when checking the environment variables I'm going to mark this as fixed.
Fix involved an API addition: protected boolean ServiceElement#_childChanged;
