Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add destructuring operators to MapChangeListener #1146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HoldYourWaffle
Copy link
Contributor

I added destructuring operators to MapChangeListener to allow usage like this:

observableMap.addListener(MapChangeListener { (key, added, removed) ->
	// exciting stuff
})

This might be more controversial than #1141 since the order is relatively arbitrary (although it feels quite natural to me).
If this addition is not desired feel free to close this pull request, otherwise I should probably also add these operators to the other collection types' listeners.

@ruckustboom
Copy link
Collaborator

ruckustboom commented Jan 6, 2020

I'm not sure I agree the order is so arbitrary. Pretty much all change listeners in JavaFX use the order (observable, old, new), which would correspond to to (map, valueRemoved, valueAdded). You could then add key as a 4th component, and you might as well add wasRemoved and wasAdded as the 5th and 6th while you're at it.

Unfortunately that would make your example a little uglier:

observableMap.addListener(MapChangeListener { (_, removed, added, key) ->
	// exciting stuff
})

But it would be more in line with JavaFX std-lib.

@ruckustboom
Copy link
Collaborator

ruckustboom commented Jan 6, 2020

Also, I'm not sure if it's still the case, but historically we would often not specify the return type when dealing with JavaFX collections and properties, and thus just pass the platform type up the line. It may be a little against the Kotlin way, but it means we aren't making assumptions for the user.

For example, look at the delegation helpers for properties.

@edvin
Copy link
Owner

edvin commented Jan 6, 2020

I agree with @ruckustboom - the changelistener order should be maintained if we add this. Not sure of the value if we do though, as the example illustrates.

@HoldYourWaffle
Copy link
Contributor Author

Sorry for not responding for so long, but I just got back into TornadoFX and funnily stumbled into this again.

If I understand correctly there are two changes I should make:

  1. Don't specify the return type explicitly but pass the platform type (non-idiomatic but fully justified IMHO)
  2. Change the order of the components to observable, old, new, key, wasRemoved, wasAdded (ugly but more consistent)

Would that be correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants