Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for location plugins #772

Open
wants to merge 111 commits into
base: main
Choose a base branch
from

Conversation

Sir-Photch
Copy link
Sponsor Contributor

Successor to #769.

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented May 5, 2024

This should be reviewable now.

I have added some UI for the departures with mock data in debug mode:

UI demo

With map view:

The LazyColumn has a bounce animation if it is scrollable, to indicate to the user that there are more entries further down.

Screen_recording_20240505_135903.webm

Without map view:

This one doesn't look as good, as there is lots of unused space (I mainly focused on the view that includes the map)

2024-05-05_13:59:38

I am wondering if the background of LineType-Icon and LineName should vary between line-types: blue for busses, red for trains, etc. This might not fit very well with the material theming though. (Maybe blend the colors?)

Right now, scrolling the departures works only if the LocationItem is opened up from the favourites, in case it is shown as a search result, the scrolling is somehow inhibited. (Modifier.consumeAllScrolling() also doesn't do anything here.) Do you have an idea why this could be?

Missing things:

  • Sort departures by departure time (and take into account that when traveling late before midnight, any departures past midnight would need to be displayed after any departures before midnight)
  • Actually write a plugin :b

@Sir-Photch Sir-Photch marked this pull request as ready for review May 5, 2024 12:19
@MM2-0
Copy link
Owner

MM2-0 commented May 5, 2024

This one doesn't look as good, there is lots of unused space (I mainly focused on the view that includes the map)

Swap the destination and departure columns , make destination fill the available space

I am wondering if the background of LineType-Icon and LineName should vary between line-types: blue for busses, red for trains, etc. This might not fit very well with the material theming though. (Maybe blend the colors?)

It would make sense to apply the line colors that the transport provider uses, usually lines are color-coded in some way. And to make them fit in, you can adjust the tone of the color (light mode: 40 for container, 100 for content, dark mode: 80 for container, 20 for content). If that still looks bad, you can try to blend them, like how the WeatherIconColors are slightly adjusted to match the theme.

@MM2-0
Copy link
Owner

MM2-0 commented May 5, 2024

I'm currently trying to find a good design for the places results, but it's just so hard to fit all the information into the constrained space we have.

Which one do you like better, left or right?
Screenshot_20240505_145500

(for other places, opening hours would be listed in place of the departure times)

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented May 5, 2024

Both look nice, I have two ideas:

  • Big variant for LocationItems that were opened from favorites, small one for search results (or vice-versa)
  • Small variant by default, tapping on map animates to larger variant (since you now have a button for navigation)

I'd still display the address though on the larger variant, if the smaller one gets discarded.
Also, where did the direction arrow go? To the user location indicator on the map? I spent quite some time figuring that out :')

What do you think about using icons for the line type, like I have it currently implemented?

And by the way, how do you come up with these mock-ups? Figma?

@MM2-0
Copy link
Owner

MM2-0 commented May 5, 2024

Small variant by default, tapping on map animates to larger variant (since you now have a button for navigation)

I was considering this as well. I think it comes down to the question whether the small map is actually usable. Because if it's not, there is no point in showing it at all.

I'd still display the address though on the larger variant, if the smaller one gets discarded.

Yes. But first I need to figure out how to format addresses properly. Because right now the only supported address format is "[street] [house number]" which isn't used in half of the world.

Also, where did the direction arrow go? To the user location indicator on the map?

Yes, the user indication now shows the direction you're looking at. I think these two information belong together so it makes sense to have them in one place. And it doesn't make a lot of sense two have two spinning arrows for essentially the same information.

I spent quite some time figuring that out :')

It will still be visible in the compact (list) view. And I will bring it back for the mapless variant.

What do you think about using icons for the line type, like I have it currently implemented?

Yeah, makes sense. My mockup has no indication for the type of transport. But I would reduce the spacing by a bit.

And by the way, how do you come up with these mock-ups? Figma?

I use Penpot, which is a Figma alternative that's FOSS and self-hostable.

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented May 5, 2024

I think it comes down to the question whether the small map is actually usable. Because if it's not, there is no point in showing it at all.

You mean not showing a map at all, in any type of location result? I disagree, a map helps to put the location into context. Let's say I am searching for some location and I get two results with the same name. Only a map really helps to distinguish between them.

And even the small map is useful to see "ah, that one is downtown" or "further out". This especially applies to the area where one is from, since I argue after living somewhere for a while you know the map of the city to a certain degree, where a zoomed-out view with both your location and the POI visible tells you a lot.

It will still be visible in the compact (list) view. And I will bring it back for the mapless variant.

Hooray! :)

