[placepage] Show all types (filtering only POIs) #8481

dvdmrtnz · 2024-06-17T16:18:29Z

dvdmrtnz commented

2024-06-17 16:18:29 +00:00

Retrying to add all types to Place Page. Now it should be better than organicmaps/organicmaps#8238, as it will only show types matched by existing checker IsPoiChecker. This should filter out useless stuff and internal things like psurface-*.

32fa582316/indexer/ftypes_matcher.cpp (L461-L482)

Related to organicmaps/organicmaps#3744
Related to organicmaps/organicmaps#4025 (comment)
Related to organicmaps/organicmaps#6882
Fixes organicmaps/organicmaps#8355

Retrying to add all types to Place Page. Now it should be better than https://git.omaps.dev/organicmaps/organicmaps/pulls/8238, as it will only show types matched by existing checker `IsPoiChecker`. This should filter out useless stuff and internal things like psurface-*. https://github.com/organicmaps/organicmaps/blob/32fa5823167212791b94a35bbfe0c682d99f2d64/indexer/ftypes_matcher.cpp#L461-L482 * Related to https://git.omaps.dev/organicmaps/organicmaps/issues/3744 * Related to https://git.omaps.dev/organicmaps/organicmaps/issues/4025#issuecomment-1807217279 * Related to https://git.omaps.dev/organicmaps/organicmaps/issues/6882 * Fixes https://git.omaps.dev/organicmaps/organicmaps/issues/8355

👍 1 🎉 2

biodranik (Migrated from github.com) reviewed 2024-06-17 17:26:10 +00:00

indexer/map_object.cpp

					
				@ -97,0 +109,4 @@

				    // Ignore types that are not POI

				    // Ignore general types that have no subtype

				    // But always show the first/main type

				    if (!isFirst && (!isPoi(type) || ftype::GetLevel(type) == 1))

biodranik (Migrated from github.com) commented

2024-06-17 17:26:10 +00:00

Do multiple area features also work well? I wonder how they pass IsPoiChecker filter, or is it a not only POI filter?

dvdmrtnz reviewed 2024-06-19 20:07:09 +00:00

indexer/map_object.cpp

					
				@ -97,0 +109,4 @@

				    // Ignore types that are not POI

				    // Ignore general types that have no subtype

				    // But always show the first/main type

				    if (!isFirst && (!isPoi(type) || ftype::GetLevel(type) == 1))

dvdmrtnz commented

2024-06-19 20:07:08 +00:00

Yes, they work fine

https://www.openstreetmap.org/way/1090710626#map=18/41.47902/2.09795

Yes, they work fine https://www.openstreetmap.org/way/1090710626#map=18/41.47902/2.09795 <img src="https://github.com/organicmaps/organicmaps/assets/47610359/18f5997b-596f-4689-885e-fd04c025ec6c" width="320"/>

biodranik (Migrated from github.com) reviewed 2024-06-24 18:14:32 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! A few comments

indexer/ftypes_matcher.cpp

biodranik (Migrated from github.com) commented

2024-06-24 18:08:44 +00:00

  std::string_view constexpr poiTypes[] = {

```suggestion std::string_view constexpr poiTypes[] = { ```

biodranik (Migrated from github.com) commented

2024-06-24 18:09:17 +00:00

Would it work?

    m_types.push_back(classif().GetTypeByPath({"amenity"}));

Would it work? ```suggestion m_types.push_back(classif().GetTypeByPath({"amenity"})); ```

indexer/map_object.cpp

biodranik (Migrated from github.com) commented

2024-06-24 18:11:54 +00:00

Will it work properly by leaving "amenity" where it's the only one type of a feature, and where types are "amenity" + "something" or "something" + "amenity" ?

biodranik (Migrated from github.com) commented

2024-06-24 18:12:34 +00:00

    if (first)
      first = false;
    else
      oss << feature::kFieldsSeparator;

```suggestion if (first) first = false; else oss << feature::kFieldsSeparator; ```

biodranik (Migrated from github.com) commented

2024-06-24 18:13:06 +00:00

  bool isFirst = true;

```suggestion bool isFirst = true; ```

biodranik (Migrated from github.com) commented

2024-06-24 18:14:16 +00:00

    oss << platform::GetLocalizedTypeName(classif().GetReadableObjectName(type));

```suggestion oss << platform::GetLocalizedTypeName(classif().GetReadableObjectName(type)); ```

dvdmrtnz reviewed 2024-06-24 18:41:49 +00:00

indexer/ftypes_matcher.cpp

dvdmrtnz commented

2024-06-24 18:41:49 +00:00

Yes, it works

dvdmrtnz reviewed 2024-06-24 18:52:05 +00:00

indexer/map_object.cpp

dvdmrtnz commented

2024-06-24 18:52:05 +00:00

The first/main type is always shown, and amenity is ignored if it's not the first/main type so:
"amenity" shows "Amenity"
"amenity"+"something" shows "Amenity • Something"
"something"+"amenity" shows "Something"

The first/main type is always shown, and amenity is ignored if it's not the first/main type so: "amenity" shows "Amenity" "amenity"+"something" shows "Amenity • Something" "something"+"amenity" shows "Something"

vng (Migrated from github.com) reviewed 2024-06-27 03:14:07 +00:00

indexer/ftypes_matcher.cpp

vng (Migrated from github.com) commented

2024-06-27 03:08:41 +00:00

Should be 1 /* level */ here.

indexer/ftypes_matcher.hpp

vng (Migrated from github.com) commented

2024-06-27 03:13:42 +00:00

IDK the motivation here, but consider using existing IsPoiChecker or IsAddressObjectChecker

IDK the motivation here, but consider using existing ```IsPoiChecker``` or ```IsAddressObjectChecker```

indexer/map_object.cpp

vng (Migrated from github.com) commented

2024-06-27 03:11:32 +00:00

nit: Put !IsFirst before other cond.

nit: Put ```!IsFirst``` before other cond.

dvdmrtnz reviewed 2024-06-27 16:20:12 +00:00

indexer/ftypes_matcher.cpp

dvdmrtnz commented

2024-06-27 16:20:12 +00:00

I want this checker to only match "amenity" while not matching subtypes like "amenity-bar" or "amenity-restaurant"... I've been trying different things and I've found that using level=2 I can get this behavior. If I change it to level=1 then this checker matches both "amenity" and all subtypes like "amenity-bar" or "amenity-restaurant"...

I want this checker to only match "`amenity`" while not matching subtypes like "`amenity-bar`" or "`amenity-restaurant`"... I've been trying different things and I've found that using level=2 I can get this behavior. If I change it to level=1 then this checker matches both "`amenity`" and all subtypes like "`amenity-bar`" or "`amenity-restaurant`"...

dvdmrtnz reviewed 2024-06-27 16:20:52 +00:00

indexer/ftypes_matcher.hpp

dvdmrtnz commented

2024-06-27 16:20:52 +00:00

The motivation is to filter out general "amenity" types while not removing subtypes like "amenity-bar" or "amenity-restaurant"...

The motivation is to filter out general "`amenity`" types while not removing subtypes like "`amenity-bar`" or "`amenity-restaurant`"...

vng (Migrated from github.com) reviewed 2024-06-27 17:03:10 +00:00

indexer/ftypes_matcher.cpp

vng (Migrated from github.com) commented

2024-06-27 17:01:47 +00:00

Ah, I see, good point.

indexer/map_object.cpp

vng (Migrated from github.com) commented

2024-06-27 17:03:01 +00:00

Better (!isPoint(type) || ftype::GetLevel(type) == 1) and remove confusing IsGeneralAmenityChecker class.

Better ```(!isPoint(type) || ftype::GetLevel(type) == 1)``` and remove confusing IsGeneralAmenityChecker class.

dvdmrtnz reviewed 2024-06-27 17:21:27 +00:00

indexer/map_object.cpp

dvdmrtnz commented

2024-06-27 17:21:27 +00:00

Okay! Makes sense! Thanks!

biodranik (Migrated from github.com) reviewed 2024-06-27 18:37:46 +00:00

map/place_page_info.cpp

biodranik (Migrated from github.com) commented

2024-06-27 18:37:46 +00:00

Are all types appended to "filtered" GetAllLocalizedTypes ? Does it look good?

What's the goal?

Are all types appended to "filtered" GetAllLocalizedTypes ? Does it look good? What's the goal?

dvdmrtnz reviewed 2024-06-27 19:05:26 +00:00

map/place_page_info.cpp

dvdmrtnz commented

2024-06-27 19:05:26 +00:00

I think it's useful when debugging some things to be able to see all the internal types. It's not for users to use, it's mainly for developers. When this debug command is activated, all types are appended at the end of the description in the place page. See organicmaps/organicmaps#8481 (comment)

I think it's useful when debugging some things to be able to see all the internal types. It's not for users to use, it's mainly for developers. When this debug command is activated, all types are appended at the end of the description in the place page. See https://git.omaps.dev/organicmaps/organicmaps/pulls/8481#issuecomment-2195167310

dvdmrtnz reviewed 2024-06-27 19:06:58 +00:00

map/place_page_info.cpp

dvdmrtnz commented

2024-06-27 19:06:58 +00:00

If you guys think it's not useful, I can drop the commit. No problem.

biodranik (Migrated from github.com) reviewed 2024-06-27 19:21:51 +00:00

map/place_page_info.cpp

biodranik (Migrated from github.com) commented

2024-06-27 19:21:51 +00:00

We are using the desktop version for debugging and development, it shows all raw tags. Also, repeating the same but not translated tags doesn't look convenient to read/debug.

Do you use the desktop version?

We are using the desktop version for debugging and development, it shows all raw tags. Also, repeating the same but not translated tags doesn't look convenient to read/debug. Do you use the desktop version?

biodranik (Migrated from github.com) approved these changes 2024-06-27 20:47:41 +00:00

biodranik (Migrated from github.com) left a comment

¡Gracias!

pastk reviewed 2024-06-27 21:01:47 +00:00

map/place_page_info.cpp

pastk commented

2024-06-27 21:01:46 +00:00

Its a useful feature, sometimes its very helpful to debug on a device.
Also power users were asking for it several times.

Its a useful feature, sometimes its very helpful to debug on a device. Also power users were asking for it several times.

biodranik (Migrated from github.com) reviewed 2024-06-27 21:37:58 +00:00

map/place_page_info.cpp

biodranik (Migrated from github.com) commented

2024-06-27 21:37:58 +00:00

At the moment, all useful types should be printed. In general, debugging on a device is an interesting idea, it can be implemented with #ifdef DEBUG to avoid unnecessary overhead in production.

Users were asking for it because they miss/expect to see standard osm tags. It's not the case.

What are your use-cases for debugging on the device?

At the moment, all useful types should be printed. In general, debugging on a device is an interesting idea, it can be implemented with `#ifdef DEBUG` to avoid unnecessary overhead in production. Users were asking for it because they miss/expect to see standard osm tags. It's not the case. What are your use-cases for debugging on the device?

pastk reviewed 2024-06-27 21:52:46 +00:00

map/place_page_info.cpp

pastk commented

2024-06-27 21:52:46 +00:00

E.g. road attribs like surface, hwtags - power users will be able to understand why routing avoids or prefers certain ways.
It'd be possible to check many things on the go without desktop access.
ifdef is not needed - PP is not opened many times a second, there won't be overhead. Actually restricting it to debug version only will kill 90% of convenience and benefits of this feature.

E.g. road attribs like surface, hwtags - power users will be able to understand why routing avoids or prefers certain ways. It'd be possible to check many things on the go without desktop access. `ifdef` is not needed - PP is not opened many times a second, there won't be overhead. Actually restricting it to debug version only will kill 90% of convenience and benefits of this feature.