[ios] Red for over speed; Speed limit for Carplay #3047

2022-07-29 18:20:45 +00:00

biodranik (Migrated from github.com) commented

const

2022-07-29 18:21:38 +00:00

biodranik (Migrated from github.com) commented

Лучше всё-таки везде длиннее, но однозначнее, тогда комментарий не нужен.

  double m_speedLimitMetersPerSecond = -1.0;

Лучше всё-таки везде длиннее, но однозначнее, тогда комментарий не нужен. ```suggestion double m_speedLimitMetersPerSecond = -1.0; ```

2022-07-29 18:23:02 +00:00

biodranik (Migrated from github.com) commented

Почему именно отрицательное значение выбрано, а не ноль, например? Если это особая константа, то лучше её вынести явно.

AntonM030481 commented

2022-07-29 19:01:56 +00:00

(Migrated from github.com)

@biodranik хорошо)
Я просто все заново красиво форс репушил, и PR закрылся в процессе (откатился на самое начало). А кнопку Reopen я не заметил.

@biodranik хорошо) Я просто все заново красиво форс репушил, и PR закрылся в процессе (откатился на самое начало). А кнопку Reopen я не заметил.

vng (Migrated from github.com) reviewed 2022-07-30 00:51:05 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift

					
				@ -175,0 +183,4 @@

				      speedBackground.backgroundColor = UIColor.clear

				    }

				    speedLegendLabel.textColor = speedLabel.textColor

				    speedWithLegendLabel.textColor = speedLabel.textColor

vng (Migrated from github.com) commented

2022-07-30 00:34:17 +00:00

Да, просто C++ -> ObjC функция для использования в Swift.

vng (Migrated from github.com) commented

2022-07-30 00:40:02 +00:00

На крайний случай valueAsString настолько проста, что достаточно в ObjC иметь простую именованную функцию типа SpeedToString

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

vng (Migrated from github.com) commented

2022-07-30 00:32:20 +00:00

Это какая-то фишка swift когда speedCamLimitMps сразу и слева и справа от присваивания?
Может тут self. добавить для наглядности?

Это какая-то фишка swift когда speedCamLimitMps сразу и слева и справа от присваивания? Может тут self. добавить для наглядности?

vng (Migrated from github.com) commented

2022-07-30 00:37:59 +00:00

Я наверное понимаю как это, но вызывает улыбку :)
if speedLimitMps { не одно и то же?

Я наверное понимаю как это, но вызывает улыбку :) if speedLimitMps { не одно и то же?

platform/localization.hpp

vng (Migrated from github.com) commented

2022-07-30 00:46:20 +00:00

Хм, действительно ли нам нужно показывать это наружу? Почему недостаточно GetLocalizedSpeedUnits() текущей? Иначе зачем клиентскому коду тоже вызывать Settings::Get ...

2022-07-30 00:47:42 +00:00

vng (Migrated from github.com) commented

Мы же тут уже внутри namespace measurement_utils

platform/measurement_utils.hpp

					
				@ -31,7 +31,8 @@ inline double InchesToMeters(double in) { return in / 39.370; }

				inline double NauticalMilesToMeters(double nmi) { return nmi * 1852; }

vng (Migrated from github.com) commented

2022-07-30 00:47:28 +00:00

Мы же тут уже внутри namespace measurement_utils

2022-07-30 00:49:48 +00:00

vng (Migrated from github.com) commented

Мой имхо MetersPerSecond - перебор. С чем это можно перепутать?

biodranik (Migrated from github.com) reviewed 2022-07-30 12:24:51 +00:00

2022-07-30 12:24:51 +00:00

biodranik (Migrated from github.com) commented

Как с чем? Я читаю всегда сначала как miles per second, стандартная аббревиатура MPS.

biodranik (Migrated from github.com) reviewed 2022-07-30 12:26:08 +00:00

2022-07-30 12:26:08 +00:00

biodranik (Migrated from github.com) commented

И многие американцы/англичане тоже могут так читать.

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 07:46:59 +00:00

android/jni/com/mapswithme/maps/Framework.cpp

AntonM030481 (Migrated from github.com) commented

2022-08-01 07:46:58 +00:00

OK

vng (Migrated from github.com) reviewed 2022-08-01 08:04:11 +00:00

2022-08-01 08:04:11 +00:00

vng (Migrated from github.com) commented

Саша, miles per second? Та на ракете летаешь? Нет такой стандартной аббревиатуры :)

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 08:20:46 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift

					
				@ -163,1 +168,4 @@

				      speed += " / " + (info.speedLimitMps == 0 ? "∞" : speedLimitMeasure.valueAsString);

				    }

				    speedLabel.text = speed

