[editor] Building levels support in the core. by biodranik · Pull Request #2670 · mapsme/omim

biodranik · 2016-03-31T16:44:17Z

No description provided.

biodranik · 2016-03-31T16:44:28Z

DUDE JTTP

Zverik · 2016-03-31T16:56:40Z

data/editor.config

То есть, решили не мёржить всё подряд?

biodranik · 2016-04-01T11:15:03Z

@Zverik PTAL DUDE JTTP

vng · 2016-04-01T11:26:44Z

base/string_utils.cpp

Зачем ssize_t, зачем static_cast ??
Лучше size_t i = 2; i < size(); { внутри писать i-2, i-1, i }

for (size_t i = 0; i + 2 < utf8.size(); ++i)
Заодно s/utf8String/utf8/

biodranik · 2016-04-01T12:47:40Z

@Zverik @vng @mpimenov поправил все замечания, PTAL DUDE JTTP

Zverik · 2016-04-01T13:02:40Z

editor/editor_config.cpp

Вот эта строчка зачем? Она не сломает какой-то важный список в приложении?

Эта строчка убирает заматчившийся building из вектора всех типов фичи, чтобы по нему зря не бегать дальше в циклах.

On Apr 1, 2016, at 16:02, Ilya Zverev notifications@github.com wrote:

In editor/editor_config.cpp #2670 (comment):

TypeAggregatedDescription & outDesc) const

{

bool isBuilding = false;

for (auto it = classificatorTypes.begin(); it != classificatorTypes.end(); ++it)

{

if (*it == "building")

{

outDesc.m_address = isBuilding = true;

outDesc.m_editableFields.push_back(feature::Metadata::FMD_BUILDING_LEVELS);

outDesc.m_editableFields.push_back(feature::Metadata::FMD_POSTCODE);

classificatorTypes.erase(it);
Вот эта строчка зачем? Она не сломает какой-то важный список в приложении?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub https://github.com/mapsme/omim/pull/2670/files/af10e69d522742081c4007f6da63a022705f438b#r58201922

Это не повлияет на отображение зданий?

Нет, это временный вектор, который создается перед вызовом этой функции только с одной целью - передать в нее список типов фичи.

On Apr 1, 2016, at 16:51, Ilya Zverev notifications@github.com wrote:

In editor/editor_config.cpp #2670 (comment):

TypeAggregatedDescription & outDesc) const

{

bool isBuilding = false;

for (auto it = classificatorTypes.begin(); it != classificatorTypes.end(); ++it)

{

if (*it == "building")

{

outDesc.m_address = isBuilding = true;

outDesc.m_editableFields.push_back(feature::Metadata::FMD_BUILDING_LEVELS);

outDesc.m_editableFields.push_back(feature::Metadata::FMD_POSTCODE);

classificatorTypes.erase(it);
Это не повлияет на отображение зданий?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub https://github.com/mapsme/omim/pull/2670/files/af10e69d522742081c4007f6da63a022705f438b#r58207848

mpimenov · 2016-04-01T14:05:16Z

LGTM

Zverik · 2016-04-01T14:06:13Z

LGTM

ygorshenin · 2016-04-01T18:17:02Z

base/base_tests/string_utils_test.cpp

biodranik · 2016-04-04T09:22:18Z

@ygorshenin @vng PTAL & merge please.

biodranik · 2016-04-04T14:53:09Z

@ygorshenin @vng PTAL & merge please.
DUDE JTTP

ygorshenin · 2016-04-04T15:29:31Z

generator/osm2meta.cpp

Т.е. наша прога сломается с очередным арабским небоскребом? Зачем как-то ограничивать это чиселко?

Чтобы фильтровать мусор из осма, где может быть всё, что угодно.

On 4.4.2016, at 18.29, ygorshenin notifications@github.com wrote:

In generator/osm2meta.cpp:

{

double d;

if (!strings::to_double(v, d) || d == 0)

return {};

return strings::to_string_dac(d, 1);

// https://en.wikipedia.org/wiki/List_of_tallest_buildings_in_the_world

auto constexpr kMaxBuildingLevelsInTheWorld = 167;
Т.е. наша прога сломается с очередным арабским небоскребом? Зачем как-то ограничивать это чиселко?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

Чтобы фильтровать мусор из осма достаточно проверить что передано число, а не что передано число до 167.

Превышающие значения будут не около 168, а за 500: это скажется и на нашем рендеринге (наверное, я не проверял), и ещё, может, на чём-нибудь. Не вижу ничего плохого в том, чтобы проверять.

biodranik · 2016-04-05T10:55:59Z

@ygorshenin @vng PTAL
DUDE JTTP

biodranik · 2016-04-05T11:51:37Z

Падают не мои тесты, они уже пофикшены в мастере, но мой реквест еще фикс не включает.

vng · 2016-04-05T15:36:55Z

LGTM

ygorshenin · 2016-04-06T12:19:30Z

LGTM

biodranik assigned Zverik Mar 31, 2016

Zverik reviewed Mar 31, 2016
View reviewed changes

data/editor.config Outdated

Copy link

Contributor

Zverik Mar 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

То есть, решили не мёржить всё подряд?

biodranik force-pushed the building-levels-support branch 2 times, most recently from 2191496 to 0245a28 Compare April 1, 2016 11:14

vng reviewed Apr 1, 2016
View reviewed changes

biodranik force-pushed the building-levels-support branch from 0245a28 to af10e69 Compare April 1, 2016 12:47

Zverik reviewed Apr 1, 2016
View reviewed changes

ygorshenin reviewed Apr 1, 2016
View reviewed changes

base/base_tests/string_utils_test.cpp Outdated

Copy link

Contributor

ygorshenin Apr 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nd

biodranik force-pushed the building-levels-support branch from af10e69 to 48f13c5 Compare April 4, 2016 09:21

ygorshenin reviewed Apr 4, 2016
View reviewed changes

biodranik added 3 commits April 5, 2016 13:54

strings::NormalizeDigits for full-width unicode numbers.

f4c112b

Correct building:levels parsing in generator.

dd8be8c

Base validation for building:levels on a client side.

f5d1ff1

biodranik added 3 commits April 5, 2016 13:55

[editor] Correctly merge editable fields for building + any other type.

c535bc4

[qt] editor.config is a part of project bundle.

32a50c6

Speed improvement from Yury Gorshenin.

baf4a0c

biodranik force-pushed the building-levels-support branch from f95ce43 to baf4a0c Compare April 5, 2016 10:55

ygorshenin merged commit 8a83e58 into mapsme:master Apr 6, 2016

Comments

Conversation

biodranik commented Mar 31, 2016

Uh oh!

biodranik commented Mar 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biodranik commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biodranik commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpimenov commented Apr 1, 2016

Uh oh!

Zverik commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biodranik commented Apr 4, 2016

Uh oh!

biodranik commented Apr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biodranik commented Apr 5, 2016

Uh oh!

biodranik commented Apr 5, 2016

Uh oh!

vng commented Apr 5, 2016

Uh oh!

ygorshenin commented Apr 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants