ThresholdMatcherEditor

[Rob Eden]

All -

I've checked in the ThresholdMatcherEditor and its supporting  
classes. Please take a look and let me know what you think.

A couple of notes:
   - ValueMatcherEditor is an interface that simply says that
     a MatcherEditor uses a value and allows logic inversion
     (more on this in a minute). We've had discussions about
     this a while back, but the basic reason is that I've
     defined stuff such that no value being set always
     causes everything to be matched. Adding the logic
     inversion method allows this to work any way you want
     whereas just wrapping the matcher in a NotMatcher
     would mess up that empty value case. Example later...
   - AbstractValueMatcherEditor makes doing the above very
     easy.
   - ThresholdMatcherEditor lets you use >, >=, <, <=, ==
     and != comparisons... as long as the items implement
     Comparable or you provide a Comparator.
   - TextMatcherEditor has been updated to extend
     AbstractValueMatcherEditor so it has the above semantics.
     This doesn't change the way it currently works, but allows
     you to invert the logic so it will match things that
     don't contain the strings you specify, but match
     everything when no text is provided.

   - Test cases have been provided and the test cases for
     TextMatcherEditor were beefed up to verify that the
     events being fired are still optimal (i.e., not just
     firing "changed" for everything or anything like that).


Bottom line as far as API changes:
   - New ThresholdMatcherEditor class
   - TextMatcherEditor now allows logic inversion


Rob

--

"Let your heart soar as high as it will.
  Refuse to be average."
                            - A. W. Tozer


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[Jesse Wilson]

Hey Rob ----

I've finally found a spare moment to go through your code.
The code is great, I like a lot of your ideas. I've made a whole
bunch of changes and I'm going to go through those changes
& I hope to justify them!

Note again that I'm being really particular about your code here -
it's always easier to criticize somebody else's code than to write
your own! But hopefully strict code reviews will result in a better
Glazed Lists!

LockFactory
I've replaced a class with an interface. To the user, the difference
is that they call LockFactory.DEFAULT.createReadWriteLock()
rather than LockFactory.createReadWriteLock(). To the API implementor,
the difference is that they can implement an interface rather than
extending our LockFactory class. The main reason I did this is
that I found the static methods createReadWriteLock() that call
non-static implementations to seem kind of gross. It was also
awkward to expose two version of each method - the static and
nonstatic versions. With this, there's no static methods.

I've also killed the debug flag. As you demonstrate in the
ThreadContentionPerformance class, it's already quite possible
to debug the implementation class of a lock, and I can't foresee
users taking advantage of a System property to do that. Plus,
if a problem occurs, there will usually be an Exception raised
anyway.

Hopefully the whole LockFactory stuff will become more automatic
when we finish the Java 5 to J2SE1.4 tool.

Naming Conventions.
We use glazedLists, and you prefer to use glazed_lists.
We use 4 spaces, and you use tabs. It's so not a big deal
'cause we can reformat in seconds. But I've changed your
code to match the rest of ours for foolish consistency.

ValueMatcherEditor & AbstractValueMatcherEditor
This interface assumes that if the value is null, then that's a special
case that means 'special case, match all'. But null doesn't go far  
enough. For
example, I could have an empty list, and that might be good enough
to mean 'special case, match all'. Similarly, with integers, 0 or -1
might mean 'special case, match all' to some users. With Strings,
null is not empty string, but for most filters, they might as well be  
equal.
Therefore I am against ValueMatcherEditor and  
AbstractValueMatcherEditor.
It is easy enough for the implementor of MatcherEditor to determine
what the special cases are for them, and handle them appropriately.

One more thing to notice about your AbstractValueMatcherEditor class -
you've used the volatile keyword, which I've never seen in practice
before. I'm aware that it means 'don't cache this value in a  
register', and
I don't think its necessary in this case. We separated Matchers  
(immutable)
from MatcherEditors (mutable) to solve our thread safety problems, and
I think this volatile keyword contradicts that distinction. I have  
trouble
imagining when a user would have multiple threads manipulating a
MatcherEditor - it's either exclusively a user interface thing (1  
thread) or
must entirely be handled with heavy synchronization. Volatile doesn't
seem to solve any problems for me.

I'd like to confirm with you that these problems are legit before
we decide to keep or kill ValueMatcherEditor /  
AbstractValueMatcherEditor


ThresholdMatcherEditor
setValue() can be called on a ThresholdMatcherEditor, which
does not fire the event you expect it to. Instead, you need to call
setThreshold, which might cause problems.
I attempted to revise this class to simplify it and I ended up
overhauling the whole thing. The biggest problem with my changes
is that invertLogic() is no longer supported, but as I said before I
think this approach needs reconsideration anyway.
Be sure to check out how I changed the class - I feel it is much
simpler than it was before. Most of this is probably due to the
fact that I got to take advantage of some new APIs that have
become available since this class was originally crafted.

