Kvaesitso icon indicating copy to clipboard operation
Kvaesitso copied to clipboard

Add support for location plugins

Open shtrophic opened this issue 1 year ago • 9 comments

Successor to #769.

shtrophic avatar Apr 21 '24 20:04 shtrophic

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:

  • [x] 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)
  • [x] Actually write a plugin :b

shtrophic avatar May 05 '24 12:05 shtrophic

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 avatar May 05 '24 12:05 MM2-0

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)

MM2-0 avatar May 05 '24 12:05 MM2-0

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?

shtrophic avatar May 05 '24 13:05 shtrophic

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.

MM2-0 avatar May 05 '24 14:05 MM2-0

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! :)

shtrophic avatar May 05 '24 14:05 shtrophic

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)

MM2-0 avatar May 05 '24 15:05 MM2-0

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:

https://github.com/MM2-0/Kvaesitso/blob/3dd7be2df06128647c0c294e77017bcfe2a6cbf1/data/openstreetmaps/src/main/java/de/mm20/launcher2/openstreetmaps/OsmLocation.kt#L71-L79

"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.

shtrophic avatar May 06 '24 05:05 shtrophic

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 avatar May 06 '24 13:05 MM2-0

I have replaced street / houseNumber with a more generic address object, because there are numerous different address formats around the world, and most APIs probably return the address as one formatted string. But apparently, the OSM API only returns street and house number as individual fields and now we need to find a reliable way to format these data back into an address. So far, I have found this.

MM2-0 avatar May 25 '24 22:05 MM2-0

But apparently, the OSM API only returns street and house number as individual fields and now we need to find a reliable way to format these data back into an address. So far, I have found this.

Hrm, we could take the first N lines returned by this library for address{,2,3} and fill the rest via the OSM addr:* tags, that is:

Address.city -> addr:city Address.state -> addr:province or addr:state Address.postalCode -> addr:postcode Address.country -> addr:country

shtrophic avatar May 26 '24 10:05 shtrophic

I think we should split the category field into an (enum) icon field and a (string) category field. I'm currently writing a Foursquare plugin and I feel like a lot of information is lost by coercing the categories into the few available enum values.

For reference, these are the place categories that are available in Foursquare: https://docs.foursquare.com/data-products/docs/categories

And for the icons enum, I think we should get inspired from what is actually available: https://fonts.google.com/icons?icon.size=24&icon.color=%235f6368&icon.category=maps&icon.set=Material+Icons&icon.style=Rounded

MM2-0 avatar May 26 '24 12:05 MM2-0

I think we should split the category field into an (enum) icon field and a (string) category field. I'm currently writing a Foursquare plugin and I feel like a lot of information is lost by coercing the categories into the few available enum values.

How do you plan on localizing these free-form strings in that case? I am referring to the mappings you have added lately for the location type to be shown as text in the search results.

Also, there would need to be some mapping anyway between some known strings and the icon field, if it is supposed to be an enum, right?

shtrophic avatar May 26 '24 14:05 shtrophic

How do you plan on localizing these free-form strings in that case? I am referring to the mappings you have added lately for the location type to be shown as text in the search results.

Send a lang parameter to the plugin, let the plugin worry about it.

Also, there would need to be some mapping anyway between some known strings and the icon field, if it is supposed to be an enum, right?

Why?

MM2-0 avatar May 26 '24 14:05 MM2-0

How do you create the mapping from category to icon then

shtrophic avatar May 26 '24 14:05 shtrophic

How do you create the mapping from category to icon then

I don't, they are two independent fields.

MM2-0 avatar May 26 '24 14:05 MM2-0

So you'd also have this specified by the plugin?

And it has to be done for OSM still. It sounds like a sensible change though, in that case we could also consider cuisine:* for amenity:restaurant to choose a more accurate icon I suppose.

shtrophic avatar May 26 '24 15:05 shtrophic

So you'd also have this specified by the plugin?

Yes

