-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Introduce back stack, simplify WelcomeActivity and fix a few issues #396
base: master
Are you sure you want to change the base?
Conversation
Thanks a lot for the PR! I like that you explained everything in detail, and I'm also a fan of That said, I'm not sure when I'll have time to read through all of this, verify the changes and test everything thoroughly. My current (technical) priorities are fixes for Nougat and porting to Oreo. Regarding back stack, if I understand you correctly, it would be like this: If you navigate from Framework to Logs to Modules and press Back, you would first go back to the Logs section, then back to the Framework section. Right? I think I had that behavior in an older version and it caused endless back navigation. Also Google seems to recommend against that:
https://developer.android.com/training/implementing-navigation/temporal.html#back-fragments
https://developer.android.com/design/patterns/navigation.html That also matches my preferences as a user: Back should be used when diving into the hierarchy (e.g. Downloads => click on a module), but that's not the case for same-level sections like Logs and Modules. You could argue that Framework is the entry point of the app and therefore one level above all the others, but not sure if that would be better. Google itself isn't entirly consistent consistent, Play Store doesn't add individual categories (tabs) to the back stack, but it always takes you back through the start page. YouTube on the other hand navigates you back through the tabs you selected. So... I'm really not sure about this, but I feel that the backstack actually makes things worse. |
Yes, that's how it works. I tested it and I can assure you that it doesn't cause endless loops ;) If it did in a previous version, it was wrongly implemented.
As you mentioned, a back stack is unrecommended for horizontal navigation such as tabs, swipeable horizontal views such as ViewPagers and dropdown selections. The navigation drawer is vertical navigation. Navigation drawer, tabs and bottom bars are all different navigational patterns.
To pick up your example, as you noticed correctly, the horizontal tabs in "My apps & games" (Updates, Installed, Library, Beta) in the Play Store aren't added to the back stack, as recommended by the guidelines for horizontal navigation and tabs. But the elements from the navigation drawer (My apps & games, Home, Game, Movies & TV, etc.) are added to the back stack. As for the YouTube app, it uses a bottom bar, which isn't the same as tabs and was only recently added to the Material design guidelines. But in this case, they do in fact break their own guidelines, as bottom navigation shouldn't provide a back stack. For the navigation drawer, it's the expected behavior that you click back and it doesn't quit the app. It should at the very minimum take you back to the home screen first. I'm looking forward to your feedback when you have more time :) |
I just found a bug and haven't yet tested if it was caused by my changes. For the moment, do not merge. |
The first commit fixes the small padding bar that can be seen at the bottom of the Framework screen as soon as the scroll container becomes active (https://goo.gl/XFroLJ). E.g. on smaller phones or in landscape view.
The second commit replaces the ActionBarDrawerToggle with a static hamburger icon. ActionBarDrawerToggle isn't needed anymore, as the arrow animation it provides (https://goo.gl/5iZ9fG) has been replaced in favor of a full-height navigation drawer (https://goo.gl/Wr1xKD). The animation which would get covered up by the full-height drawer is currently disabled anyway, but I thought it would be cleaner this way. One class less to include or something. Eh...
The 3rd commit is the biggest and was the main task I seeked out to fix. The other issues just popped up on the way and I put the navigation-unrelated ones into seperate commits later on, using
git add -p
😅.Basically, the Xposed Installer doesn't have a back stack at the moment. When you open the app and navigate to other categories through the navigation drawer, the app closes when you press back, instead of going to the previous category like it should.
While adding the back stack (basically just using
.addToBackStack(null)
on all Fragment transactions expect the first one) and fixing the bugs that popped up afterwards (title and navigation item selection didn't refresh when you went back), I also noticed that there was a bunch of unneeded, strange code that had to do with states. It seems like the person who wrote it was unaware that the NavigationView and DrawerLayout which are used in the app already handle most of this. So the current app selects menu items when you click them (done automatically), restores their checked state on orientation changes (done automatically) and reopens the navigation drawer if it's open during orientation changes (done automatically).It also keeps a history of the previously selected menu item, to reselect it if you open an Activity (Settings, Support, About) instead of a Fragment (Framework, Modules, Download, Logs) through the navigation menu. This is because else, if you went back after opening an Activity, the Settings, Support or About category would be selected instead of the shown fragment view. Well, turns out you could also return false in the
onNavigationItemSelected
callback method and the menu entry wouldn't get selected in the first place, so you wouldn't have to fix the issue manually later on.Something else I didn't like were the
constructs which were scattered in the Activity. This support code waits 250 ms (until the navigation drawer is slid in), before it starts loading the fragment view. Without this, the sliding in of the navigation drawer would drop frames on many devices, as too much work is done on the main UI thread at the same time. I replaced those constructs by adding the attributes
fragmentToOpen
andactivityToOpen
and theonDrawerClosed
callback. This way, we don't need the uglypostDelayed(new Runnable...
's, as the fragment loading starts at the exact moment the drawer is closed, instead of a hardcoded ms value.One potential downside of this approach is that the 250 ms which were used before didn't quite match the full animation duration, so the loading started a bit earlier (probably at the cost of a few dropped frames). Since it now does match the duration exactly (using the animation speed the user or manufacturer has configured), the category switching might feel a bit slower. I removed the small fade animation to combat this a bit. I think this way is the cleaner approach, but I'd understand if you didn't want to merge it. The faster loading is only a byproduct of not using the correct duration though, so you could also remove the blocking code altogether. This way or the other, you should remove the redundant code that is currently in place (menu item history, duplicate restoring).
The last commit just changes the color of the currently black cloud icons in the Framework screen to the same grey that is also used for the other icons on the same screen. This way, it looks good with all themes.
This PR was done for the #Hacktoberfest. The things you do for a free T-Shirt...