SimonHalvdansson / Harmonic-HN

Modern Android client for Hacker News
https://play.google.com/store/apps/details?id=com.simon.harmonichackernews
Apache License 2.0
613 stars 40 forks source link

Predicitive back support #83

Open AppearamidGuy opened 1 year ago

AppearamidGuy commented 1 year ago

I can't find anything conclusive about predictive back navigation in Android 14, but it is listed under new features in the changelog so maybe it will be enabled by default for everybody (instead of being behind a toggle in developer settings like in Android 13).

Harmonic already opts in into predictive back and has some code in place for it, but doesn't actually implement it properly. I strongly recommend using Android 14 preview for testing instead of 13 as there are significant changes between them. Here's current status for Harmonic: Screen Status Notes
Welcome -
Stories ⚠️ Works outside of search. Search ui doesn't have nice animations and the code can be improved a bit
Comments ⚠️ Done in 6d86b4f32f4b68fa9b5f3a554550eef44133db9d. Might have bugs
Submissions Done in 8661c5299d85122af9e5b444f42ccc626d0021f9
Writing a comment ⚠️ Done in 8661c5299d85122af9e5b444f42ccc626d0021f9. No predictive back animation when displaying confirmation dialog
Settings ⚠️ Done in 8661c5299d85122af9e5b444f42ccc626d0021f9. No predictive back animation after changing a setting that restarts MainActivity
About Done in 8f0dfecb0f3dcf433204baf8cb6af95ba7e23f22

One problem I've found is that navigating back to a translucent activity looks terrible because the activity behind it (i.e. stories screen) is also visible. It works fine with non-translucent activities, but any activity that uses swipeback (so, almost every one) has to be translucent. A workaround that seems to work is to set translucent=false when navigating forward from a translucent activity and set it back to true in onResume.

Regarding animations, there's a new api overrideActivityTransition but I haven't looked into it too much.

Finally, settings screen does interesting stuff with restarting activities which is not the intended purpose of OnBackPressedCallback, so it requires a different approach. But at least (I assume) it's not a frequently visited screen, so supporting predictive back here doesn't feel so important.

SimonHalvdansson commented 1 year ago

Yeah everything definitely needs to be done with Android 14 as you say, up until now I just activated the flag the get the main screen animation on Android 13 with the developer option enabled. Thanks for the API tips also, those will come in handy.

What were you thinking with the ComposeActivity thing? To not have the dialog show at all on Android 14? Because that might be a reasonable solution.

I should note that I am planning to remove the SwipeBack stuff from everything except for CommentActivity in the near future, partially inspired by predictive back which treats this much better. Do you think it should be removed from CommentActivity also? Or is it too engrained in the users by now?

AppearamidGuy commented 1 year ago

What were you thinking with the ComposeActivity thing? To not have the dialog show at all on Android 14? Because that might be a reasonable solution.

No, the dialog is still necessary to explain user why back didn't work. But I hope that it can be shown with some animation when user triggers predictive back.

I should note that I am planning to remove the SwipeBack stuff from everything except for CommentActivity in the near future, partially inspired by predictive back which treats this much better. Do you think it should be removed from CommentActivity also? Or is it too engrained in the users by now?

I'm sure somebody will be very unhappy if you remove it from comments, so no (at least not yet). Though as a user I don't care because I have it disabled. And as a developer I don't like having to support it, but that's just part of the job.

Interestingly enough, Google has its own implementation of SwipeBack that's called SlidingPaneLayout and uses a completely different approach. But it doesn't support either fragments or predictive back out of the box, so while it has its advantages it's not strictly better.

SimonHalvdansson commented 1 year ago

Okay I see, I sure hope it is not too much work to set up the dialog showing animation for ComposeActivity - that could really be an issue.

Also the SlidingPaneLayout thing - I've seen it mentioned for a long time but watched a video on it now that you mentioned it and learned more about it. The fact that it has the dragging thing is really nice - the SwipeBack library that is used now is really shady I think. It would be nice to migrate to it if predictive back worked nice out of the box and whatever the fragment issue is could be worked around...

AppearamidGuy commented 1 year ago

the SwipeBack library that is used now is really shady I think

I agree. The approach used is very clever, but I doubt os developers ever intended it to be used like this.

So I've experimented with SlidingPaneLayout a bit and it works, but not great. The main problem so far is lagginess of comments opening animation. Also, I couldn't get integration with fragments' lifecycles working and I suspect that's the reason for lags.

I'll continue working on this for now, but this might take a while

SimonHalvdansson commented 1 year ago

Okay that is a very big undertaking but could be nice if it worked! For now, I've got the translucent=true thing you recommended working and I am activating/deactivating the OnBackPressed callback dynamically as the app is used to trigger the back animation when it is appropriate. Consequently, I think we now have OK predictive back working throughout the app! Key commits for this were https://github.com/SimonHalvdansson/Harmonic-HN/commit/6d86b4f32f4b68fa9b5f3a554550eef44133db9d, https://github.com/SimonHalvdansson/Harmonic-HN/commit/bd3839f447f341a5dac5431f85aa94417b08c486 and https://github.com/SimonHalvdansson/Harmonic-HN/commit/8661c5299d85122af9e5b444f42ccc626d0021f9.

AppearamidGuy commented 1 year ago

I've realized there's one more problem with SlidingPaneLayout - it doesn't support more than two child views. This means you can't open another story while reading comments and then swipe back to the first story. So I guess you're stuck with SwipeBack for now

SimonHalvdansson commented 1 year ago

Hm yeah I guess that would require starting a new activity which of course we would not want to have transparent background on.

I am sort of envisioning that once people get on Android 14 they'll like the predictive back animation so much that they might disable SwipeBack. As I see it, this is sort of Googles main idea for predictive back as it solves the same problem (peek and see the old thing).

So yeah let's stick with SwipeBack if there's no easy alternative.

AppearamidGuy commented 12 months ago

I think there's a bug in comments, there's no animation when webview can go back and "use device back for webview"=false

Also I've updated current status in the OP. Great job getting everything to work!

SimonHalvdansson commented 12 months ago

Yes good catch, fairly sure it is fixed by https://github.com/SimonHalvdansson/Harmonic-HN/commit/757ece7ce3dc472bc56f4863235ab049cf461ece

SimonHalvdansson commented 10 months ago

I sort of feel with the latest 2.0 update that things are as good as they're going to get for a while. Until Android starts supporting predictive back for dialogs I am not going to be attempting this. The only possible improvement is Search but this comes for free once we transition to SearchBar or whatever the name of the material component is. As such, I am tempted to close this issue and leave the search improvements to https://github.com/SimonHalvdansson/Harmonic-HN/issues/18

carstenhag commented 9 months ago

Until Android starts supporting predictive back for dialogs I am not going to be attempting this

Maybe I am understanding Google's guidance or your comment wrong, but afaik in this case it is expected that there is no preview/animation.
In other words: Only when some navigation event should occur, there should be a preview animation. A dialog being shown does not fall under this.

SimonHalvdansson commented 9 months ago

Until Android starts supporting predictive back for dialogs I am not going to be attempting this

Maybe I am understanding Google's guidance or your comment wrong, but afaik in this case it is expected that there is no preview/animation.
In other words: Only when some navigation event should occur, there should be a preview animation. A dialog being shown does not fall under this.

This feels reasonable, especially as Google has not implemented such as animation anywhere themselves. I guess the only thing missing to mark this issue as fully completed would be the SearchView rework.