@MM2-0
Copy link
Owner

MM2-0 commented May 5, 2024

And even the small map is useful to see "ah, that one is downtown" or "further out". This especially applies to the area where one is from, since I argue after living somewhere for a while you know the map of the city to a certain degree, where a zoomed-out view with both your location and the POI visible tells you a lot.

But how small can we make the map until that information is no longer conveyed?

By the way, how did you come up with the list for LocationCategory? Is it an exhaustive list? I'm wondering whether we should change it to string (to allow plugins to put arbitrary texts there) or to expose the enum to the plugin sdk (and not allow any custom values)

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented May 6, 2024

But how small can we make the map until that information is no longer conveyed?

I'd say the small variant you proposed is around that limit.

By the way, how did you come up with the list for LocationCategory? Is it an exhaustive list?

No, not at all. I picked the "most common" from https://taginfo.openstreetmap.org/tags. OSM tags consist of key-value pairs like amentity=fast_food, leasure=sports_hall or key=value1;value2:

private val categoryTags = setOf(
"amenity",
"shop",
"sport", // "sport:soccer"
"railway", // "railway:stop"
"highway", // "highway:bus_stop"
"tourism", // "tourism:museum"
"leisure", // "leisure:fitness_center"
)

"railway" and "highway" are removed in this PR because public transport providers will serve that niche.

I chose an enum because I think handling strings that enumerate on something is cumbersome. Exposing that enum to the SDK sounds good to me, and after all, anyone could draft a quick PR for categories that are missing.

@MM2-0
Copy link
Owner

MM2-0 commented May 6, 2024

No, not at all. I picked the "most common" from https://taginfo.openstreetmap.org/tags. OSM tags consist of key-value pairs like amentity=fast_food, leasure=sports_hall or key=value1;value2:

Do you know if there are some ready-to-use human-readable labels for these tags available somewhere? Maybe I could save my translators some time if I could import these labels from OSM instead of adding and translating them manually.

Edit: OSMAnd seems to have some, maybe I can import them from there: https://github.com/osmandapp/OsmAnd/blob/master/OsmAnd/res/values-de/phrases.xml

@MM2-0
Copy link
Owner

MM2-0 commented Jun 7, 2024

I think the only thing left to do is fix OSM categories? And then this can be merged?

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented Jun 8, 2024

I think the only thing left to do is fix OSM categories? And then this can be merged?

I suppose so.

One more thing though, is the Attribution (supposed to be) used in the UI, or shown to the user in any way? I could use this to display which public transport network the location originated from.

@MM2-0
Copy link
Owner

MM2-0 commented Jun 8, 2024

One more thing though, is the Attribution (supposed to) be used in the UI, or shown to the user in any way?

Oh, true. Some providers legally require this.

@Sir-Photch
Copy link
Sponsor Contributor Author

I think the only thing left to do is fix OSM categories? And then this can be merged?

And, actually, address as well. 🙃
I'll have a look at category though.

@MM2-0
Copy link
Owner

MM2-0 commented Jun 8, 2024

Yet another breaking change in the plugin API. Sorry about that, make sure to update launcher and plugin sdk.

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented Jun 8, 2024

No problem... Actually, one unrelated problem to that: getting a localized string for OsmLocation.category requires a AndroidContext, doesn't it? How do we pass / inject it down to that level?

@MM2-0
Copy link
Owner

MM2-0 commented Jun 8, 2024

Actually, one unrelated problem to that: getting a localized string for OsmLocation.category requires a AndroidContext, doesn't it? How do we pass / inject it down to that level?

