android / android-ktx

A set of Kotlin extensions for Android app development.
https://android.github.io/android-ktx/core-ktx/
7.48k stars 565 forks source link

Spannable String clicks and other feedback #511

Open nschwermann opened 6 years ago

nschwermann commented 6 years ago

Add ClickableSpan as part of the builder. It should be nestable in other spans. I am not exactly familiar what the DSL in ktx looks like for spannable strings but something like this should be possible. Looking at the code I think it would look something like this...

textView.setSpanableString{ 
   italic{
    append("This italic text isn't clickable")
   }
  append("/n")
  bold {
    append("This bold text IS clickable")
    onClick{
       //Click action goes here
    }
  }
  color(Color.RED){
     append("This color text is not clickable")
  }

}

Also, it looks like you have to manually call append for each textStyle, I suggest adding a text attribute for each style instead.

textView.setSpannableString{
  italic{
     text = "Italic text here" //dsl for append
     onClick{
       //click action
     }
  }
} 
JakeWharton commented 6 years ago

I'm a bit hesitant about adding clicks because of how easy it creates leaks. Although it's no different than using the ClickableSpan subclass I suppose.

Your syntax doesn't make sense in either context though. Our extensions are on a builder so the contents of the lambda are what is surrounded by the span.

nschwermann commented 6 years ago

Thanks for the feedback. Sorry I didn't take the time to better understand the current API. I have a similar (much older) implementation that allowed for combining spans but I had forgot how exactly I did it I had to go back and look. Looks something like this.

textView.setSpannableString{
  span{
    text = getString(R.string.hello)
    characterStyle = ForegroundColorSpan(getColor(R.color.accent))
    onClick {
      snackbar("Hello!")
    }
} 

This solution let you set multiple spans on the same text (one CharacterStyle, ParagraphStyle, UpdateAppearance, UpdateLayout, TextAppearanceSpan (giving a resource int reference), and ClickableSpan (given an onClick lambda)

So in my case the span dsl method had an argument of a Holder class which held all of the styles to apply to the builder for that text for each section.

public fun TextView.setTextWithBuilder(init :SpannableStringBuilder.() -> Unit) : Unit 
public fun SpannableStringBuilder.span(init : SpanHolder.() -> Unit) : SpannableStringBuilder

So it looks like with your API we could have something like this.

inline fun SpannableStringBuilder.click(click : (View) -> Unit, 
  updateDrawState : ((TextPain) -> Unit)? = null,
  builderAction : SpannableStringBuilder.() -> Unit) = inSpans(
   object : ClickableSpan {
      override fun updateDrawState(ds: TextPaint){updateDrawState?.invoke()}
      override fun onClick(widget: View) { click(widget) }
  }, builderAction = builderAction)

The only other thing is I believe for ClickableSpans to work correctly the flag needs to be Spanned.SPAN_EXCLUSIVE_EXCLUSIVE

Zhuinden commented 6 years ago

Actually, the tricky part is that you need to set the LinkMovementMethod on the TextView for it to work for some reason.

0809987675 commented 6 years ago

good