IssueTracker

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date:   Fri Jul 10 16:23:09 2020

    Semantics no-op cleanups
   
    Partly in response to lmr's broad code review, I did a pass of
    superficial API/implementation cleanup.  The main changes are:
   
    - I changed each Boolean SemanticsProperty where false is equivalent to
      not being present to take "Unit" instead.  This is conceptually
      clearer: it avoids questions like "can I cancel out a semantics from a
      merged child by setting it to false?"  Because "property = Unit" looks
      weird, I also changed the style of these to "property()".
   
    - I moved the Semantics id generator closer to where it's used, in
      SemanticsModifierCore.  I made it internal and an AtomicInt.
      (Note that integer ids are heavily used in the Android
      AccessibilityNodeInfo APIs so I can't simply remove them entirely.)
   
    - I deleted dead code.  Some examples include SemanticsHintOverrides,
      a public API not connected to anything, and SemanticsPropertyKey
      merge() open method which is never called.  (In both cases I have
      a different plan in mind for accessibility.)
   
    Fixes: 145951226
    Fixes: 145955412
    Test: existing tests
    Relnote: "Single-value semantics properties now use a calling style.
    For example, 'semantics { hidden = true }' is now written as:
    'semantics { hidden() }'."
   
    Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M       ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M       ui/ui-core/api/0.1.0-dev15.txt
M       ui/ui-core/api/current.txt
M       ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M       ui/ui-core/api/public_plus_experimental_current.txt
M       ui/ui-core/api/restricted_0.1.0-dev15.txt
M       ui/ui-core/api/restricted_current.txt
M       ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M       ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M       ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M       ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M       ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M       ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M       ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M       ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D       ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M       ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M       ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M       ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M       ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M       ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M       ui/ui-foundation/api/0.1.0-dev15.txt
M       ui/ui-foundation/api/current.txt
M       ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M       ui/ui-foundation/api/public_plus_experimental_current.txt
M       ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M       ui/ui-foundation/api/restricted_current.txt
M       ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M       ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M       ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M       ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M       ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M       ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M       ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M       ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M       ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M       ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M       ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M       ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M       ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M       ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M       ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M       ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M       ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M       ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M       ui/ui-text/api/0.1.0-dev15.txt
M       ui/ui-text/api/current.txt
M       ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M       ui/ui-text/api/public_plus_experimental_current.txt
M       ui/ui-text/api/restricted_0.1.0-dev15.txt
M       ui/ui-text/api/restricted_current.txt
M       ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M       ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

> It sounds like what you actually want is a setupToolbarWithNavController() equivalent that you could call in each Fragment?

No, not really. Since this call is basically adding a listener to each navigation event, I expected it to work no matter what the application state is.
If I need to call this from every fragment, this reduces the usefulness of this helper to zero.

This whole thing may be caused by my misunderstanding on how we should handle single activity applications. I am not sure the documentation is very clear on that topic? here are the grey areas:
- How can the action bar be managed by the activity (here with the nav controller events) if the action bar is added by fragments?
- When going deeper in the app hierarchy (like opening a document or anything), the drawer shouldn't be available anymore. However since it is attached to the activity, when the navigation occurs, the drawer still works and this is weird.
- How to build a navigation graph with a drawer? For example in Google+ there's the stream, and if I click "Events" in the drawer it opens another fragment (I guess). In the navigation graph, it is not possible to add activities so I'm guessing the "Events" fragment is linked to the stream fragment? This doesn't seem to make any sense (to my understanding).

These 3 last questions are out of topic of this bug, but they may help understand the context, what I was trying to do and what I was expecting.
If there is some documentation or samples, videos or anything that would explain that, please share.

Thanks a lot for your time.

> If I need to call this from every fragment, this reduces the usefulness of this helper to zero.

You'll still get the title set from the android:label in your navigation graph and proper behavior when it comes to the Up button (particularly important if you're using the DrawerLayout versions), so I wouldn't say zero. :)

> How can the action bar be managed by the activity (here with the nav controller events) if the action bar is added by fragments?

If you have a Toolbar in each Fragment, you should consider not using the ActionBar APIs at all. This was covered in the Fragment talk at I/O 2018: https://www.youtube.com/watch?v=WVPH48lUzGY&t=2255

> When going deeper in the app hierarchy (like opening a document or anything), the drawer shouldn't be available anymore. However since it is attached to the activity, when the navigation occurs, the drawer still works and this is weird.

Navigation drawers were always built to be a global mechanism for returning to a root set of destinations, so this seems to be WAI.

> How to build a navigation graph with a drawer? For example in Google+ there's the stream, and if I click "Events" in the drawer it opens another fragment (I guess). In the navigation graph, it is not possible to add activities so I'm guessing the "Events" fragment is linked to the stream fragment? This doesn't seem to make any sense (to my understanding).

Navigation destinations in a drawer would typically be children of the root of your navigation graph and therefore available to all other destinations. And you definitely can use <activity> destinations in your navigation graph (just note that navigating to a new activity is effectively leaving the navigation graph)

> If you have a Toolbar in each Fragment, you should consider not using the ActionBar APIs at all

So basically if I have a toolbar in each fragment layout, and my activity has a theme without action bar:
1. I shouldn't use setupActionBarWithNavController
2. I have to set the "up" arrow icon myself when getting into the app hierarchy, inside the fragment code
3. I have to set all the titles myself, they're not derived from the navigation graph
Is that correct?

> Navigation drawers were always built to be a global mechanism for returning to a root set of destinations, so this seems to be WAI.

Not sure about that, see my Google+ example. The drawer is only visible on the main screens (stream, collections, communities, notifications), but it is not when getting deeper (post view, events, contacts, etc)

> > How to build a navigation graph with a drawer?

Actually my question here was more like "how to disable the drawer when getting deeper into the app".
Like how to disable the drawer when opening a post in Google+. Is this something that has to be done by hand, or is it handled by the navigation API?

> And you definitely can use <activity> destinations in your navigation graph

Thanks for the clarification about <activity> and more globally how to use the navigation graph.

> It sounds like what you actually want is a setupToolbarWithNavController() equivalent that you could call in each Fragment?

After giving it more thought, and following your recommendations in the comments, actually maybe this is what I want/have to do.

So basically when setting up the fragment, I link to to the nav controller so that the navigation graph data is populated to the toolbar (home "up" button and title, basically).

The problem is, what is this `setupToolbarWithNavController()` you are referring to? It's not part of `NavigationUI`, is it?
I'm using the -alpha02 dependencies on 3.2 canary 17.

>So basically if I have a toolbar in each fragment layout, and my activity has a theme without action bar...

I think the proposed solution of having a setupToolbarWithNavController()-like method would do all the things you list, exactly like the setupActionBarWithNavController() does for the ActionBar.

> Not sure about that, see my Google+ example. The drawer is only visible on the main screens (stream, collections, communities, notifications), but it is not when getting deeper (post view, events, contacts, etc)

I'm not saying that Google+ does things correctly. I'm saying that the discussions with the material design team has made it clear that the navigation drawer is a global navigation pattern that should be available everywhere (in that respect, very similar to how the bottom nav bar should always be visible - also not something apps do consistently).

> Actually my question here was more like "how to disable the drawer when getting deeper into the app".
Like how to disable the drawer when opening a post in Google+. Is this something that has to be done by hand, or is it handled by the navigation API?

This isn't something Navigation would do automatically, but you can certainly add an OnNavigatedListener via https://developer.android.com/reference/androidx/navigation/NavController#addonnavigatedlistener and take any action you want, including locking the DrawerLayout closed.

I'm proposing making this bug into a feature request for adding a setupToolbarWithNavController type of method if you're okay with that.

> I'm not saying that Google+ does things correctly. I'm saying that the discussions with the material design team has made it clear that the navigation drawer is a global navigation pattern that should be available everywhere (in that respect, very similar to how the bottom nav bar should always be visible - also not something apps do consistently).

Very interesting. Thanks for the insights.

> This isn't something Navigation would do automatically, but you can certainly add an OnNavigatedListener via https://developer.android.com/reference/androidx/navigation/NavController#addonnavigatedlistener and take any action you want, including locking the DrawerLayout closed.

Thanks again. That's what I would have done, I was just wondering if the navigation API had something automatic here. But if the drawer is a global navigation pattern and should always be available, that would be useless.

About these two items, it's weird because none of the apps I have tested quickly today seem to have a global drawer: Inbox, Gmail, Google+, Google Music, Play Store, Messages, Maps, Photos — all of them hide/lock the drawer when opening an item in the app (email, post, track, app, etc).
The only two notable exceptions are docs (spreadsheets etc): they open a new task when opening a document, so that makes sense; and YouTube, they got rid of the drawer altogether.
So that's a bit contradictory with a global navigation pattern, and it doesn't make me want to have a global drawer everywhere.

> I'm proposing making this bug into a feature request for adding a setupToolbarWithNavController type of method if you're okay with that.

That would be awesome. For an activity-managed ActionBar, use `NavigationUI.setupActionBarWithNavController()`; and for a custom Toolbar, use `NavigationUI.setupToolbarWithNavController()`. Both do the same: get information from the navigation graph (label, whether it's the start destination or not, menus, etc).
Is that right?
If so, please convert this to a feature request.

Thanks Ian! (I guess this is you :))

Please disregard my remark about the drawer and most apps, I guess I was decaffeinated because I was wrong for almost all of them.

A NavigationUI.setupWithNavController(Toolbar, NavController, DrawerLayout) method has been added internally and will be available in alpha03