organicmaps/organicmaps-tmp

Archived

Fork 1

[editor]Add new POI in editor #4034

Merged

Jean-BaptisteC merged 1 commit from editor_POI into master

2022-12-08 15:39:34 +00:00

Jean-BaptisteC commented

2022-12-05 20:06:25 +00:00

(Migrated from github.com)

I have again add new type in editor.config

@biodranik I have some questions about this file:

Is it possible to arrange by alphabetical order foreach types for better visibility ?
Order is use in code ? 6600f3fb5f/data/editor.config (L157)
Can i add missing group in types ?
We have missing this :) 6600f3fb5f/data/editor.config (L2)
Do you think it's necessary to include field internet for shop ?

I have again add new type in editor.config @biodranik I have some questions about this file: - Is it possible to arrange by alphabetical order foreach types for better visibility ? - Order is use in code ? https://github.com/organicmaps/organicmaps/blob/6600f3fb5f35d956eb879bf3d7d2226ea0c838aa/data/editor.config#L157 - Can i add missing group in types ? - We have missing this :) https://github.com/organicmaps/organicmaps/blob/6600f3fb5f35d956eb879bf3d7d2226ea0c838aa/data/editor.config#L2 - Do you think it's necessary to include field internet for shop ?

biodranik commented

2022-12-05 22:05:14 +00:00

(Migrated from github.com)

Aren't types displayed sorted already?
Need to review the code to remember the reasoning behind it. It was 2016 ) Can you try to check it yourself?
What missing groups do you have in mind?
Argh, good catch! I think we can safely replace it to organicmaps or omaps.
Some shops have internet. It is VERY useful when you're in roaming or didn't buy a SIM card yet, or exhausted your limits while traveling. And our search easily finds these spots.

1. Aren't types displayed sorted already? 2. Need to review the code to remember the reasoning behind it. It was 2016 ) Can you try to check it yourself? 3. What missing groups do you have in mind? 4. Argh, good catch! I think we can safely replace it to organicmaps or omaps. 5. Some shops have internet. It is VERY useful when you're in roaming or didn't buy a SIM card yet, or exhausted your limits while traveling. And our search easily finds these spots.

biodranik (Migrated from github.com) reviewed 2022-12-05 22:05:27 +00:00

data/editor.config

biodranik (Migrated from github.com) commented

2022-12-05 22:01:29 +00:00

Why are you not adding an internet field? Btw, should it be included in POI by default? What are the exceptions without Internet?

Jean-BaptisteC commented

2022-12-06 16:56:13 +00:00

(Migrated from github.com)

Aren't types displayed sorted already?

Types have alphabetical order in app, but maybe you can sort by key and by values types in xml

2. Need to review the code to remember the reasoning behind it. It was 2016 ) Can you try to check it yourself?

I have find commit 424d4d1457

3. What missing groups do you have in mind?

Some shops have don't shop group in xml (I have check in Wiki)

4. Argh, good catch! I think we can safely replace it to organicmaps or omaps.

Fixed :)

5. Some shops have internet. It is VERY useful when you're in roaming or didn't buy a SIM card yet, or exhausted your limits while traveling. And our search easily finds these spots.

Ok to add in all shop but maybe disable on office and POI about education ?
https://wiki.openstreetmap.org/wiki/Key:internet_access

> 1. Aren't types displayed sorted already? Types have alphabetical order in app, but maybe you can sort by key and by values types in xml > 2. Need to review the code to remember the reasoning behind it. It was 2016 ) Can you try to check it yourself? I have find commit https://git.omaps.dev/organicmaps/organicmaps/commit/424d4d14571dfd154b183cd4820d106344745dd2 > 3. What missing groups do you have in mind? Some shops have don't shop group in xml (I have check in Wiki) - https://wiki.openstreetmap.org/wiki/Tag:shop%3Ddry_cleaning - https://wiki.openstreetmap.org/wiki/Tag:shop%3Dmassage > 4. Argh, good catch! I think we can safely replace it to organicmaps or omaps. Fixed :) > 5. Some shops have internet. It is VERY useful when you're in roaming or didn't buy a SIM card yet, or exhausted your limits while traveling. And our search easily finds these spots. Ok to add in all shop but maybe disable on office and POI about education ? https://wiki.openstreetmap.org/wiki/Key:internet_access

biodranik commented

2022-12-06 22:38:08 +00:00

(Migrated from github.com)

Do You mean sorting types in the editor.config? It is possible partially if types have the same priority. Generic building type, for example, should have the lowest priority.
I meant the code logic behind it. It starts here: 6f67523f87/editor/editor_config.cpp (L109) , you can see where and why is this function used.
I don't see that "shop" group is defined. Was it removed if you check in the git history? Why? How is it different from other groups? Did you get how the config works?
Why do you want to disable internet for offices and education? If they have a free or guest wifi, then it's ok.
Can you please add a separate github check called editor-check.yml that will checkout only the main repo and validate the editor config by running xmllint -noout -schema config.xsd editor.config ?
Please check/fix failing editor tests in checks.

1. Do You mean sorting types in the editor.config? It is possible partially if types have the same priority. Generic building type, for example, should have the lowest priority. 2. I meant the code logic behind it. It starts here: https://github.com/organicmaps/organicmaps/blob/6f67523f87a0c642dc06057a12aa8d6bd0384ece/editor/editor_config.cpp#L109 , you can see where and why is this function used. 3. I don't see that "shop" group is defined. Was it removed if you check in the git history? Why? How is it different from other groups? Did you get how the config works? 4. Why do you want to disable internet for offices and education? If they have a free or guest wifi, then it's ok. 5. Can you please add a separate github check called editor-check.yml that will checkout only the main repo and validate the editor config by running `xmllint -noout -schema config.xsd editor.config` ? 6. Please check/fix failing editor tests in checks.

Jean-BaptisteC commented

2022-12-07 17:25:01 +00:00

(Migrated from github.com)

I have reorder alphabetticaly amenity types and shops types
Add internet field on all shops types
I have add missing groups on somes types
I have try to fixed failed CI by replace mapsme by omaps in all files in editor directory
Of course, but i prefere to do that in separated PR

(I have see wrong indentation in 219 line and unused line in 958 line) :)

- I have reorder alphabetticaly amenity types and shops types - Add internet field on all shops types - I have add missing groups on somes types - I have try to fixed failed CI by replace mapsme by omaps in all files in editor directory - Of course, but i prefere to do that in separated PR (I have see wrong indentation in 219 line and unused line in 958 line) :)

biodranik commented

2022-12-07 18:09:14 +00:00

(Migrated from github.com)

We usually try to separate logical commits. It makes reverting in cases of errors easier. It would be great if sorting commit would be done separately (or maybe even merged separately).

Jean-BaptisteC commented

2022-12-07 20:32:19 +00:00

(Migrated from github.com)

OK, no problem :)
In this PR, i have just add new types in editor (i didn't have doing tests on device).
When i have time, i create a second PR to reorganize editor.config and replace mapsme by omaps.

OK, no problem :) In this PR, i have just add new types in editor (i didn't have doing tests on device). When i have time, i create a second PR to reorganize editor.config and replace mapsme by omaps.

biodranik (Migrated from github.com) approved these changes 2022-12-08 15:39:20 +00:00

biodranik (Migrated from github.com) left a comment

Good. Let's