AntonM030481 (Migrated from github.com) commented

2022-08-01 08:20:46 +00:00

https://developer.apple.com/documentation/corelocation/cllocation/1423798-speed
A negative value indicates an invalid speed.

https://developer.apple.com/documentation/corelocation/cllocation/1423798-speed A negative value indicates an invalid speed.

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 08:23:59 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift

AntonM030481 (Migrated from github.com) commented

2022-08-01 08:23:59 +00:00

OK

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 08:25:17 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-01 08:25:17 +00:00

OK

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 08:26:44 +00:00

2022-08-01 08:26:44 +00:00

AntonM030481 (Migrated from github.com) commented

OK

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 08:28:09 +00:00

platform/localization.hpp

AntonM030481 (Migrated from github.com) commented

2022-08-01 08:28:09 +00:00

OK

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 08:34:00 +00:00

2022-08-01 08:34:00 +00:00

AntonM030481 (Migrated from github.com) commented

Мили - это всегда Mi,
MPH - единственное исключение.

Сейчас speedMetersPerSecond есть только в turns_sound_settings.

Мили - это всегда Mi, MPH - единственное исключение. Сейчас speedMetersPerSecond есть только в turns_sound_settings.

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 08:58:37 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-01 08:58:37 +00:00

У нас в коде такой подход постоянно встречается:
https://stackoverflow.com/questions/29322977/whats-the-difference-between-if-nil-optional-and-if-let-optional
Временная переменная вместо сравнения с nil чтобы потом не делать unwrap из optional.

У нас в коде такой подход постоянно встречается: https://stackoverflow.com/questions/29322977/whats-the-difference-between-if-nil-optional-and-if-let-optional Временная переменная вместо сравнения с nil чтобы потом не делать unwrap из optional.

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 09:00:06 +00:00

platform/measurement_utils.hpp

					
				@ -31,7 +31,8 @@ inline double InchesToMeters(double in) { return in / 39.370; }

				inline double NauticalMilesToMeters(double nmi) { return nmi * 1852; }

AntonM030481 (Migrated from github.com) commented

2022-08-01 09:00:06 +00:00

OK

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 09:01:26 +00:00

2022-08-01 09:01:26 +00:00

AntonM030481 (Migrated from github.com) commented

OK

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 09:09:24 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-01 09:09:24 +00:00

Conditional unwrapping. Чаще всего используется с тем же именем.
Прояснил в другом месте.

Conditional unwrapping. Чаще всего используется с тем же именем. Прояснил в другом месте.

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 09:48:14 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/MWMNavigationDashboardEntity.h

AntonM030481 (Migrated from github.com) commented

2022-08-01 09:48:14 +00:00

OK

biodranik (Migrated from github.com) reviewed 2022-08-01 11:35:30 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

biodranik (Migrated from github.com) commented

2022-08-01 11:35:30 +00:00

Вопросов нет, если это optional. Вопросы есть, если это обычная переменная )

biodranik (Migrated from github.com) reviewed 2022-08-01 11:37:11 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

biodranik (Migrated from github.com) commented

2022-08-01 11:37:10 +00:00

А, вижу, там optional. Странно конечно, можно было бы сделать оптимальнее.

Вообще, старый swift код был написан до нас, без нашего ревью. Поэтому слепо применять старые практики не стоит. Код должен быть читабельный, простой, не вызывать вопросов, и не иметь ненужного оверхеда в рантайме.

А, вижу, там optional. Странно конечно, можно было бы сделать оптимальнее. Вообще, старый swift код был написан до нас, без нашего ревью. Поэтому слепо применять старые практики не стоит. Код должен быть читабельный, простой, не вызывать вопросов, и не иметь ненужного оверхеда в рантайме.

biodranik (Migrated from github.com) reviewed 2022-08-01 11:38:17 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