And it has to be done for OSM still. It sounds like a sensible change though, in that case we could also consider cuisine:* for amenity:restaurant to choose a more accurate icon I suppose.

Yes. And for the category labels, I would import the strings from OsmAnd, to save our translators some work.

MM2-0 avatar May 26 '24 19:05 MM2-0

WIP Foursquare plugin: https://github.com/Kvaesitso/Plugin-Foursquare

Overall, most things seem to be working fine. Transmission of opening hours seems to be broken. Some category names are in English for some reason. And of course, I couldn't test departure times yet.

MM2-0 avatar May 26 '24 21:05 MM2-0

WIP Foursquare plugin: https://github.com/Kvaesitso/Plugin-Foursquare

Cool, thanks. I'll use that set up a transportation plugin in the coming days 👍🏼

shtrophic avatar May 27 '24 17:05 shtrophic

Great! Do you know how to setup the development version of the plugin SDK in your local maven repo?

MM2-0 avatar May 27 '24 21:05 MM2-0

Not yet! :b But I figured I could use 1.2.0-SNAPSHOT you've just tagged? Shouldn't be too many commits behind?

shtrophic avatar May 28 '24 15:05 shtrophic

Yes, but then you need to set up the Github Maven repo, which isn't trivial either. Plus you can't fix bugs that you might find.

Here is what you need to do:

  1. Set the plugin SDK version to something that ends with -SNAPSHOT (I have already done this). -SNAPSHOT versions differ from normal versions in that they can be updated without changing the version number.
  2. Run ./gradlew publish in Kvaesitso's root directory. This will build and publish the artifacts to your local maven repo ($HOME/.m2/repository)
  3. Add mavenLocal() to your plugin project repositories.
  4. Add the plugin SDK dependency (use the SNAPSHOT version)

If you need to make changes to the SDK:

  1. Run ./gradlew publish again
  2. In your plugin project, in Android Studio, open the Gradle sidebar (on the right side). Right click your root project and select "Refresh Gradle Dependencies"

MM2-0 avatar May 28 '24 15:05 MM2-0

HERE plugin, that supports both places and departure times: https://github.com/Kvaesitso/Plugin-HERE (TODO: parse opening hours, parse place icons, add weather provider)

MM2-0 avatar Jun 02 '24 16:06 MM2-0

Finally found a use for pinned tags :D

Screenshot_20240602-190421

MM2-0 avatar Jun 02 '24 17:06 MM2-0

WIP (!) public transport plugin: https://github.com/Sir-Photch/KvaesitsoPlugin-PublicTransport

I'm running into an issue where just the id param of get() is not enough to get a Location that has all of its fields populated. Specifically, I can get the updated departures given the id of a location, but nothing else. For a "full" location, I'd either need the label or latitude / longitude of the location that is supposed to be updated via get(). I see two solutions to this problem:

  1. pass more information in GetParams, but this would not really make sense for any other plugins that don't need this additional information
  2. Add another StorageStrategy, similar to Deferred, but indicating that the returned Location of get() may contain incomplete information / only information that is relevant for updating. The non-null fields in that Location may then be used for updating the launchers copy. Maybe StorageStrategy.Updatable? Not sure about that name... Do you have something in mind?

A different idea thats coming to my mind is changing the return-type of get() to something like GetResult<Location>? which can be one of: GetResult.Update, GetResult.Replace or null, indicating that either:

  1. the present copy in the launcher datastore is to be updated with all non-null fields of the encapsulated location
  2. the present copy is to be replaced by the encapsulated location
  3. the copy is to be removed.

That would mean another breaking change to the sdk; But it would get more flexible as well?

shtrophic avatar Jun 05 '24 21:06 shtrophic

