mvysny / vaadin-on-kotlin

Writing full-stack statically-typed web apps on JVM at its simplest
https://www.vaadinonkotlin.eu/
MIT License
186 stars 17 forks source link

trimmingConverter for FilterBar? #45

Closed jhult closed 4 years ago

jhult commented 4 years ago

Karibu-DSL has a trimmingConverter available. It would be nice if this (or something similar) was available for a FilterBar.

mvysny commented 4 years ago

Thank you, that's a good idea! However I wonder: usually you always want to have such a functionality, and so I wonder whether the eq() or ilike() or fullText() functions should perhaps perform trimming automatically...

Perhaps the best way would be to have eq(), ilike() and fullText() to perform trimming automatically, and still have the trimmingConverter() function around, for highly customized filters.

Could you please provide a feedback on the FilterBar? Do you like the idea? Is the FilterBar code understandable, or is it too complicated? The Builder has 3 generics and that is usually too much... also lots of ops are only available as extension functions (e.g. bind()) which could be confusing since Intellij will not auto-complete that method unless the binding can be finalized...

jhult commented 4 years ago

I think it would be fine to automatically trim inside eq(), ilike(), or fullText(). However, maybe add an optional Boolean which allows disabling this.

I converted from the original Grid filtering available to the new FilterBar method in the past few weeks. That was quite the effort as it took me a while to figure everything out. However, I did eventually get it all converted over.

I ran into issues using inRange because it required DateInterval. I ended up rolling some of my own helper functions. Here is an example:

fun <BEAN : Any, FILTER : Filter<BEAN>> FilterBar<BEAN, FILTER>.datePicker(column: Grid.Column<BEAN>): FilterBar.Binding<BEAN, FILTER> {
    require(!column.key.isNullOrBlank()) { "The column needs to have the property name as its key" }
    val component = DatePicker().apply {
        max = LocalDate.now()
        isClearButtonVisible = true
        setWidthFull()
    }
    return FilterBar.Binding.Builder(filterFactory, this, column, component) {
        // value getter
        component.value
    }.withConverter {
        DateInterval(it, it).toFilter(column.key, filterFactory, LocalDateTime::class.java)
    }.bind()
}

I couldn't use LocalDate.toFilter() because it was private. I couldn't use DateInterval.toFilter() because I didn't initially have a DateInterval

Also, it would be nice if the actual Component type (e.g. TextField, DatePicker, etc.) was stored in the Builder and available to retrieve (instead of just Component).

jhult commented 4 years ago

Also, on a perhaps side topic, I was expecting that these 2 would be compatible but they are not:

In addition, it would be nice for those 2 *FilterFactory classes to be open so they could be overridden. Having the actual Filter classes open would also be nice, but they are data classes (for good reason) and thus can't be open :(

mvysny commented 4 years ago

Awesome feedback, thank you so much! Could you please create separate issues for every topic, so that it can be discussed separately? We can then focus on discussing the String trimming in filters.

mvysny commented 4 years ago

Regarding the PredicateFilterFactory: https://github.com/mvysny/vaadin-on-kotlin/issues/46

mvysny commented 4 years ago

I converted from the original Grid filtering available to the new FilterBar method in the past few weeks. That was quite the effort as it took me a while to figure everything out. However, I did eventually get it all converted over.

I'm sorry to hear that, but I'm happy that you managed to do the conversion. I think the new API is better since it puts you in control (as opposed to the old auto-generator API where you had to hook into certain places to get the behaviour you needed). However, I need to keep the backward compatibility in mind also, to ease the migration path.

mvysny commented 4 years ago

I couldn't use LocalDate.toFilter() because it was private.

Could you please open a new feature request and write your use-case for using a single DatePicker as a filter component? Perhaps we can include your excellent code into VoK if the use-case is generic enough.

mvysny commented 4 years ago

If I'm not mistaken, this leaves only the original issue to be implemented. Even though ILikeFilter, LikeFilter and FullTextFilter perform automatic trimming, eq() does not, so trimmingConverter() still makes sense. I'll implement :+1:

mvysny commented 4 years ago

Implemented LocalDate.toFilter() in 342e73fa1f048d83944f76a955263d53519799ba