[android] convert place page to fragment #4174
Labels
No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
No milestone
No project
No assignees
3 participants
No due date set.
Dependencies
No dependencies set.
Reference: organicmaps/organicmaps-tmp#4174
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "pp-refactor"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Hello and happy new year (well nearly)! I am back with some more Android refactoring.
This PR aims to make the place page handling easier by moving the content and the buttons in their own fragments.
As a result, the place page view and buttons have less coupling with the rest of the app, making it easier to debug and maintain. Fragments are independent and have their own lifecycle methods. Interaction with those fragments is done using a ViewModel storing the pp buttons and map object to show. This viewmodel could be reused by other UI elements (such as the fullscreen compass view) if those were converted to fragment as well.
Thanks to this change, the app could gain performance/battery improvements as these UI elements are only rendered when needed.
The only UX change is concerning the place page buttons: I removed the phone number button as I always felt it was unnecessary and made buttons placement inconsistent (see #3157). I also removed some logic to change the button order depending on some complexe but IMO useless condition. Now the buttons placement is a lot easier to predict and understand.
The
PlacePageButtonsclass has been fully rewritten. ThePlacePageControllerCompositestuff has been fully removed as it added useless complexity. Instead thePlacePageControllerhandles directly the place page. ThePlacePageViewhas only been converted to a fragment without a full rewrite as it is way too complexe. I'll still try to simplify it when continuing work on this PR. I also want to convert the fullscreen compass view to a fragment so it uses the same viewmodel as the place page.This is still a work in progress as I still have some unanswered questions.
I'll try to find answers to those questions but in the meantime, feel free to help me test this PR and give your feedback!
Thank you, Arnaud! That's a great NY gift )
I and @rtsisyk comment later on your questions. At the moment the most important issue to fix is a lazy WebView initialization. We have a lot of strange crashes now when WebView is initialized at the app startup.
Regarding the recreation performance on POI selection, it would be easier/better to test on some low end device. I have Android 5 for example.
Regarding the webview, I can look into it and maybe move it to its own fragment so it is only initialized when needed.
For the performance, I do not have low end devices at hand, but I can try to compare the the master branch to see if we have any noticeable difference using the emulator.
Thanks, i have started to test on my Pixel 6:
Examples:
I can doing video if necessary
Happy new year 🎉
I can't seem to reproduce your issue, could you share a video please?
https://user-images.githubusercontent.com/87148630/210180010-6ad25c98-b5b1-4a0b-b462-e922d2797d04.mp4
Other thing to reproduce, you need to have no POI selected, when you select POI
Oh yes this bug I noticed and I am working on a fix. The bottom sheet remembers the last peek height from the previous content when opening. So if you open then close the place page for POIs with different preview sizes, you will see this bug. I have to find a way to pass the new peek height before it opens.
@Jean-BaptisteC Can you please confirm my latest commit fixes the issue on your device as well?
Your correction is good, problem is fixed
But i have detect a crash :)
To reproduce i search
Gare de Metz(building=train_station, not POI public_station) and select velo routing from my position hereI am not able to reproduce with all objects in maps, you can try with most far hotels from your positon (use category hotel) and select pedestrian routing
Your crash does not seem to be related to be related to this PR, can you reproduce this on the master branch or with the version in the stores? I can't reproduce it.
Updated the main post with my answers to the questions. Please tell me what you think.
The very first tap on the PP is visibly slower now than on the master branch. If PP is cached anyway after the first click, then we should consider caching PP like it was done on the master branch for a smoother first-click experience. OM should be fast ;-)
Another issue (not a critical one) is that for some reason any subsequent tap on the POI does not work instantly, there is a (small) delay. It would be great to profile it and make the real-time reaction feeling, if it is possible, of course.
I moved the bookmark section in its own fragment and only add it when the selected POI is bookmarked (not committed yet, I need to check for regressions). Selecting POIs is now a lot faster, even for the first click. But the first click on a bookmark sees a little hang. My guess is Webview is slowing everything. I can try to dynamically add the webview component only when the bookmark has html content. This way only bookmarks with HTML would see this first click slowdown. Would that be acceptable?
This is exactly what I had in mind! There is some method to detect if the text in the bookmark is HTML or not, so the real check should be something like this:
Maybe even some simple HTML can be rendered in the TextView (e.g. HTML with only bold/italic/headers/paragraphs, without tables, images etc.)
Also, what about Wiki articles? Are they rendered into the TextView or WebView?
And if you refactor descriptions, then don't forget about the need to display:
We discussed it somewhere already, right?
I managed to get the conditional loading of the webview working, and indeed the place page is faster, but you can see the webview loading when opening a place page with an html description.
From what I could see, the Wikipedia article uses a simple text view.
I face an annoying issue: the animation when the peek view changes is not always played when switching from a bookmark to a regular feature.
And for the description, this is referenced in organicmaps/organicmaps#1953. This refactoring should make it easier to implement. I'm also working on a fix for organicmaps/organicmaps#2825.
Cool, you're the refactoring guru, Arnaud!
Slower/visible webview for bookmarks can be handled later/differently. For example, like this:
I think I finally found away to make the place page fast with smooth animations. But my solution will involve some UX changes!
TL;DR: Place page uses the half extend state for bookmarks with descriptions and wiki articles to show the extended preview. To use this I had to set the bottom sheet to not fit the content, so now it can always be extended fullscreen.
vvv Video at the end vvv
I had issues with the peek height not playing the resize animation when the content height changed because of the webview loading, fragment being added and other unknown factors annoying to debug. I won't get into details but each time I thought I fixed the issue, I found a new bug with it. One example of this was when a POI with a small HTML bookmark was opened, the place page was not tall enough to reach the extended peek height, so when the content finished loading it would snap to the correct position because it would be tall enough.
To make the place page behave in a more predictable way and fix all those bugs, I introduced a new state: half expanded. This state is already built in the standard bottom sheet component, but was not used here because the place page was set to automatically fit to the content. So now the place page can always be extended (fit to content = false), even if there is not enough data to fit the screen in order to enable this state.
The peek height now always follows the preview height no matter the POI type (before it would be set to an extended value for wiki and bookmarks). If the place page must be opened with a bigger peek height (wiki or bookmarks), then it is opened directly to the half expanded state. As the place page does not fit the content anymore, it will always be able to reach this height even if there is not enough content, and if the content changes the place page will not move. From this state, the user can then expand or collapse the sheet. The half expanded state can only be reached when first opening the POI, a user cannot go back to this state by dragging the place page. I don't thing this is an issue as if the user want to have the sheet opened and move on the map, the collapsed state is better.
Pros:
Cons:
This is still work in progress but I'm quite satisfied with how it turned out, please tell me what you think!
https://user-images.githubusercontent.com/80701113/211664050-3ca03d9b-892c-46ce-b601-a4db372e7542.mp4
That's really great !!!!
Just some commentaries:
Thanks for the valuable feedback @Jean-BaptisteC! I added more padding to the place page so it is farther away from the status bar when fullscreen, and the place page should not collapse to its initial state when moving between screens or editing the POI.
Please tell me if your issues are not properly fixed or you find something else!
I've tested this branch and found that the full-screen Place Page looks strange (especially for simple POIs without any additional info, a lot of empty space), and is less convenient to close/use (previously you can tap on the map to close). I'm not sure at the moment that it is the right way to go.
Other minor issues:
To make the place page look less empty, we could add information about OSM and guide the user to add more information. This information could be displayed as text or using an image. For the closing issue, I don't think that's a big deal, I never was able to close the place page by clicking elsewhere on the map unless I was in the middle of nowhere. Users can still do a simple swipe down to close it.
I plan an reworking the description stuff to better handle all the cases in #1953.
Good idea, will work on that when refactoring descriptions
One of the main issues with that "unnecessary" full-screen PP is that it's way harder to press "Edit Place", coordinate, or some other button when it's in the upper part of the screen. Is it really a required UX change? Was the problem mostly in web views with dynamic/unpredicted height? Let's brainstorm on other ways to fix it.
Will showing the PP on the bookmark/wiki (later also on OSM description/wikidata photo) expanded to some fixed height help to fix the issue?
P.S. Now there is a small visual delay when PP is hidden, because the bottom buttons are not animated down, like the PP. Is it hard to implement?
The place page is already expanded to a fixed height when a bookmark note or wikipedia article are available. But in some cases, this height is bigger than the actual place page, and the late loading of the webview may change this height and you would see a snap. Another issue I had was that adding a fragment to the bottom sheet would cancel the peek height change animation (so using the half extended state would avoid that limitation).
Here are my ideas to fix those 2 issues without relying on a fullscreen place page:
TransitionManageranimations to animate the peek height instead of relying on theanimateparameter of thesetPeekHeightfunction. I still have to experiment with it but it seems to play the animation properly compared to theanimateparameter. Or we could use a value animator to gradually change the peek height. I'll try to see which solution works best.I thought about animating the buttons to follow the place page when it moves out of screen. I don't think it would be that hard to implement, I might look into it when I finished reworking the place page view.
After some experimenting, the best way I found to properly animate the peek height is to use a value animator and set the peek height in its callback. Thanks to this I was able to use the peek height for bookmarks and Wikipedia instead of the half expanded state. So the UX is back to what it was, with the place page adapting to the content's height.
Something is still missing though: the place page will collapse when opening the editor and going back, or saving a bookmark. I'm still looking at how to fix this but it might be tricky as the c++ code is calling the same "open the place page" function whether we exit the editor, save a bookmark or click on a place on the map.
Ok I think I found a way to stop the place page from collapsing. Now it collapses only when the map object in the place page changes. The comparison sees a POI and its bookmarked version as different objects, so the place page will still collapse.
Please test and tell me what you think now!
Awesome! Let's test it. We can help with the C++ part if necessary, just ask what is needed.
It works ok on my device. Let's beta-test it )
Other issues to fix in PP (maybe separate issues should be created):
I think I got back to the initial behavior, so unless you want it to be changed, I think we are good.
This was increased to make the title farther away from the status bar when the PP is fullscreen. I don't really know how to fix both issues at the same time.
I never noticed it, nothing in the code changes that and it uses Android's native components.
The current behavior when tapping on the PP preview is to toggle expanded/collapsed state. Do you want to make it always hide the PP, or expand when collapsed and hide when expanded?
Skipping the collapsed state when swiping down after first expanding the PP is easy to do and will be added in the next commit.
PP rarely goes into a full-screen. What about
if (fullscreen) then margin += statusbar_height? Or even better, hide the status bar?Try to expand PP and slightly drag it down. Then drag a bit more and more.
The latter: expand when collapsed and hide when expanded.
I'll experiment with it and see the best results.
Could you record a video please? I'm not sure I understand, everything works well on my end.
Done! Please test and let me know what you think.
I pushed a new commit to make the peek height change animation smoother, especially when the place page is expanded and you select a new POI with a place page content smaller/bigger. This prevents the place page from jumping to a bigger/smaller height and then playing the animation.
It is not perfect, there are some edge cases I did not take into account (for example when you grab the sheet when it is resizing), I'll try to refine it but please give it a go and tell me your feedback.
This is one of the reasons I wanted to make the place page always full height. Handling those kind of issues is so complicated, I'm not even sure I'll be able to have a fully working fix. @biodranik I really think we should investigate the possibility of having the place page always full height, this makes everything so much easier and smoother. If closing the sheet is an issue for you, then we can make it so that pressing in an empty area of the sheet closes it. If it looks weird to you, I'm sure that we can find some info about OSM to fill the area.
@ -147,2 +145,4 @@// if (mElevationInfo != null)// render(mElevationInfo);}What is this elevation info?
?
This check should be done before calling initWebView() (it actually already exists there).
Let's crash here and fix it?
nit: final here and in other places.
Activity a = requireActivity() ?
When it can be null? Does crashing here help to debug wrong states?
When it can be null? Does crashing here help to fix wrong code?
Tested this PR.
Is it possible to show somehow that PP is swipe-able up (expandable).
Here is the initial states for me:
But there is enough room to show full PP:
If the POI has wiki page, this initial state is like this:
This sounds ok to me. Consistent behaviour is better, IMHO. I personally never needed this "Call" button.
I would be surprised if we found another place where the place page can be re-used, but it never hurts to reduce the code coupling.
Probably, but not nessasry. Let's finish with the current change first.
Let's eat an elephant one bite at a time.
All complex layouts in the original implementation that are created every time users starts the app - this is definitely too slow.
This is perfect! Previously, the app crashed right on the first start, and we probably lost countless potential users who intentionally or not intentionally broken their system WebView.
I am still reviewing the code itself and trying to comprehend all changes. The PR is already too big to fit into the cache memory. Please don't start any new refactoring in this area until the current changeset is fully polished and merged. Don't spread yourself too thin. :)
I found a subtle different in old and new implementation:
https://user-images.githubusercontent.com/1799054/213907880-51a0712d-6f74-49bc-95af-673455953db1.mp4
https://user-images.githubusercontent.com/1799054/213907887-b531a304-2784-4471-9723-240c91ec963a.mp4
I also don't understand what is "description header". Could you please add a screenshot?
updateBookmarkDetails()also checks forif (mWvBookmarkNote != null)@ -147,2 +145,4 @@// if (mElevationInfo != null)// render(mElevationInfo);}I don't really know, but it is currently unused and linked to organicmaps/organicmaps#2829
Currently when a bookmark has a note or when a Wikipedia article is available, a grey rectangle with "Description" in it is shown above. With the current state of the refactoring, this header is not shown anymore for bookmark notes. This comment was a reminder for me of this change. I don't think bringing back this header is a good idea, but we should think of a better solution. For now I put the edit bookmark above the notes so we know the notes are linked with the bookmark.
I'll look more into this, but when the place page is closed, the mapobject is set to null. The fragment should be destroyed but to make sure I added this check. I can try to remove it and see if it works.
Thanks everyone for the feedback!
@biodranik I'll rework the action buttons to take into account your feedback.
@vng do you have any idea on how to make the user more aware they can swipe up the place page? The current behavior has been there for quite some time already.
@rtsisyk this behavior was requested by organicmaps/organicmaps#4174 (comment)
@biodranik I fixed the
add stopbutton always in themoremenu, and I tried to implement this:But to make it work, I need the function
Framework.nativeCouldAddIntermediatePoint()to return true in the case you describe? For now it only returns true when a route is calculated.When the place page is closed, the mapObject is set to null. The controller detects when this value becomes null and destroys the fragments. But this means the fragments themselves could receive the event that the mapObject became null.
One way to fix this would be to destroy the fragments and then set the value to null, or never set it to null.
Does it mean that the "description" header is displayed now only for Wiki articles? Let's remove it completely. It occupies an important PP space.
That would make it hard for users to predict where the place page will stop when clicking on a POI. Would it be better to always make the place page initially open to the same height, no matter the content? The current preview does not give much more information than what is already on the map so you often have to expand the place page to see useful information.
Is it possible to unsubscribe first, before setting a null map object?
If it's too complex to fix now, then please add your comment above to the code.
And I think there is a lot of empty space between grey line and first bold text.
This was to avoid the title from being too close to the statusbar when expanding fullscreen. I'll try to find a better way to fix that.
Probably same height is ok, but with check on maximum content size. To avoid empty white space top or bottom.
All my comments are for discussion and may be controversial. Please, don't hurry to implement :)
It shouldn't be critical.
A simple trick is used on iOS. Instead of a horizontal line, when PP can be expanded, a > rotated to 90 degrees left is displayed. When PP can be closed, a > rotated 90 degrees right is displayed. These symbols have very small heights and big widths to take up less vertical space.
We need to rethink it carefully. After saving some important vertical space, we can try to expand some important info, e.g. up to 2 or 3 useful rows, while ignoring coordinates and edit buttons. Let's leave it for the next steps.
Showing a predictable height has it's advantages if it is consistent for specific conditions: e.g. only all features without wiki/bookmark description/osm description are not expanded, etc.
I mentioned it before. We need to find a way to fix it to avoid overlapping with the system toolbar.
Let's leave this change for the next PR. It will need some testing. I would suggest to create a checklist issue with all we mentioned here.
Let's focus on testing and merging this PR, it is important for the upcoming release (actually, it blocks the release).
The previous android version had a similar thing. I removed it in earlier refactoring (when I migrated the place page to Android's bottom sheet implementation) to make it more consistent with other bottom sheet and because it was ugly and blurry. I haven't seen many apps with arrows as bottom sheet handles, so I don't know if that would help the user much. IMO the users are more used to seeing a flat handle meaning they can grab and swipe it (see https://m3.material.io/components/bottom-sheets/overview). Maybe a small text saying "v expand to see more v" at the bottom of the preview would help?
How about preventing the place page to expand fullscreen so it stops slightly before the statusbar?
I also noticed an issue, if you expand the place page, then select a new item which content is exactly the same height (eg: two address nodes), then the place page will stay expanded and not collapse. EDIT: this is now fixed
I fixed all the issues I could find with the refactoring, so I'll start working on the description. I'll refer to organicmaps/organicmaps#1953 for design ideas.
I was also wondering if it would be better to move large or html bookmark descriptions in a new screen/activity the user can access with button like the "more" button on a wiki summary. In case imported bookmark notes are very large, it could be hard to reach useful information displayed after.
it'd be good to keep image display (organicmaps/organicmaps#3171 organicmaps/organicmaps#2152 organicmaps/organicmaps#3252) in mind for refactoring too :)
Those kind of features are why I think we should try and make the place page always take the whole height. Content loading after the page is shown will change the place page size, and we will need to handle animating between the different sizes with the current implementation.
From beta tester: When starting pedestrian navigation.
Could not reproduce the crash, is it possible to get more information on how it was triggered?
I moved the Wikipedia section to its own fragment, so now I can start working on the OSM description.
Same crash with identical call stack (Pixel 6, android 13): "OM beta crashes when clicking on a poi and then rotating the phone."
Thanks, will investigate.
I also tried to add the OSM description but I don't understand how to retrieve it. I tried using
mMapObject.getMetadata(Metadata.MetadataType.FMD_DESCRIPTION);but the resulting string is always empty, even on nodes I know have a description. Is there another way to get it?map_object.cpp, ForEachMetadataReadable
If you want to add description, we should invent some way to pass StringUtf8Multilang into Java. Leave it for separate PR.
+1 for the separate PR.
Let's think about the interface to query descriptions. Do we need them in all languages at once? Maybe there should be API like this:
Most apps are now using the flat handle, Apple maps (and OM for iOS) also draw an X button on the top right. Let's discuss it later separately.
I don't think that it will work with longer Place Pages. It is logical to drag/scroll everything, not just some content view.
We can always pre-calculate max image heights and fit images there. Always full-screen PP makes UX worse if it's almost empty, so, unfortunately, it is not an option. Let's think what can we do in a different way to simplify the implementation.
@vng @biodranik the reported crashes should be fixed.
I'll continue with the refactoring by moving more parts of the place page in different fragments to make it more manageable.
Should we use the same logic as for the title? Device language first then fallback to local language.
We can set the max height of the bottom sheet to a value smaller than the screen height. When it reaches this value, then the user can scroll the content. But yeah it might not be the best UX wise.
Why is PP subscribed to location updates at this moment? Wouldn't it be wiser/better/faster/safer to subscribe to location updates when MapObject is set, and unsubscribe when it's cleared?
ditto
We subscribe to location updates when the fragment is created, at the same time we subscribe to map object updates. But it seems that after an orientation change, the location is updated before the map object. I tried moving the subscription later in the lifecycle but it changed nothing.
A solution would be to subscribe to location change when the map object gets a value for the first time.
Found a better way, I simply initialize the mMapObjet value in onCreateView before registering for the location events
👍 then these checks are not necessary and can be removed, right?
I have doing some tests, when i consult POI (without lot of informations), i open editor, i go back on place page -> location button is hide by place page and buttons zoom are down
I have detect crash but i don't know if it's caused by your rework (i don't able to reproduce on latest release):
The latest public release doesn't have these changes. They were only included in the beta version.
@rtsisyk @vng @arnaudvergnet @Jean-BaptisteC let's focus on testing and polishing this PR, it is very important to be appropriately finished, without introducing any new bugs.
This should now be fixed
Damn fine testing, thanks a lot! I was able to reproduce I'll work on a fix
I'll need some help on this one. The error comes from JNI but I do not have much experience with it.
It occurs when calling the
OnClickfrommDownloadClickListenerinPlacePageView. I don't really know why my changes make this crash.Wow, thanks for the details. So now PP is recreated every time, right? Compared to the master branch, where it is created only once, correct?
I think there are some global variables in JNI that assume that PP is created only once, or not recreated on every new PP, or wrongly cleaned on PP destroy.
I, @vng, or @rtsisyk will take a look. @Jean-BaptisteC you're the God of the QA, thanks! :)
I tried starting and canceling the download from the dialog shown on the map, and it works every time. But as soon as I open the place page, clicking on the same button in the dialog (which worked before) now crashes the app.
Right now the PP is destroyed when hidden, and recreated when shown. But I don't understand how that would be an issue.
Here is a short example. Even when closing the PP, the button crashes.
https://user-images.githubusercontent.com/80701113/215294148-c5530b31-09af-4bd6-b74a-e69e7b6c76fb.mp4
@arnaudvergnet On the device I have this error:
And
02-02 18:18:25.200 30175 30175 F ganicmaps.debu: runtime.cc:598] JNI DETECTED ERROR IN APPLICATION: JNI NewObjectV called with pending exception java.lang.IllegalStateException: Fragment PlacePageView{9d2b38f} (5086c6c7-c27c-4547-a594-a2e8249b8f4f) not attached to a context.It looks like the downloader code does not work now as before.
Internally it is a complex jni/C++ implementation that downloads map files from C++, but notifies the subscribed Place Page by calling callbacks. Maybe these callbacks are incorrectly handled now.
What did you do to get this error?
Selected any object on the non-downloaded area, closed PP, selected it again, and pressed Download button in the PP.
LGTM! Thank you, Arnaud!
I've found another minor but annoying issue, but I'm not sure if it is caused by your changes:
It is very hard to tap the "more..." button for a partially visible Wikipedia article in PP. I often miss it, and the PP closes. Would it be possible to make the whole Wikipedia article view clickable too?