Please review my responses and we can get a dialog
going regarding ValueMatcherEditor.

Cheers,
Jesse


On 3-Aug-05, at 11:51 AM, Rob Eden wrote:

> All -
>
> I've checked in the ThresholdMatcherEditor and its supporting  
> classes. Please take a look and let me know what you think.
>
> A couple of notes:
>   - ValueMatcherEditor is an interface that simply says that
>     a MatcherEditor uses a value and allows logic inversion
>     (more on this in a minute). We've had discussions about
>     this a while back, but the basic reason is that I've
>     defined stuff such that no value being set always
>     causes everything to be matched. Adding the logic
>     inversion method allows this to work any way you want
>     whereas just wrapping the matcher in a NotMatcher
>     would mess up that empty value case. Example later...
>   - AbstractValueMatcherEditor makes doing the above very
>     easy.
>   - ThresholdMatcherEditor lets you use >, >=, <, <=, ==
>     and != comparisons... as long as the items implement
>     Comparable or you provide a Comparator.
>   - TextMatcherEditor has been updated to extend
>     AbstractValueMatcherEditor so it has the above semantics.
>     This doesn't change the way it currently works, but allows
>     you to invert the logic so it will match things that
>     don't contain the strings you specify, but match
>     everything when no text is provided.
>
>   - Test cases have been provided and the test cases for
>     TextMatcherEditor were beefed up to verify that the
>     events being fired are still optimal (i.e., not just
>     firing "changed" for everything or anything like that).
>
>
> Bottom line as far as API changes:
>   - New ThresholdMatcherEditor class
>   - TextMatcherEditor now allows logic inversion
>
>
> Rob
>
> --
>
> "Let your heart soar as high as it will.
>  Refuse to be average."
>                            - A. W. Tozer
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

[Rob Eden]

> LockFactory
> I've replaced a class with an interface. To the user, the difference
> is that they call LockFactory.DEFAULT.createReadWriteLock()
> rather than LockFactory.createReadWriteLock(). To the API implementor,
> the difference is that they can implement an interface rather than
> extending our LockFactory class. The main reason I did this is
> that I found the static methods createReadWriteLock() that call
> non-static implementations to seem kind of gross. It was also
> awkward to expose two version of each method - the static and
> nonstatic versions. With this, there's no static methods.

Tomayto, tomahto. Whatever you prefer. :-)

> I've also killed the debug flag. As you demonstrate in the
> ThreadContentionPerformance class, it's already quite possible
> to debug the implementation class of a lock, and I can't foresee
> users taking advantage of a System property to do that. Plus,
> if a problem occurs, there will usually be an Exception raised
> anyway.

Ok, but you won't see an exception. You'll probably just be using the  
wrong locks and not know it. It seemed friendlier for three lines of  
code.

> Hopefully the whole LockFactory stuff will become more automatic
> when we finish the Java 5 to J2SE1.4 tool.

That'd be nice, but it still might not be a bad idea to allow for  
custom lock implementations. Someone might want to write JNI code to  
use OS-level locks for some reason. Who knows. IMHO, user control is  
a good thing (within reason).

> Naming Conventions.
> We use glazedLists, and you prefer to use glazed_lists.
> We use 4 spaces, and you use tabs. It's so not a big deal
> 'cause we can reformat in seconds. But I've changed your
> code to match the rest of ours for foolish consistency.

Yup, I do use C-style variable names. Sorry. Old habits die hard.

Sorry about the tabs. Hadn't noticed you guys use spaces. I've  
updated IDEA's formatting settings.

> ValueMatcherEditor & AbstractValueMatcherEditor
> This interface assumes that if the value is null, then that's a  
> special
> case that means 'special case, match all'. But null doesn't go far  
> enough. For
> example, I could have an empty list, and that might be good enough
> to mean 'special case, match all'. Similarly, with integers, 0 or -1
> might mean 'special case, match all' to some users. With Strings,
> null is not empty string, but for most filters, they might as well  
> be equal.
> Therefore I am against ValueMatcherEditor and  
> AbstractValueMatcherEditor.
> It is easy enough for the implementor of MatcherEditor to determine
> what the special cases are for them, and handle them appropriately.

The issue is: tell me how to implement a matcher that can have its  
logic inverted BUT always matches everything when the value is null.  
If you can meet this requirement, then I'll go away and stop griping  
about this.

Agreed, there are other values that can be special. There's no easy  
way to solve every case. My goal with the way things were designed  
was to make the things that 90% of people would be doing extremely  
easy and make the stuff the remaining 10% wants to do possible. The  
argument is this:
     1) Matchers rock for filters like iTunes searching
        field and "browse" lists thingy. See also Acquisition,
        Spotlight, etc.
     2) A very common model for those setups is that no value
        means no filter (whether the filter is inclusive or
        exclusive). This is probably the common case,
        but my argument does not depend on it being so.
     3) If you want to implement #2 with ValueMatcherEditor,
        no problem... done. If you do not want that behavior,
        no problem... just set the value to "new Object()"
        or something like that. Something bogus or a magic
        "no match" token. Easy.
     4) Now, take a look at the diagram I've attached. We
        want to implement a simple search bar that combines
        filters from four matchers. All four can be inverted.
        If the matcher itself doesn't support
        setLogicInverted() this becomes very tedious. You would
        have to remove the matcher from the
        CompositeMatcherEditor and wrap it in a NotMatcher.
        But, you also have to be listening for an empty text
        field (while good OO design might prevent you from
        having a reference to) so you can remove the
        NotMatcher only to have to re-add it when they type
        a character. This of course, is making you refilter
        your list more than necessary because you would
        have to refilter when you modify the matcher set.
        AbstractValueMatcherEditor can know that you don't
        have a value and so no re-filter is needed.

Basically, if you can tell me how to easily implement my diagram  
using your scheme, I'll be convinced. Yes, the burden is on the  
MatcherEditor writer and it's not as simple as with your design  
(although I don't think it's much more complicated). However, it  
makes usage of MatcherEditors much simpler, IMHO and that's the  
common case by far.

> One more thing to notice about your AbstractValueMatcherEditor class -
> you've used the volatile keyword, which I've never seen in practice
> before. I'm aware that it means 'don't cache this value in a  
> register', and
> I don't think its necessary in this case. We separated Matchers  
> (immutable)
> from MatcherEditors (mutable) to solve our thread safety problems, and
> I think this volatile keyword contradicts that distinction. I have  
> trouble
> imagining when a user would have multiple threads manipulating a
> MatcherEditor - it's either exclusively a user interface thing (1  
> thread) or
> must entirely be handled with heavy synchronization. Volatile doesn't
> seem to solve any problems for me.

The use of volatile is technically correct in this case, although  
whether or not it would be necessary in real life is unknown. The  
main situation is for getValue()/setValue(). It is allowed by the JVM  
spec for VM's to cache the value of fields and it will especially  
happen in multi-processor situations. For example, if the value was A  
and then the value is set to B, threads may not see the change (even  
over long periods of time) by calling getValue() if volatile is  
removed because they might not re-sync. There are at least two ways  
to fix this:
     1) use the volatile keyword. This tells the VM
        to re-sync anytime it accesses the field
     2) synchronize the contexts (e.g., methods) that
        access the field. So, we could synchronize the
        getValue/setValue methods.

I thought about #2, but the penalty would be higher because there's  
significant work done (like firing events that could cause the whole  
map to be re-filtered) than could be painful to do in a synchronized  
context.

As for an example usage, take a look at the implementation of any of  
the Atomic* classes in 5.0. They're all implemented using volatile.  
Notice there is no synchronization or locking involved for get() and  
set().

> I'd like to confirm with you that these problems are legit before
> we decide to keep or kill ValueMatcherEditor /  
> AbstractValueMatcherEditor
>
>
> ThresholdMatcherEditor
> setValue() can be called on a ThresholdMatcherEditor, which
> does not fire the event you expect it to. Instead, you need to call
> setThreshold, which might cause problems.

Users can't call setValue(). It's protected... only for implementations.

> I attempted to revise this class to simplify it and I ended up
> overhauling the whole thing. The biggest problem with my changes
> is that invertLogic() is no longer supported, but as I said before I
> think this approach needs reconsideration anyway.
> Be sure to check out how I changed the class - I feel it is much
> simpler than it was before. Most of this is probably due to the
> fact that I got to take advantage of some new APIs that have
> become available since this class was originally crafted.

Yeah, but... logic inversion is the whole reason for the complexity.  
In my mind it's a big thing (see above).

Rob

--

"Let your heart soar as high as it will.
  Refuse to be average."
                            - A. W. Tozer



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[Jesse Wilson]

On 4-Aug-05, at 11:32 PM, Rob Eden wrote:

>> I can't foresee users taking advantage of a System property to do  
>> that. ...
> Ok, but you won't see an exception. You'll probably just be using  
> the wrong locks and not know it. It seemed friendlier for three  
> lines of code.
>> ...Hopefully the whole LockFactory stuff will become more automatic
>> when we finish the Java 5 to J2SE1.4 tool.
> That'd be nice, but it still might not be a bad idea to allow for  
> custom lock implementations. Someone might want to write JNI code  
> to use OS-level locks for some reason. Who knows. IMHO, user  
> control is a good thing (within reason).

