Thread
#compose
    j

    julioromano

    1 year ago
    In
    @Composable
    APIs, is it preferable to have a default
    onClick
    param as an empty lambda (
    onClick: () -> Unit = {}
    ) or as a nullable function (
    onclick: (() -> Unit)? = null
    )? Are there any pros and cons?
    a

    albertosh

    1 year ago
    API-wise, I prefer to work with non nullable objects while possible. It’s even a design pattern, the null object one. Applied to this usecase, our “null” object would be an empty closure so we can execute it freely without any concerns But beware that that’s API wise, it just makes developer life easier. But it comes with some penalties at runtime as doing a null check will probably be more performant than executing a closure, probably someone else can bring more light on the topic Of course, we have to put everything in a context. For this scenario, I don’t think that an empty closure exeution will have a significant impact in the overall performance. But it’s something to keep in mind
    Bryan Herbst

    Bryan Herbst

    1 year ago
    Offering an empty
    onClick
    as a default seems strange- it implies that your Composable requires handling click behavior, but offers doing nothing as a default. If I think about this in the context of a
    Button
    for example, I expect clicking the button would produce a ripple or other indication when I tap it, but that just wouldn’t do anything. That doesn’t sound like expected behavior. On the other hand, making the
    onClick
    nullable implies that handling clicks is optional. A good example of this type of API design decision in action is
    Checkbox
    , which offers a nullable
    onCheckedChange
    . Checkbox only adds the
    selectable()
    Modifier when this parameter is not null, so it doesn’t handle clicks at all when
    onCheckedChange
    is null. Short version: your choice here implies behavior for the component, and you should consider what correct behavior for your component is, including what’s required vs. what’s not.
    j

    julioromano

    1 year ago
    Cool, I like this semantics:1.
    onClick: () -> Unit = {}
    => onClick is required 2.
    onclick: (() -> Unit)? = null
    => onClick is optional But since in case 1 onClick is required, why having a default value at all? Doesn’t the
    = {}
    add confusion? It seems sort of… Yeah this parameter is required but you don’t have to pass it in…
    Albert Chang

    Albert Chang

    1 year ago
    That's why you shouldn't provide a default value when it is required.
    Alexandre Elias [G]

    Alexandre Elias [G]

    1 year ago
    note that there is an important behavior difference between passing null or {} to a Composable like Checkbox. when onClick is null, we do not provide a semantic onClick action to screenreaders, and they change their behavior to "inert UI element" instead of "clickable UI element".
    this accessibility concern was actually the primary motivation why we decided to make optional onClicks nullable
    likewise, in your library components, the recommended pattern is that when your onClick is optional, make sure that your implementation looks like
    val clickable = if (onClick != null) { Modifier.clickable(...) } else { Modifier }
    ... modifier.then(clickable)(as opposed to the shorter way
    modifier.clickable(onClick ?: {}, ...)
    ). That way, not only will you get a slight performance improvement, but more importantly it will send the right signals to accessibility
    j

    julioromano

    1 year ago
    I was a bit misled by the new (albeit experimental) API of
    Surface
    in beta09: it’s got two different @Composables with either no
    onClick
    at all or a non-null one. Since I needed an optional onClick in my custom Composable (which uses Surface internally) I had to use the version of Surface without onClick and then place a Box inside it and apply the onClick modifier to the Box:
    modifier = Modifier
            .fillMaxSize()
            .run {
              if (onClick == null) this
              else clickable(onClick = onClick)
            }
    Alexandre Elias [G]

    Alexandre Elias [G]

    1 year ago
    I see. I understand you needed to do that in order to get the indicator ripple to clip correctly. In fact, an earlier version of
    Button
    did it this way. However, we found a problem with this approach: the strong Compose convention is to put passed-in
    modifier
    parameter on the outermost
    Layout
    , but given that the
    Box
    is inside the
    Surface
    , either this convention must be violated, or a
    testTag
    that's passed in will appear not clickable
    here are two alternatives I would suggest: A) stop using
    Surface
    itself and instead directly chain the elevation, shadow, clip modifiers, B) conditionally use one 
    Surface
     overload or the other by storing the "content" lambda in a 
    val
     instead of following the usual concise last-lambda style
    there is a subtle behavior difference between A and B. A will pass through input events to other elements covered by the composable when nonclickable, and B will unconditionally block them (Surfaces are opaque to input). in usecases like Switch/Slider we use solution A inside Compose, whereas in cases like Pane/Drawer/Card I recommend B instead
    j

    julioromano

    1 year ago
    This was a very good explanation and helped a lot. Thank you!