biodranik (Migrated from github.com) commented

2022-08-01 11:38:17 +00:00

Я не понимаю, зачем отдельно speedcam limit. Когда он отличается от обычного speed limit? Зачем усложнять?

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 14:12:08 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-01 14:12:07 +00:00

Насчет оверхэда в ссылке выше пишут выше что разницы нет в ассемблере.
Вроде нормальная практика так делать, когда unwrapped значение далее используется кроме как в самом if.
В данном случае - целых 2 раза)

Насчет оверхэда в ссылке выше пишут выше что разницы нет в ассемблере. Вроде нормальная практика так делать, когда unwrapped значение далее используется кроме как в самом if. В данном случае - целых 2 раза)

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 14:29:59 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift

					
				@ -175,0 +183,4 @@

				      speedBackground.backgroundColor = UIColor.clear

				    }

				    speedLegendLabel.textColor = speedLabel.textColor

				    speedWithLegendLabel.textColor = speedLabel.textColor

AntonM030481 (Migrated from github.com) commented

2022-08-01 14:29:59 +00:00

Хотелось универсальное решение втч и для расстояния, когда в зависимости от длины используются крупные или мелкие юниты. А для этого логично получать пару размера и юнитов одновременно.
А что именно с классом не нравится? Вызов 3 C++ функций вместо одной?
Где можно почитать про overhead в аналогичных ситуациях?

Кстати, сейчас есть такое, например:
c2c1f979ac/iphone/Maps/UI/PlacePage/Components/ElevationProfile/ElevationProfilePresenter.swift (L243-L244)

В Java сейчас такой подход (но только нужно число, а не строка):
c2c1f979ac/android/jni/com/mapswithme/util/StringUtils.cpp (L57-L60)

Хотелось универсальное решение втч и для расстояния, когда в зависимости от длины используются крупные или мелкие юниты. А для этого логично получать пару размера и юнитов одновременно. А что именно с классом не нравится? Вызов 3 C++ функций вместо одной? Где можно почитать про overhead в аналогичных ситуациях? Кстати, сейчас есть такое, например: https://github.com/organicmaps/organicmaps/blob/c2c1f979ac9d9422eae5ae72dad1975b49a47061/iphone/Maps/UI/PlacePage/Components/ElevationProfile/ElevationProfilePresenter.swift#L243-L244 В Java сейчас такой подход (но только нужно число, а не строка): https://github.com/organicmaps/organicmaps/blob/c2c1f979ac9d9422eae5ae72dad1975b49a47061/android/jni/com/mapswithme/util/StringUtils.cpp#L57-L60

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 14:51:12 +00:00

2022-08-01 14:51:12 +00:00

AntonM030481 (Migrated from github.com) commented

А потом сравнивать с этой константой? Насколько это уместно для double?

AntonM030481 (Migrated from github.com) reviewed 2022-08-01 16:31:53 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift

					
				@ -175,0 +183,4 @@

				      speedBackground.backgroundColor = UIColor.clear

				    }

				    speedLegendLabel.textColor = speedLabel.textColor

				    speedWithLegendLabel.textColor = speedLabel.textColor

AntonM030481 (Migrated from github.com) commented

2022-08-01 16:31:53 +00:00

Замерил быстродействие:
Measure.init(asSpeed: v)
vs

let imperial = Settings.measurementUnits() == .imperial
var altMeasurement = Measurement(value: v, unit: UnitLength.meters)
altMeasurement.convert(to: imperial ? UnitLength.feet : UnitLength.meters)

Если убрать запрос к platform::GetLocalizedSpeedUnits(units):

auto units = measurement_utils::Units::Metric;
settings::TryGet(settings::kMeasurementUnits, units);
_value = measurement_utils::MpsToUnits(mps, units);
_unit = @(units == measurement_utils::Units::Metric ? "kmph" : "mph");

результат одинаков
а если его оставить - разница в 20 раз.
Вывод - bottleneck в запросе к локализации строки (что ожидаемо), а не в конструировании объекта.

