issue 515

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

issue 515

hbrands
Administrator
Hey James,

please have a look at http://java.net/jira/browse/GLAZEDLISTS-515

WDYT?

Thanks,
Holger

Reply | Threaded
Open this post in threaded view
|

Re: issue 515

Rob Eden
I think they should just be using the passed in parameters (or Matchers.true/falseMatcher as appropriate). There's really no need to reference currentMatcher at all (for reading).

Note however that things might not work entirely as expected if they're overriding getMatcher() and ignoring currentMatcher as the fire* methods also update that value.

Rob

On Tue, Jul 26, 2011 at 2:07 PM, Holger Brands <[hidden email]> wrote:
Hey James,

please have a look at http://java.net/jira/browse/GLAZEDLISTS-515

WDYT?

Thanks,
Holger


Reply | Threaded
Open this post in threaded view
|

Re: issue 515

hbrands
Administrator
I see what you mean.

Thinking more about it, especially your last sentence, I think it's unfortunate that ThreadedMatcherEditor is derived from AbstractMatcherEditor:
Users are allowed to subclass ThreadedMatcherEditor but the protected field "currentMatcher" never has a valid value.
And does it make sense to call the protected fire*-methods in a subclass of ThreadedMatcherEditor?

Assuming it makes no sense (please correct me if I'm wrong!), we could do the following:

a) extract another base class from AbstractMatcherEditor which only has the listenerList with addMatcherEditorListener(..), removeMatcherEditorListener(..) and fireChangedMatcher(MatcherEditor.Event) methods

b) derive ThreadedMatcherEditor from the new base class

c) make AbstractMatcherEditor.getMatcher() final to ensure the "hidden" contract (getMatcher() == currentMatcher)

d) make AbstractMatcherEditor.currentMatcher private, so you can only change it with the protected fire*-methods. This also eliminates the programming error of changing the currentMatcher without notifying registered listeners

e) optionally we could offer convenience factory methods for MatcherEvents in the topmost base class for users who don't want to derive from AbstractMatcherEditor, e.g.
protected MatcherEditor.Event<E> createChangedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CHANGED, matcher);
}
protected MatcherEditor.Event<E> createConstrainedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CONSTRAINED, matcher);
}
...

Does this make sense?

Holger

2011/7/26 Rob Eden <[hidden email]>
I think they should just be using the passed in parameters (or Matchers.true/falseMatcher as appropriate). There's really no need to reference currentMatcher at all (for reading).

Note however that things might not work entirely as expected if they're overriding getMatcher() and ignoring currentMatcher as the fire* methods also update that value.

Rob

On Tue, Jul 26, 2011 at 2:07 PM, Holger Brands <[hidden email]> wrote:
Hey James,

please have a look at http://java.net/jira/browse/GLAZEDLISTS-515

WDYT?

Thanks,
Holger



Reply | Threaded
Open this post in threaded view
|

Re: issue 515

Rob Eden
Well, I don't know that I'd say it's never correct. I think if the fire* methods in AbstractMatcherEditor used the parameters, it's probably about as good as possible (other than currentMatcher should be marked as volatile).

I could definitely be missing something, but I don't see offhand what could be done beside that.

Rob

On Wed, Jul 27, 2011 at 5:25 AM, Holger Brands <[hidden email]> wrote:
I see what you mean.

Thinking more about it, especially your last sentence, I think it's unfortunate that ThreadedMatcherEditor is derived from AbstractMatcherEditor:
Users are allowed to subclass ThreadedMatcherEditor but the protected field "currentMatcher" never has a valid value.
And does it make sense to call the protected fire*-methods in a subclass of ThreadedMatcherEditor?

Assuming it makes no sense (please correct me if I'm wrong!), we could do the following:

a) extract another base class from AbstractMatcherEditor which only has the listenerList with addMatcherEditorListener(..), removeMatcherEditorListener(..) and fireChangedMatcher(MatcherEditor.Event) methods

b) derive ThreadedMatcherEditor from the new base class

c) make AbstractMatcherEditor.getMatcher() final to ensure the "hidden" contract (getMatcher() == currentMatcher)

d) make AbstractMatcherEditor.currentMatcher private, so you can only change it with the protected fire*-methods. This also eliminates the programming error of changing the currentMatcher without notifying registered listeners

e) optionally we could offer convenience factory methods for MatcherEvents in the topmost base class for users who don't want to derive from AbstractMatcherEditor, e.g.
protected MatcherEditor.Event<E> createChangedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CHANGED, matcher);
}
protected MatcherEditor.Event<E> createConstrainedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CONSTRAINED, matcher);
}
...

Does this make sense?

Holger


2011/7/26 Rob Eden <[hidden email]>
I think they should just be using the passed in parameters (or Matchers.true/falseMatcher as appropriate). There's really no need to reference currentMatcher at all (for reading).

Note however that things might not work entirely as expected if they're overriding getMatcher() and ignoring currentMatcher as the fire* methods also update that value.

Rob

On Tue, Jul 26, 2011 at 2:07 PM, Holger Brands <[hidden email]> wrote:
Hey James,

please have a look at http://java.net/jira/browse/GLAZEDLISTS-515

WDYT?

Thanks,
Holger




Reply | Threaded
Open this post in threaded view
|

Re: issue 515

hbrands
Administrator
To come back to this issue, I've refactored the current code to show what I meant.
I've attached the three main files and a diff as patch file.

There is a new base class AbstractMatcherEditorListenerSupport extracted from AbstractMatcherEditor.
ThreadedMatcherEditor is now derived from AbstractMatcherEditorListenerSupport.
AbstractMatcherEditor.currentMatcher is now private.
AbstractMatcherEditor.getMatcher() is now final.

If users want to implement a MatcherEditor without the notion of a "currentMatcher" they should derive from
AbstractMatcherEditorListenerSupport instead of AbstractMatcherEditor, as done with ThreadedMatcherEditor.

This solution could break client code, but would be easy to fix.

What do you think?

Holger

2011/7/28 Rob Eden <[hidden email]>
Well, I don't know that I'd say it's never correct. I think if the fire* methods in AbstractMatcherEditor used the parameters, it's probably about as good as possible (other than currentMatcher should be marked as volatile).

I could definitely be missing something, but I don't see offhand what could be done beside that.

Rob


On Wed, Jul 27, 2011 at 5:25 AM, Holger Brands <[hidden email]> wrote:
I see what you mean.

Thinking more about it, especially your last sentence, I think it's unfortunate that ThreadedMatcherEditor is derived from AbstractMatcherEditor:
Users are allowed to subclass ThreadedMatcherEditor but the protected field "currentMatcher" never has a valid value.
And does it make sense to call the protected fire*-methods in a subclass of ThreadedMatcherEditor?

Assuming it makes no sense (please correct me if I'm wrong!), we could do the following:

a) extract another base class from AbstractMatcherEditor which only has the listenerList with addMatcherEditorListener(..), removeMatcherEditorListener(..) and fireChangedMatcher(MatcherEditor.Event) methods

b) derive ThreadedMatcherEditor from the new base class

c) make AbstractMatcherEditor.getMatcher() final to ensure the "hidden" contract (getMatcher() == currentMatcher)

d) make AbstractMatcherEditor.currentMatcher private, so you can only change it with the protected fire*-methods. This also eliminates the programming error of changing the currentMatcher without notifying registered listeners

e) optionally we could offer convenience factory methods for MatcherEvents in the topmost base class for users who don't want to derive from AbstractMatcherEditor, e.g.
protected MatcherEditor.Event<E> createChangedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CHANGED, matcher);
}
protected MatcherEditor.Event<E> createConstrainedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CONSTRAINED, matcher);
}
...

Does this make sense?

Holger


2011/7/26 Rob Eden <[hidden email]>
I think they should just be using the passed in parameters (or Matchers.true/falseMatcher as appropriate). There's really no need to reference currentMatcher at all (for reading).

Note however that things might not work entirely as expected if they're overriding getMatcher() and ignoring currentMatcher as the fire* methods also update that value.

Rob

On Tue, Jul 26, 2011 at 2:07 PM, Holger Brands <[hidden email]> wrote:
Hey James,

please have a look at http://java.net/jira/browse/GLAZEDLISTS-515

WDYT?

Thanks,
Holger






matcherEditor_patch.txt (10K) Download Attachment
AbstractMatcherEditor.java (4K) Download Attachment
AbstractMatcherEditorListenerSupport.java (2K) Download Attachment
ThreadedMatcherEditor.java (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: issue 515

hbrands
Administrator
If there are no objections, I'll commit this at the weekend...

Holger

2011/8/13 Holger Brands <[hidden email]>
To come back to this issue, I've refactored the current code to show what I meant.
I've attached the three main files and a diff as patch file.

There is a new base class AbstractMatcherEditorListenerSupport extracted from AbstractMatcherEditor.
ThreadedMatcherEditor is now derived from AbstractMatcherEditorListenerSupport.
AbstractMatcherEditor.currentMatcher is now private.
AbstractMatcherEditor.getMatcher() is now final.

If users want to implement a MatcherEditor without the notion of a "currentMatcher" they should derive from
AbstractMatcherEditorListenerSupport instead of AbstractMatcherEditor, as done with ThreadedMatcherEditor.

This solution could break client code, but would be easy to fix.

What do you think?

Holger


2011/7/28 Rob Eden <[hidden email]>
Well, I don't know that I'd say it's never correct. I think if the fire* methods in AbstractMatcherEditor used the parameters, it's probably about as good as possible (other than currentMatcher should be marked as volatile).

I could definitely be missing something, but I don't see offhand what could be done beside that.

Rob


On Wed, Jul 27, 2011 at 5:25 AM, Holger Brands <[hidden email]> wrote:
I see what you mean.

Thinking more about it, especially your last sentence, I think it's unfortunate that ThreadedMatcherEditor is derived from AbstractMatcherEditor:
Users are allowed to subclass ThreadedMatcherEditor but the protected field "currentMatcher" never has a valid value.
And does it make sense to call the protected fire*-methods in a subclass of ThreadedMatcherEditor?

Assuming it makes no sense (please correct me if I'm wrong!), we could do the following:

a) extract another base class from AbstractMatcherEditor which only has the listenerList with addMatcherEditorListener(..), removeMatcherEditorListener(..) and fireChangedMatcher(MatcherEditor.Event) methods

b) derive ThreadedMatcherEditor from the new base class

c) make AbstractMatcherEditor.getMatcher() final to ensure the "hidden" contract (getMatcher() == currentMatcher)

d) make AbstractMatcherEditor.currentMatcher private, so you can only change it with the protected fire*-methods. This also eliminates the programming error of changing the currentMatcher without notifying registered listeners

e) optionally we could offer convenience factory methods for MatcherEvents in the topmost base class for users who don't want to derive from AbstractMatcherEditor, e.g.
protected MatcherEditor.Event<E> createChangedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CHANGED, matcher);
}
protected MatcherEditor.Event<E> createConstrainedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CONSTRAINED, matcher);
}
...

Does this make sense?

Holger


2011/7/26 Rob Eden <[hidden email]>
I think they should just be using the passed in parameters (or Matchers.true/falseMatcher as appropriate). There's really no need to reference currentMatcher at all (for reading).

Note however that things might not work entirely as expected if they're overriding getMatcher() and ignoring currentMatcher as the fire* methods also update that value.

Rob

On Tue, Jul 26, 2011 at 2:07 PM, Holger Brands <[hidden email]> wrote:
Hey James,

please have a look at http://java.net/jira/browse/GLAZEDLISTS-515

WDYT?

Thanks,
Holger






Reply | Threaded
Open this post in threaded view
|

Re: issue 515

hbrands
Administrator
Ok, I checked in the change...

Holger

2011/8/19 Holger Brands <[hidden email]>
If there are no objections, I'll commit this at the weekend...

Holger


2011/8/13 Holger Brands <[hidden email]>
To come back to this issue, I've refactored the current code to show what I meant.
I've attached the three main files and a diff as patch file.

There is a new base class AbstractMatcherEditorListenerSupport extracted from AbstractMatcherEditor.
ThreadedMatcherEditor is now derived from AbstractMatcherEditorListenerSupport.
AbstractMatcherEditor.currentMatcher is now private.
AbstractMatcherEditor.getMatcher() is now final.

If users want to implement a MatcherEditor without the notion of a "currentMatcher" they should derive from
AbstractMatcherEditorListenerSupport instead of AbstractMatcherEditor, as done with ThreadedMatcherEditor.

This solution could break client code, but would be easy to fix.

What do you think?

Holger


2011/7/28 Rob Eden <[hidden email]>
Well, I don't know that I'd say it's never correct. I think if the fire* methods in AbstractMatcherEditor used the parameters, it's probably about as good as possible (other than currentMatcher should be marked as volatile).

I could definitely be missing something, but I don't see offhand what could be done beside that.

Rob


On Wed, Jul 27, 2011 at 5:25 AM, Holger Brands <[hidden email]> wrote:
I see what you mean.

Thinking more about it, especially your last sentence, I think it's unfortunate that ThreadedMatcherEditor is derived from AbstractMatcherEditor:
Users are allowed to subclass ThreadedMatcherEditor but the protected field "currentMatcher" never has a valid value.
And does it make sense to call the protected fire*-methods in a subclass of ThreadedMatcherEditor?

Assuming it makes no sense (please correct me if I'm wrong!), we could do the following:

a) extract another base class from AbstractMatcherEditor which only has the listenerList with addMatcherEditorListener(..), removeMatcherEditorListener(..) and fireChangedMatcher(MatcherEditor.Event) methods

b) derive ThreadedMatcherEditor from the new base class

c) make AbstractMatcherEditor.getMatcher() final to ensure the "hidden" contract (getMatcher() == currentMatcher)

d) make AbstractMatcherEditor.currentMatcher private, so you can only change it with the protected fire*-methods. This also eliminates the programming error of changing the currentMatcher without notifying registered listeners

e) optionally we could offer convenience factory methods for MatcherEvents in the topmost base class for users who don't want to derive from AbstractMatcherEditor, e.g.
protected MatcherEditor.Event<E> createChangedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CHANGED, matcher);
}
protected MatcherEditor.Event<E> createConstrainedEvent(Matcher<E> matcher)  {
  return new MatcherEditor.Event<E>(this, Event.CONSTRAINED, matcher);
}
...

Does this make sense?

Holger


2011/7/26 Rob Eden <[hidden email]>
I think they should just be using the passed in parameters (or Matchers.true/falseMatcher as appropriate). There's really no need to reference currentMatcher at all (for reading).

Note however that things might not work entirely as expected if they're overriding getMatcher() and ignoring currentMatcher as the fire* methods also update that value.

Rob

On Tue, Jul 26, 2011 at 2:07 PM, Holger Brands <[hidden email]> wrote:
Hey James,

please have a look at http://java.net/jira/browse/GLAZEDLISTS-515

WDYT?

Thanks,
Holger