Skip to main content

Please review fix of 6823064 - Unnecessary reload of current configuration

7 replies [Last post]
fda
Offline
Joined: 2005-05-27

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
fda
Offline
Joined: 2005-05-27

The original fix failed and rolled back. See new thread for refix:

http://forums.java.net/jive/thread.jspa?threadID=65172&tstart=0

bkurotsu
Offline
Joined: 2004-12-13

Wasn't the extra copy also for synchronizing template or external values? Did Sergey mention this?

fda
Offline
Joined: 2005-05-27

Sergey says it was a kind of quick fix, he was short in time, but the fix was required urgently.

bkurotsu
Offline
Joined: 2004-12-13

Well, I'm always concerned about changes like this which affect the behavior of the value... It causes behavior regressions. This is setDefaultValue(), which isn't called throughout the lifetime, which makes it a bit less dangerous.

The justification in the described in the bug seems ok. And the only concern I would have is if the programmer calls setDefaultValue() at some strange time. If they did this for example:
setValue(null);
setDefaultValue("Empty");

In this case, maybe they wanted a blank value, but then to have it reset to "Empty" if the user does Clear. Not sure why they would want this, but the new code would change this behavior.

In the end though, I'm ok with integrating the change - could you give the scenario above a little thought though and see if you come up with any other ill effects?

fda
Offline
Joined: 2005-05-27

Yes, the change certainly change the current behavior, but not so dramatically, I hope.

When the effect as you described is needed they can use the following code:
setDefaultValue("Empty");
setValue(null);

I cannot think out any other ill effect caused by the fix.

I think that the design of Question classes is not the optimal one. The defaultValue is set from the constructor to the value, when value is always null, because children cannot reset it, super ctor must be called first...

abstract class PropertiesQuestion {
protected PropertiesQuestion(Interview interview, String tag) {
super(interview, tag);
clear();
setDefaultValue(value);
}
public void clear() {
setValue(defaultValue);
}
}

class MyQuestion extends PropertiesQuestion {
MyQuestion() {
super(...);
this.value = ...
}
}

bkurotsu
Offline
Joined: 2004-12-13

Yes, some of that strange initialization was due to earlier implementations of the API and the need to remain (binary) compatible with interviews already created. I'm still ok with the change, but we should warn existing interview owners of the change.

fda
Offline
Joined: 2005-05-27

Would it make sense to send a separate message to all JT users or just write in the release note?

Message was edited by: fda