Замерил быстродействие: `Measure.init(asSpeed: v)` vs ``` let imperial = Settings.measurementUnits() == .imperial var altMeasurement = Measurement(value: v, unit: UnitLength.meters) altMeasurement.convert(to: imperial ? UnitLength.feet : UnitLength.meters) ``` Если убрать запрос к platform::GetLocalizedSpeedUnits(units): ``` auto units = measurement_utils::Units::Metric; settings::TryGet(settings::kMeasurementUnits, units); _value = measurement_utils::MpsToUnits(mps, units); _unit = @(units == measurement_utils::Units::Metric ? "kmph" : "mph"); ``` результат одинаков а если его оставить - разница в 20 раз. Вывод - bottleneck в запросе к локализации строки (что ожидаемо), а не в конструировании объекта.

biodranik (Migrated from github.com) reviewed 2022-08-01 21:10:41 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift

					
				@ -175,0 +183,4 @@

				      speedBackground.backgroundColor = UIColor.clear

				    }

				    speedLegendLabel.textColor = speedLabel.textColor

				    speedWithLegendLabel.textColor = speedLabel.textColor

biodranik (Migrated from github.com) commented

2022-08-01 21:10:40 +00:00

А что именно с классом не нравится? Вызов 3 C++ функций вместо одной?

Динамическая аллокация, которой лучше избегать в часто повторяемых операциях. Такой же объект, созданный на стеке, вообще бы не вызывал вопросов.

Кстати, сейчас есть такое, например:

Вот в т.ч. и из-за подобных велосипедов на изолиниях теперь всегда метры рисуются.

Вывод - bottleneck в запросе к локализации строки (что ожидаемо), а не в конструировании объекта.

Если пользователь идёт в настройки и меняет юниты (крайне редкая операция), то диалоги пересоздаются. Вывод: строки/юниты можно и нужно инициализировать один раз для сессии диалога, и на ведре, и на айосе.

> А что именно с классом не нравится? Вызов 3 C++ функций вместо одной? Динамическая аллокация, которой лучше избегать в часто повторяемых операциях. Такой же объект, созданный на стеке, вообще бы не вызывал вопросов. > Кстати, сейчас есть такое, например: Вот в т.ч. и из-за подобных велосипедов на изолиниях теперь всегда метры рисуются. > Вывод - bottleneck в запросе к локализации строки (что ожидаемо), а не в конструировании объекта. Если пользователь идёт в настройки и меняет юниты (крайне редкая операция), то диалоги пересоздаются. Вывод: строки/юниты можно и нужно инициализировать один раз для сессии диалога, и на ведре, и на айосе.

biodranik (Migrated from github.com) reviewed 2022-08-01 21:12:46 +00:00

2022-08-01 21:12:46 +00:00

biodranik (Migrated from github.com) commented

Т.е. критерий выбора — невалидность определяется как < 0? Тогда всё равно константа делает код нагляднее.

На ноль даблы можно смело сравнивать, если они не модифицируюся и не являются результатом вычисления, а только следствием явного присваивания. У нас включен fast math если что, там вообще многие проверки на даблы убраны.

Т.е. критерий выбора — невалидность определяется как < 0? Тогда всё равно константа делает код нагляднее. На ноль даблы можно смело сравнивать, если они не модифицируюся и не являются результатом вычисления, а только следствием явного присваивания. У нас включен fast math если что, там вообще многие проверки на даблы убраны.

biodranik commented

2022-08-01 21:15:08 +00:00

(Migrated from github.com)

Я просто все заново красиво форс репушил, и PR закрылся в процессе (откатился на самое начало). А кнопку Reopen я не заметил.

Форс репуш никак не должен закрывать PR. Это было что-то другое.

> Я просто все заново красиво форс репушил, и PR закрылся в процессе (откатился на самое начало). А кнопку Reopen я не заметил. Форс репуш никак не должен закрывать PR. Это было что-то другое.

biodranik commented

2022-08-01 21:48:53 +00:00

(Migrated from github.com)

@AntonM030481 нужно отредактировать коммит и убрать правки сабмодулей.

biodranik (Migrated from github.com) reviewed 2022-08-01 21:50:20 +00:00

2022-08-01 21:50:20 +00:00

biodranik (Migrated from github.com) commented

Столько спора из-за более понятной строчки ) Или мой вариант менее понятен?

Столько спора из-за _более понятной_ строчки ) Или мой вариант _менее понятен_?

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 07:43:11 +00:00

2022-08-02 07:43:11 +00:00

AntonM030481 (Migrated from github.com) commented

Тогда везде надо исправлять. Необходимо единообразие как раз во избежание недопонимания.
У нас скорость как правило именно в метрах в секунду и почти везде постфикс mps.

Тогда везде надо исправлять. Необходимо единообразие как раз во избежание недопонимания. У нас скорость как правило именно в метрах в секунду и почти везде постфикс mps.

vng (Migrated from github.com) reviewed 2022-08-02 07:46:26 +00:00

2022-08-02 07:46:26 +00:00

vng (Migrated from github.com) commented

Лично у меня такие длинные название переменных m_speedLimitMetersPerSecond вызывают диссонанс.

AntonM030481 commented

2022-08-02 08:06:39 +00:00

(Migrated from github.com)

Убраны правки сабмодулей.
Знать бы еще откуда они берутся...
Я их руками не правлю, и через git их не меняю.

Убраны правки сабмодулей. Знать бы еще откуда они берутся... Я их руками не правлю, и через git их не меняю.

AntonM030481 commented

2022-08-02 08:19:15 +00:00

(Migrated from github.com)

(base) antonmakouski@x86_64-apple-darwin13 organicmaps % git status      
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
  (commit or discard the untracked or modified content in submodules)
        modified:   3party/boost (new commits)
        modified:   3party/icu/icu (new commits, modified content, untracked content)
        modified:   tools/kothic (new commits)

no changes added to commit (use "git add" and/or "git commit -a")

``` (base) antonmakouski@x86_64-apple-darwin13 organicmaps % git status On branch master Your branch is up to date with 'origin/master'. Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) (commit or discard the untracked or modified content in submodules) modified: 3party/boost (new commits) modified: 3party/icu/icu (new commits, modified content, untracked content) modified: tools/kothic (new commits) no changes added to commit (use "git add" and/or "git commit -a") ```

vng commented

2022-08-02 08:29:34 +00:00

(Migrated from github.com)

New commits??
git submodule update --init --recursive

New commits?? git submodule update --init --recursive

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 08:35:32 +00:00

platform/localization.hpp

AntonM030481 (Migrated from github.com) commented

2022-08-02 08:35:32 +00:00

Идея была в том, чтобы не вызывать лишний раз
settings::TryGet(settings::kMeasurementUnits, units);
Сейчас между localisation и measurement_utils много ненужных связей.
И по-хорошему это надо переделать.
На этот раз я решил предоставить унифицированный интерфейс для ios (конечный клиент ничего не знает о системе измерений, все локализовано в одном Core классе).
А в следующий раз наберусь сил и доделаю переделки, описанные выше.

Идея была в том, чтобы не вызывать лишний раз `settings::TryGet(settings::kMeasurementUnits, units);` Сейчас между localisation и measurement_utils много ненужных связей. И по-хорошему это надо переделать. На этот раз я решил предоставить унифицированный интерфейс для ios (конечный клиент ничего не знает о системе измерений, все локализовано в одном Core классе). А в следующий раз наберусь сил и доделаю переделки, описанные выше.

AntonM030481 commented

2022-08-02 08:43:13 +00:00

(Migrated from github.com)

Submodule path '3party/boost/libs/utility': checked out 'fe417f623790318068b8de8eec0d0cb963e9cb44'
Submodule path '3party/boost/more': checked out '631827729ba7dbed212697ab0f6d61a0748d00a5'
Submodule path '3party/boost/tools/boost_install': checked out '85e1406a570cb8aa13c8b1f885f7cc2a9982c783'
From https://github.com/boostorg/boostbook
 * [new tag]         boost-1.80.0.beta1 -> boost-1.80.0.beta1
Submodule path '3party/boost/tools/boostbook': checked out 'a6701f5d278daba797697a1e5000ee41f25bcaff'
error: The following untracked working tree files would be overwritten by checkout:
        icu4c/source/data/unidata/ucdterms.txt
Please move or remove them before you switch branches.
Aborting
From https://github.com/organicmaps/kothic
 * branch            d6a507cfc09638d489588f2cbd32569f34004318 -> FETCH_HEAD
