Skip to content
This repository has been archived by the owner on Sep 28, 2024. It is now read-only.

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants