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 ability to setup comparator to SortedFilteredList, added example #1024

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

Conversation

sith
Copy link

@sith sith commented Jul 4, 2019

It looks like that SortedFilteredList doesn't have ability to setup comparator.

@edvin
Copy link
Owner

edvin commented Jul 5, 2019

I wonder if this approach would be more convenient to use:

class SortedFilteredList<T>(
        val items: ObservableList<T> = FXCollections.observableArrayList(),
        initialPredicate: (T) -> Boolean = { true },
        val filteredItems: FilteredList<T> = FilteredList(items, initialPredicate),
        val sortedItems: SortedList<T> = SortedList(filteredItems),
        comparator: java.util.Comparator<T>? = null) : ObservableList<T> {

    val comparatorProperty: ObjectProperty<java.util.Comparator<in T>>
        get() = sortedItems.comparatorProperty()
    var comparator by comparatorProperty

    init {
        comparator?.let { comparatorProperty.value = it }
        items.onChange { refilter() }
    }
...
}

First of all we won't break compatibility, and it's probably more realistic that you want to send in a Comparator instance, not a property. Now you can also bind a property if you want, the way you did it.

What do you think?

@sith
Copy link
Author

sith commented Jul 5, 2019

I am good with this approach.
Although I have 2 suggestions:

  1. refilter function is doing sorting so better name with javadoc would be helpful here. refilter function could be marked as deprecated and new one with better name is introduced.
  2. the main constructor of SortedFilteredList is a little bit messy. Now there will be 5 parameters and it is not clear which one should be set, and I think it is error prone. For example:
    private val numbers = SortedFilteredList(filteredItems = FilteredList<MyItem>(FXCollections.observableArrayList()))
    adding to numbers does nothing as it adds to underlying items collection that is not connected now. So I think constructor should have only 3 parameters, original source(with default value if needed), initialFilter and comparator

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

2 participants