Submodule path 'tools/kothic': checked out 'd6a507cfc09638d489588f2cbd32569f34004318'
Unable to checkout '904cf62457de2440e8526deb75c95a3f7296f517' in submodule path '3party/icu/icu'

``` Submodule path '3party/boost/libs/utility': checked out 'fe417f623790318068b8de8eec0d0cb963e9cb44' Submodule path '3party/boost/more': checked out '631827729ba7dbed212697ab0f6d61a0748d00a5' Submodule path '3party/boost/tools/boost_install': checked out '85e1406a570cb8aa13c8b1f885f7cc2a9982c783' From https://github.com/boostorg/boostbook * [new tag] boost-1.80.0.beta1 -> boost-1.80.0.beta1 Submodule path '3party/boost/tools/boostbook': checked out 'a6701f5d278daba797697a1e5000ee41f25bcaff' error: The following untracked working tree files would be overwritten by checkout: icu4c/source/data/unidata/ucdterms.txt Please move or remove them before you switch branches. Aborting From https://github.com/organicmaps/kothic * branch d6a507cfc09638d489588f2cbd32569f34004318 -> FETCH_HEAD Submodule path 'tools/kothic': checked out 'd6a507cfc09638d489588f2cbd32569f34004318' Unable to checkout '904cf62457de2440e8526deb75c95a3f7296f517' in submodule path '3party/icu/icu' ```

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 09:09:59 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift

					
				@ -175,0 +183,4 @@

				      speedBackground.backgroundColor = UIColor.clear

				    }

				    speedLegendLabel.textColor = speedLabel.textColor

				    speedWithLegendLabel.textColor = speedLabel.textColor

AntonM030481 (Migrated from github.com) commented

2022-08-02 09:09:59 +00:00

Я бы предпочел просто использовать статические переменные вместо того чтобы засорять классы ненужными переменными. По тестам быстродействие вполне нормальное (сравнимое со встроенным Measurement).
6d5d5ad891/platform/localization.cpp (L19-L45)

Я бы предпочел просто использовать статические переменные вместо того чтобы засорять классы ненужными переменными. По тестам быстродействие вполне нормальное (сравнимое со встроенным Measurement). https://github.com/AntonM030481/organicmaps/blob/6d5d5ad891141932f9263f66556e9e052ec55f58/platform/localization.cpp#L19-L45

AntonM030481 commented

2022-08-02 10:35:40 +00:00

(Migrated from github.com)

Локальную проблему с сабмодулями решил зайдя в каждый и вручную сделав чекаут на ревизии указанные тут

Локальную проблему с сабмодулями решил зайдя в каждый и вручную сделав чекаут на ревизии указанные [тут](https://github.com/organicmaps/organicmaps/tree/master/3party)

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 13:19:13 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/MWMNavigationDashboardEntity.h

AntonM030481 (Migrated from github.com) commented

2022-08-02 13:19:13 +00:00

Вернул readonly в публичном интерфейсе [MWMNavigationDashboardEntity.h]
Вернул передекларирование без readonly в [MWMNavigationDashboardManager+Entity.mm]
для возможности инициализации.
Но этого оказалось недостаточно.
uncaught exception 'NSInvalidArgumentException', reason: '-[MWMNavigationDashboardEntity setIsValid:]: unrecognized selector sent to instance' при первом же присваивании.
Помог возврат к передекларированию без readonly в [MWMNavigationDashboardEntity.mm]. Но почему - непонятно. В этом mm нет присваивания этим полям и все компилируется.

Вернул readonly в публичном интерфейсе [MWMNavigationDashboardEntity.h] Вернул передекларирование без readonly в [MWMNavigationDashboardManager+Entity.mm] для возможности инициализации. Но этого оказалось недостаточно. `uncaught exception 'NSInvalidArgumentException', reason: '-[MWMNavigationDashboardEntity setIsValid:]: unrecognized selector sent to instance'` при первом же присваивании. Помог возврат к передекларированию без readonly в [MWMNavigationDashboardEntity.mm]. Но почему - непонятно. В этом mm нет присваивания этим полям и все компилируется.

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 14:05:13 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-02 14:05:13 +00:00

speedCamLimitMps - это лимит именно от камеры.
speedLimitMps - это лимит дороги.
Иногда у камеры проставлено значение, иногда у дороги.
Скорость камеры имеет приоритет.
В общем случае лимит скорость камеры может не совпадать с текущим лимитом дороги,
потому что мы предупреждаем о камере заранее и, возможно, там где будет камера, ограничение дороги будет отличаться от текущего ограничения дороги.

speedCamLimitMps - это лимит именно от камеры. speedLimitMps - это лимит дороги. Иногда у камеры проставлено значение, иногда у дороги. Скорость камеры имеет приоритет. В общем случае лимит скорость камеры может не совпадать с текущим лимитом дороги, потому что мы предупреждаем о камере заранее и, возможно, там где будет камера, ограничение дороги будет отличаться от текущего ограничения дороги.

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 14:30:26 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-02 14:30:26 +00:00

добавил self

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 14:31:14 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-02 14:31:14 +00:00

добавить self?

vng (Migrated from github.com) reviewed 2022-08-02 15:13:55 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

vng (Migrated from github.com) commented

2022-08-02 15:13:55 +00:00

Смотри сам, особенно если в swift так принято. Мне нормально по всякому

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 15:33:47 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-02 15:33:46 +00:00

Я не знаю как принято. Написал свою первую строчку на Swift неделю назад)
Использовать то же самое имя при развертывании - логично.
Нужно ли использовать self - наверное, зависит от контекста.
Не нашел с ходу ничего в coding style, кроме того что сравниваем с nil, если развернутое значение не нужно.

