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] |
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] > > |
> 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. 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. 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] |
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). 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 |
Free forum by Nabble | Edit this page |