On Mon, Mar 24, 2014 at 3:24 PM, Mario Ivankovits <ma...@datenwort.at> wrote:
> Hi Tomas!
>
> No worries, I’ll go the way on my own there then.
>
>
> But, after this discussion I do not see why one ever used .equals() at all.
>
> Look, it does not fit my needs, I do not see any use-case where one would add
> an removeListener with asymmetric .equals() and thus it is better you use ==
> I think.
As Martin pointed out, sometimes == doesn't work for you and you need
(symmetric) equals(). Another example is my proposed solution #2 to
your problem.
Also, using equals instead of identity comparison is consistent with
how Collection.remove() determines equality.
> This clarifies that EXACTLY the added listener instance is required to remove
> any listener AND it gives no room to discussions like we had because the
> indention is perfectly clear - even to those reading JavaFX core code and bug
> fixing things in JavaFX.
> For me this would be fine and understandable - I will go the
> decorator-cache-map so I will be fine always.
>
>
> BTW: There are always implementation details one has to assume. For example,
> I have to assume that the internals will always use the add/removeListener
> methods and not modifying the list (or whatever) directly.
> There is something which we need to hold on - even if there is no
> documentation for things like this.
> Any yay, if you patch some JavaFX core stuff, you will get deeper insights
> and do not need to assume anymore, you KNOW how it works. Sometimes it is not
> easy to misuse things then to get your job done! ;-)
>
>
> Also, if there is a risk in there current implementation it does not help
> killing the discussion with the argument that there are other risks too.
> I am pretty well aware about stuff like that.
>
>
> PS: I would love to explain in detail why I decorate change listener, but
> this is out of scope of this thread.
>
> Regards,
> Mario
>
>
> Am 24.03.2014 um 14:04 schrieb Tomas Mikula <tomas.mik...@gmail.com>:
>
>> Hi Mario,
>>
>> On Mon, Mar 24, 2014 at 8:46 AM, Mario Ivankovits <ma...@datenwort.at> wrote:
>>> Thanks for your answer!
>>>
>>> One thing, I think, we can agree on, is that it is overly complex for an OO
>>> language to decorate things like these listeners.
>>
>> I'm curious why you need to decorate the listeners in the first place.
>>
>>> What about introducing another interface like ChangeListenerDecoration with
>>> a special named „boolean decorates(ChangeListener)) method and call this if
>>> the listeners hold by *ExpressionHelp class implements that interface?
>>
>> If you are writing your own ObservableValue implementation, you can as
>> well implement your own ExpressionHelper that does whatever you want
>> (just that you don't forget this option ;-)).
>>
>>> BTW: If we care about symmetry, why not change the
>>> listener.equals(otherListener) to listener == otherListener at all? Then,
>>> there are not implementation details one can make use of.
>>> I even might argue that it is a security risk to use the .equals() the way
>>> it is now, as an evil listener implementation is able to remove all
>>> listeners from the list by simply returning true from equals()
>>> It should be the responsibility of the listener in the list to know if the
>>> passed in listener justify the removal of itself, right? … from a security
>>> standpoint! :-)
>>
>> You cannot secure your application at this level. An "evil" listener
>> might as well introduce memory leaks, cause stack overflow, or wipe
>> out your hard disk.
>>
>> Tomas
>>
>>>
>>> Regards,
>>> Mario
>>>
>>>
>>> Am 24.03.2014 um 07:31 schrieb Martin Sladecek <martin.slade...@oracle.com>:
>>>
>>>> Mario,
>>>> thanks for your suggestion, but I think your specific case will not
>>>> justify the change. First of all, as it was already said, equals should be
>>>> symmetric. Knowing the implementation, you could rely on the fact that
>>>> either the equals is called on the provided listener or on the listeners
>>>> already in the list. Might be that somebody else would need the asymmetry
>>>> to be implemented in the "listener" provided to removeListener() method
>>>> and the current situation suits him, so why do the change for your case?
>>>>
>>>> Anyway, relying on some implementation-specific behaviour is generally a
>>>> sign of bad code practice, so it should give you a hint that you should
>>>> find a different solution (like the one you discussed with Tomas already).
>>>> It might happen that the code would be changed / re-factored in the future
>>>> or some bug would be fixed there and behaviour will change again, breaking
>>>> your application in some future JFX version.
>>>>
>>>> Regards,
>>>> -Martin
>>>>
>>>>
>>>> On 22.3.2014 15:47, Mario Ivankovits wrote:
>>>>> The only thing which I ask for is to flip this „if" in the
>>>>> *ExpressionHelper classes:
>>>>> So, JavaFX does not break anything and it is up to the app developer to
>>>>> take the risk or not.
>>>>>
>>>>>
>>>>> if (listener.equals(changeListeners[index])) {
>>>>>
>>>>> If we flip this to
>>>>>
>>>>> if (changeListeners[index].equals(listener))
>>>>>
>>>>>
>>>>>
>>>>> Am 22.03.2014 um 15:42 schrieb Kevin Rushforth
>>>>> <kevin.rushfo...@oracle.com<mailto:kevin.rushfo...@oracle.com>>:
>>>>>
>>>>> If you are talking about a change to the JavaFX API, we are not likely to
>>>>> accept such a change if it breaks the contract of equals() or hashcode().
>>>>> In your own app it may not matter, but it is dangerous to assume it won't
>>>>> matter to anyone. Martin owns the core libraries and can comment further.
>>>>>
>>>>> -- Kevin
>>>>>
>>>>>
>>>>> Mario Ivankovits wrote:
>>>>>
>>>>> Hi Thomas!
>>>>>
>>>>> Thanks for your input. Because I want to decorated listeners added by
>>>>> JavaFX core I can not use the sub/unsub pattern.
>>>>> Your second proposal is almost what I do right now. In removeListener I
>>>>> consult a map where the decorated listeners and their undecorated one
>>>>> lives.
>>>>>
>>>>> Regarding the symmetric doubts. Such listeners will always be removed by
>>>>> passing in the undecorated object to removeListener.
>>>>> They should act like a transparent proxy.
>>>>>
>>>>> Even if this breaks the equals paradigm, in this special case I can
>>>>> perfectly live with an equals/hashCode implementation like this below.
>>>>> It won’t break your app; as long as both objects do not live in the same
>>>>> HashSet/Map …. for sure - but why should they?
>>>>>
>>>>> @Override
>>>>> public boolean equals(Object obj)
>>>>> {
>>>>> return obj == delegate; // && this == obj
>>>>> }
>>>>>
>>>>> @Override
>>>>> public int hashCode()
>>>>> {
>>>>> return delegate.hashCode();
>>>>> }
>>>>>
>>>>>
>>>>> Regards,
>>>>> Mario
>>>>>
>>>>>
>>>>> Am 22.03.2014 um 14:22 schrieb Tomas Mikula
>>>>> <tomas.mik...@gmail.com><mailto:tomas.mik...@gmail.com>:
>>>>>
>>>>>
>>>>>
>>>>> A simpler and more universal solution is to also override removeListener:
>>>>>
>>>>> public void removeListener(ChangeListener<? super T> listener) {
>>>>> super.removeListener(decorated(listener));
>>>>> }
>>>>>
>>>>> And the equals() method on decorated listeners only compares the
>>>>> delegates (thus is symmetric).
>>>>>
>>>>> Regards,
>>>>> Tomas
>>>>>
>>>>>
>>>>> On Sat, Mar 22, 2014 at 2:07 PM, Tomas Mikula
>>>>> <tomas.mik...@gmail.com><mailto:tomas.mik...@gmail.com> wrote:
>>>>>
>>>>>
>>>>> The suspicious thing about your solution is that your smart
>>>>> implementation of equals() is not symmetric.
>>>>>
>>>>> In case the observable value is visible only within your project, you
>>>>> could do this:
>>>>>
>>>>> interface Subscription {
>>>>> void unsubscribe();
>>>>> }
>>>>>
>>>>> class MyObservableValue<T> implements ObservableValue<T> {
>>>>> public Subscription subscribe(ChangeListener<? extends T> listener)
>>>>> {
>>>>> ChangeListener<T> decorated = decorate(listener);
>>>>> addListener(decorated);
>>>>> return () -> removeListener(decorated);
>>>>> }
>>>>> }
>>>>>
>>>>> and use subscribe()/unsubscribe() instead of
>>>>> addListener()/removeListener():
>>>>>
>>>>> Subscription sub = observableValue.subscribe(listener);
>>>>> // ...
>>>>> sub.unsubscribe();
>>>>>
>>>>> Of course this is not possible if you need to pass the observable
>>>>> value to the outside world as ObservableValue.
>>>>>
>>>>> Regards,
>>>>> Tomas
>>>>>
>>>>> On Sat, Mar 22, 2014 at 6:57 AM, Mario Ivankovits
>>>>> <ma...@datenwort.at><mailto:ma...@datenwort.at> wrote:
>>>>>
>>>>>
>>>>> Hi!
>>>>>
>>>>> In one of my ObservableValue implementations I do have the need to
>>>>> decorate ChangeListener added to it.
>>>>> Today this is somewhat complicated to implement, as I have to keep a map
>>>>> of the original listener to the decorated one to being able to handle the
>>>>> removal process of a listener. Because the outside World did not know
>>>>> that I decorated the listener and the instance passed in to
>>>>> removeListener ist the undecorated one.
>>>>>
>>>>> We can make things easier with a small change in
>>>>> ExpressionHelper.removeListener. When iterating through the listener
>>>>> list, the listener passed in as argument to removeListener is asked if it
>>>>> is equal to the one stored
>>>>>
>>>>> if (listener.equals(changeListeners[index])) {
>>>>>
>>>>> If we flip this to
>>>>>
>>>>> if (changeListeners[index].equals(listener)) {
>>>>>
>>>>> a listener decoration can be smart enough to not only check against
>>>>> itself, but also against its delegate.
>>>>>
>>>>> What do you think?
>>>>>
>>>>> I could prepare a patch for the *ExpressionHelper classes.
>>>>>
>>>>>
>>>>> Best regards,
>>>>> Mario
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>