Я не знаю как принято. Написал свою первую строчку на Swift неделю назад) Использовать то же самое имя при развертывании - логично. Нужно ли использовать self - наверное, зависит от контекста. Не нашел с ходу ничего в coding style, кроме того что сравниваем с nil, если развернутое значение не нужно.

AntonM030481 (Migrated from github.com) reviewed 2022-08-02 15:41:25 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/MWMNavigationDashboardEntity.h

AntonM030481 (Migrated from github.com) commented

2022-08-02 15:41:25 +00:00

@biodranik как лучше поступить в такой ситуации?

biodranik (Migrated from github.com) reviewed 2022-08-02 20:21:48 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/MWMNavigationDashboardEntity.h

biodranik (Migrated from github.com) commented

2022-08-02 20:21:48 +00:00

Конечно будет падать, если в публичном интерфейсе read only. А как раньше работало? Раньше публично никто не менял?

AntonM030481 (Migrated from github.com) reviewed 2022-08-03 06:50:14 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/MWMNavigationDashboardEntity.h

AntonM030481 (Migrated from github.com) commented

2022-08-03 06:50:14 +00:00

В публичном readonly.
В [MWMNavigationDashboardManager+Entity.mm] переопределяется как writable и там же меняется.
Я не понимаю, почему приходится переопределять в [MWMNavigationDashboardEntity.mm], поскольку там не вносятся изменения.

В публичном readonly. В [MWMNavigationDashboardManager+Entity.mm] переопределяется как writable и там же меняется. Я не понимаю, почему приходится переопределять в [MWMNavigationDashboardEntity.mm], поскольку там не вносятся изменения.

biodranik (Migrated from github.com) reviewed 2022-08-03 20:31:10 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/MWMNavigationDashboardEntity.h

biodranik (Migrated from github.com) commented

2022-08-03 20:31:10 +00:00

Т.е. вылетает в текущей версии кода в PR?

AntonM030481 (Migrated from github.com) reviewed 2022-08-05 08:23:05 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/MWMNavigationDashboardEntity.h

AntonM030481 (Migrated from github.com) commented

2022-08-05 08:23:05 +00:00

стало падать после возвращения readonly

AntonM030481 (Migrated from github.com) reviewed 2022-08-05 08:38:37 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-05 08:38:37 +00:00

добавил комментарии про это.

добавил комментарии про [это](https://git.omaps.dev/organicmaps/organicmaps/pulls/3047#discussion_r935639978).

AntonM030481 (Migrated from github.com) reviewed 2022-08-05 08:39:02 +00:00

iphone/Maps/UI/CarPlay/CarPlayMapViewController.swift

AntonM030481 (Migrated from github.com) commented

2022-08-05 08:39:02 +00:00

пока оставлю как есть.

AntonM030481 (Migrated from github.com) reviewed 2022-08-05 11:20:03 +00:00