Preview doesn't render BottomAppBar cutout [158551084]

Fixed

Bug

Status Update

No update yet.

Description

cl...@google.com

created issue #1

Jun 9, 2020 11:28AM

Component used: BottomAppBar

Version used: AS 4.1 Canary 8 + Compose snapshot at 6557927

I created a BottomAppBar with a FAB and cutout, and running the emulator it appears correct but in the preview the cutout is not rendered correctly.

Code:

@Composable
fun ReplyScaffold(content: @Composable () -> Unit) {
    Scaffold(
        backgroundColor = AppBackground,
        bottomBar = { ReplyBottomBar() },
        floatingActionButton = { ReplyFAB() },
        floatingActionButtonPosition = Scaffold.FabPosition.Center,
        isFloatingActionButtonDocked = true
    ) {
        content()
    }
}

@Composable
fun ReplyBottomBar() {
    // TODO: cutout doesn't work on preview
    BottomAppBar(cutoutShape = RoundedCornerShape(50)) {
        IconButton(onClick = {}) {
            Icon(Icons.Default.KeyboardArrowUp)
        }
        Spacer(modifier = Modifier.weight(1f))
        IconButton(onClick = {}) {
            Icon(Icons.Default.Search)
        }
    }
}

@Composable
fun ReplyFAB() {
    FloatingActionButton(
        onClick = {}
    ) {
        Icon(Icons.Default.Edit)
    }
}

@Preview
@Composable
fun ScaffoldPreview() {
    AppTheme {
        ReplyScaffold {
            Text("Hello")
        }
    }
}

Screenshot 2020-06-09 at 12.25.42.png

333 KB

View

Download

Comments

lp...@google.com <lp...@google.com> #2Jun 9, 2020 11:30AM

I know that there is an API council request to change Modifier.drawLayer to Modifier.graphicsLayer` thus dropping the "draw" prefix to the modifier. I think it might be reasonable to rename the following to match the naming of the fields within the layer API:

Introduce the following:

   Modifier.drawOpacity -> Modifier.alpha
   Modifier.scale,
   Modifier.rotateX,
   Modifier.rotateY,
   Modifier.rotate (maybe rotateZ but maybe drop the Z to keep it simple?)
   Modifier.translate

I think it is reasonable to have both Modifier.offset as well as translate with the documentation indicating that translate is more effective for animated use cases. The only concern I have is if we can somehow detect several of these modifiers in a chain would be able to effectively squash them into a single layer, (ex: Modifier.alpha(0.5f).scale(2f).rotate(45) etc.). What do you think?

an...@google.com <an...@google.com> #3Jun 9, 2020 11:36AM

1) you forgot to mention:
Modifier.drawShadow -> Modifier.shadow

2) I think for scale we can both overloads like
Modifier.scale(scaleX: Float, scaleY: Float)
Modifier.scale(scale: Float) = scale(scale, scale)

3) can we do similar thing for rotation? in simple cases you only want to rotate x and y, right?
Modifier.rotation(rotationX: Float, rotationY: Float, rotationZ: Float)
Modifier.rotation(rotation: Float) = rotation(rotation, rotation, 0)

4) and please, lets not add Modifier.translate() just yet. I am ok with this being available as more low level api Modifier.drawLayer() where we have more options, but not something so easy discoverable. in the doc we agreed that it could be useful to keep it for some complex cases. I am strongly convinced we shoudn't have both Modifier.offsetPx() and Modifier.translation()

5) so the decision to rename to graphicLayer() is final? I have a cl which will be affected by that:

https://android-review.googlesource.com/c/platform/frameworks/support/+/1496274. Should I already use new name GraphicLayerScope there?

[Deleted User] <[Deleted User]> #4Jun 11, 2020 02:32PM

And about the possible squashing, it is not really related to this topic as even now it is possible to do Modifier.drawLayer(alpha=0.5f).drawLayer(scale=2f).drawLayer(rotate=45). But yes, we could explore it in the future. I think not squashing at all is good enough for now

[Deleted User] <[Deleted User]> #5Jun 11, 2020 02:35PM

sure we can have Modifier.shadow. The general pattern is is to drop the draw prefix and match the name of the modifier with the field on the layer itself instead of deviating with Opacity vs alpha (should change to alpha)
I am apprehensive about keeping the scale/rotation parameters separate as it is a deviation from the atomic transformation done in drawLayer currently. That is the pivot point used for scale/rotation is shared and the order of the transformations is always fixed.
Most of the time folks are only interested in the "regular" rotation which is rotateZ. I think we should have distinct rotationX/Y methods
I think we need to decouple the layer primitive from the modifier. For example, the graphics/DrawScope API doesn't deal with Modifiers at all, and the Modifier should wrap that Layer primitive instead. This would still allow for usage of translate within the Layer primitive for graphics purposes. Modifier.offset will always be slower than Modifier.translate though and force a full layout pass instead of just a quick invalidation so we are paying a performance penalty if we only expose Modifier.offsetPx
The discussion came up in this API council feedback doc regarding renaming Modifier.drawLayer to Modifier.graphicsLayer: https://docs.google.com/document/d/1JBwAbh_gwssah1X21waDpbS1f7M6yNbb3V8qanTlWds/edit?ts=5f5fd265

an...@google.com <an...@google.com> Jul 21, 2020 05:09PM

Reassigned to an...@google.com.

an...@google.com <an...@google.com> #6Aug 4, 2020 04:27PM

Reassigned to ma...@google.com.

Regarding squashing, the issue will be exacerbated by breaking these out into separate modifiers. While you can still chain multiple transformations together, keeping separate modifiers per property will make it too easy to create too many layers by mistake. At least with drawLayer all the properties are there

lp...@google.com <lp...@google.com> Sep 16, 2020 04:26PM

Reassigned to lp...@google.com.

lp...@google.com <lp...@google.com> Sep 29, 2020 07:50PM

Marked as fixed.

ap...@google.com <ap...@google.com> #7Sep 29, 2020 07:50PM

2,3. will leave the decision on scale/rotation on you as you have more context here.

4. "full layout pass" sounds scary but in fact we only need to re-invoke affected layout block(s), even without remeasuring. so it is cheap already. as I mentioned in the doc previously we do relayout for scrolling of the scrolling containers which is an operation of the same complexity as a translation animation as I would say. instead of providing two mechanisms for offsetting the component we just need one which is performant. if you have ideas how to do even less work in simple relayouts lets optimize it instead of overcomplicating the public api.

5. Yes, I saw this feedback. My question was mostly about when are you going to rename/as there is no reason to introduce DrawLayerScope to rename it to GraphicLayerScope right after