LocationsRepository has access to context which you can pass to the constructor of OsmLocationProvider.

Oh, by the way, if you add strings for categories, please add them to poi.xml (in values in the i18n module) and (if possible) copy them from https://github.com/osmandapp/OsmAnd/blob/master/OsmAnd/res/values/phrases.xml. I will later import OsmAnd into my Weblate instance and use Weblate's auto translation feature to synchronize the translations automatically.

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented Jun 8, 2024

I've added some tag matching to categorize the OSM locations. However, there seems to be an issue where osm locations that were already marked as favorite are not updating their icons to the now categorized one...

This seems to alleviate itself when the launcher is restarted (?)

...Aaand I just noticed that you've asked me to copy relevant localization strings over from OsmAnd. Sorry, I will clean them up tomorrow!

@MM2-0
Copy link
Owner

MM2-0 commented Jun 8, 2024

This seems to alleviate itself when the launcher is restarted (?)

They should update when the item is refreshed. So it should fix itself when the user taps an item (?)

...Aaand I just noticed that you've asked me to copy relevant localization strings over from OsmAnd. Sorry, I will clean them up tomorrow!

All good, and thanks for fixing the categories

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented Jun 9, 2024

They should update when the item is refreshed. So it should fix itself when the user taps an item (?)

I can't say that this was the case. But now I cannnot test that anymore because the locations I had marked as favorite in the emulator got replaced (by me). So this could be an issue once this update lands and people already have locations saved?

Actually, this should be testable by building the current release version, marking a location as favorite and then switching back to this branch.

@MM2-0
Copy link
Owner

MM2-0 commented Jun 9, 2024

I can't say that this was the case. But now I cannnot test that anymore because the locations I had marked as favorite in the emulator got replaced (by me). So this could be an issue once this update lands and people already have locations saved?

I just tested it:

  • The category is updated as soon as I long press a pinned location
  • The icon is updated when the launcher is restarted. The reason for that is, that the launcher caches icons, so the change isn't reflected immediately

- run providers in parallel
- flatten code
@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented Jun 9, 2024

The category is updated as soon as I long press a pinned location

Yes, category works fine. But is long pressing now required for refreshing?

Also, I had a look at OsmAnd strings; They use a different naming scheme.
Instead of poi_category_park it is just poi_park. The strings that we already had use the former format. In this case, I wouldn't change it for consistency sake

@MM2-0
Copy link
Owner

MM2-0 commented Jun 9, 2024

Yes, category works fine. But is long pressing now required for refreshing?

Always has been?

Also, I had a look at OsmAnd strings; They use a different naming scheme.
Instead of poi_category_park it is just poi_park. The strings that we already had use the former format. In this case, I wouldn't change it for consistency sake

Alright. Gotta move them back to strings.xml then.

@Sir-Photch
Copy link
Sponsor Contributor Author

Sir-Photch commented Jun 9, 2024

Always has been?

Oh, I thought that locations always refresh themselves if they are opened up and the data is older than (now) 1 minute

I have added some best-effort parsing for OsmLocation.address, based on a library thats based on the one you suggested. The latter uses java that was incompatible with android, which is probably why this kotlin "port" exists.

That being said, it is a unversioned library with version code set to -SNAPSHOT. I'm not sure if you are okay with this? The repo hasn't seen any commits since two years though, so it might be considered "stable": https://github.com/woheller69/AndroidAddressFormatter

@MM2-0
Copy link
Owner

MM2-0 commented Jun 9, 2024

Oh, I thought that locations always refresh themselves if they are opened up and the data is older than (now) 1 minute

Oh yeah, right. That's what I meant. You are right, single tap leads to the same result.

The latter used java that was incompatible with android, which is probably why this kotlin "port" exists.

Like what?

That being said, it is a unversioned library with version code set to -SNAPSHOT. I'm not sure if you are okay with this? The repo hasn't seen any commits since two years though, so it might be considered "stable": https://github.com/woheller69/AndroidAddressFormatter

Better fork it (copy the source code to a new module in libs). To avoid surprises, like breaking API or license changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants