Ability to decorate ChangeListener
Mario Ivankovits
mario at datenwort.at
Mon Mar 24 14:24:15 UTC 2014
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.
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.mikula at gmail.com>:
> Hi Mario,
>
> On Mon, Mar 24, 2014 at 8:46 AM, Mario Ivankovits <mario at 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.sladecek at 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.rushforth at oracle.com<mailto:kevin.rushforth at 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.mikula at gmail.com><mailto:tomas.mikula at 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.mikula at gmail.com><mailto:tomas.mikula at 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 <mario at datenwort.at><mailto:mario at 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
More information about the openjfx-dev
mailing list