Two solutions come to mind:

  1. You can store whatever you want in the ID field, as long as it is unique and stable. So you could use a format like $ID:$LAT:$LON and extract the parameters in get
  2. You could store additional information in an internal database (but that's probably inefficient because the plugin isn't informed about whether an item has been saved to the database so you'd have to store this information about every result the plugin has ever seen)

But I admit, a way to partially update a result could be benefitial.

Add another StorageStrategy, similar to Deferred, but indicating that the returned Location of get() may contain incomplete information / only information that is relevant for updating.

Maybe turn StorageStrategy? into sealed interface with StoreReference and StoreCopy being data objects and data class Deferred(val mergeResults: Boolean = false). I think the bigger challenge is how to type the Location (and File) type correctly. Because we'd need a Location type which has every field nullable, but then, if we use the same type in search, we could produce invalid results (i.e. a location without a label). Maybe that's fine though, the launcher has to check for that anyways and filter out invalid results. And if we use null as "keep the old value", then how do we tell the launcher to unset a value?

A different idea thats coming to my mind is changing the return-type of get() to something like GetResult<Location>? which can be one of: GetResult.Update, GetResult.Replace or null

Maybe add a second method getUpdate? So that it's clear whether partial results are allowed? But that still wouln't solve the null issue. Unless update returns an UpdateResult with a flag mergeResults? And then what? Add a fourth storage strategy that uses getUpdate instead of get?

MM2-0 avatar Jun 05 '24 22:06 MM2-0

Or maybe Deferred should always call getUpdate and StoreReference always calls get

MM2-0 avatar Jun 05 '24 22:06 MM2-0

And if we use null as "keep the old value", then how do we tell the launcher to unset a value?

Indicate not to merge, but to replace the launchers copy.

Maybe that's fine though, the launcher has to check for that anyways and filter out invalid results.

Since the label is what a user searches for, I'd just define a set of "minimum required" fields that need to be populated, and which need to be set for locations returned by search, and make label one of them.

Or maybe Deferred should always call getUpdate and StoreReference always calls get Maybe turn StorageStrategy? into sealed interface with StoreReference and StoreCopy being data objects and data class Deferred(val mergeResults: Boolean = false).

Problem here is that everything returned by get() would be treated as merge?

You can store whatever you want in the ID field, as long as it is unique and stable.

Sure, but the only other keys apart from id are lat/lon and label, and assuming that they are stable is probably not the right thing to do.

I had a look at how to add

sealed interface GetResult<TResult> {
    data class Replace<TResult>(val with: TResult) : GetResult<TResult>
    data class Merge<TResult>(val with: TResult) : GetResult<TResult>
}

and to change get() to

open suspend fun get(id: String, params: GetParams): GetResult<TResult>? = null

where null means "delete from launcher store", and the other two variants (of this "enum") indicate on how the launcher is supposed to behave. Problem is, how to represent this as a cursor? Add another column, like, GetColumns?

shtrophic avatar Jun 06 '24 16:06 shtrophic

I think the cleanest way from an API design point of view would be something like

suspend fun refresh(item: TResult): TResult?

But the content provider APIs aren't really designed for that. I guess we could use https://developer.android.com/reference/android/content/ContentProvider#query(android.net.Uri,%20java.lang.String[],%20android.os.Bundle,%20android.os.CancellationSignal) and pass the existing record in queryArgs, but Bundles have a limit of 512 kiB / 1 MiB (I'm not sure, different sources claim different things), so we'd have to make sure that that limit is never exceeded.

This would solve our nullability issue since we can still enforce not-null values for fields like label, while still giving the opportunity to unset optional values.

And then we have

abstract suspend fun search(query: TQuery, params: SearchParams): List<TResult>

for search, which must always be implemented

open suspend fun get(id: String, params: GetParams): TResult = null

for StorageStrategy.StoreReference, which requires a full record to be returned and

open suspend fun refresh(oldValue: TResult): TResult? = oldValue

for StorageStrategy.Deferred, which can update whatever fields it wants.

And while we are at it, we should make Deferred the default behavior and deprecate StoreCopy because StoreCopy is essentially equivalent to Deferred with the default refresh implementation. And maybe find a new name for StorageStrategy.

MM2-0 avatar Jun 06 '24 17:06 MM2-0