My general approach to API design is to give the user
as simple of an interface as possible. There's lots of places
where we have a balance between flexibility and simplicity.
In the case of which locking implementation to use, I think
that we can probably just do the right thing - use Java 1.4
locks on Java 1.4 and Java 5.0 locks on Java 5.0. For a great
read on the problem of offering users too much choice, I
really like this Joel on Software essay:
http://www.joelonsoftware.com/uibook/chapters/fog0000000059.html

>> ValueMatcherEditor & AbstractValueMatcherEditor
>> This interface assumes that if the value is null, then that's a  
>> special
>> case that means 'special case, match all'. But null doesn't go far  
>> enough....
> The issue is: tell me how to implement a matcher that can have its  
> logic inverted BUT always matches everything when the value is  
> null. If you can meet this requirement, then I'll go away and stop  
> griping about this.
> ....
> Basically, if you can tell me how to easily implement my diagram  
> using your scheme, I'll be convinced. Yes, the burden is on the  
> MatcherEditor writer and it's not as simple as with your design  
> (although I don't think it's much more complicated). However, it  
> makes usage of MatcherEditors much simpler, IMHO and that's the  
> common case by far.

I'm going to implement this feature here in an email. Here's my  
approach:
  - there's three states - 'always true', which is when no value
is selected, ie. the selected value doesn't mean anything,
plus 'passthrough', and 'invert'.
  - there's two methods: 'setUseFilter(boolean)' and 'setIinverted
(boolean)'
This gives us a bonus fourth state, 'always false'. But in that special
case we're going to treat it like 'match all' - the user probably  
doesn't
want to show nothing when they have [inverted] selected with an
invalid value.

public class TriStateMatcherEditor extends AbstractMatcherEditor {
     private MatcherEditor target;
     private boolean inverted = false;
     private boolean useFilter = true;
     public TriStateMatcherEditor(MatcherEditor target) {
         this.target = target;
         target.addMatcherEditorListener(new MatcherEditor.Listener() {
             public void changedMatcher(MatcherEditor.Event event) {
                  if(!useFilter) return;
                  if(inverted) {
                      fireChangedMatcher(new MatcherEditor.Event
(TriStateMatcherEditor.this, invertType(event.getType()),  
Matchers.invert(event.getMatcher()));
                  } else {
                      fireChangedMatcher(new MatcherEditor.Event
(TriStateMatcherEditor.this, event.getType(), event.getMatcher()));
                  }
             }
         });
         fireChanged(target.getMatcher());
     }
     public void setInverted(boolean inverted) {
         if(this.inverted == inverted) return;
         this.inverted = inverted;
         if(!useFilter) continue;
         if(this.inverted) fireChanged(Matchers.invert
(target.getMatcher()));
         else fireChanged(target.getMatcher());
     }
     public void setUseFilter(boolean useFilter) {
         if(this.useFilter == useFilter) return;
         this.useFilter = useFilter;
         if(useFilter) {
             if(inverted) fireChanged(Matchers.invert
(target.getMatcher());
             else fireChanged(target.getMatcher());
         } else {
             fireMatchAll(); // don't care if we're inverted or not
         }
     }
}

  - the combobox from your mockup controls the 'setInverted'
field of this MatcherEditor
   - some sort of user-defined validator can control the
useFilter field. For Strings, use something like this in a document
listener: setUseFilter(getText().trim().length() > 0). This allows the
user to define what values make sense and what values don't make
sense. It shouldn't just be 'null', as is what AbstractValueMatcher
imposes.


> The use of volatile is technically correct in this case, although  
> whether or not it would be necessary in real life is unknown. ...
> As for an example usage, take a look at the implementation of any  
> of the Atomic* classes in 5.0. They're all implemented using  
> volatile. Notice there is no synchronization or locking involved  
> for get() and set().

My problem was that we don't want to add synchronization either.
volatile is necessary for the atomic classes because they are
explicitly designed for concurrent use. I don't think that's a good
practice for our MatcherEditors though. The other thing is that the
actual keyword volatile buys you absolutely nothing in terms of
concurrency in this case. It doesn't make anything safer.

Users are going to be using MatcherEditors from the EDT
and matchers from all sorts of threads. The matcher editors
don't need to be threadsafe, and sprinkling 'volatile' throughout
wouldn't make them that way anyway.

Let me know what you think of the proposed TriStateMatcherEditor.
I don't think we want that in Glazed Lists but it might be a good
topic for an article or screencast!

Cheers,
Jesse