[SDK-511] New methods in MapGL #18

maxal9999 · 2021-04-08T21:43:59Z

FloorPlan and Map.floorPlan
Map.padding
GeographicalBounds.extend
Map.setStyleState and Map.patchStyleState
isSupported, notSupportedReason

teanet

Желательно поправить

teanet · 2021-04-12T07:33:28Z

Sources/MapGL/Model/FloorPlan.swift

+
+public class FloorPlan {
+	private var _currentLevelIndex: Int = 0
+	private let onLevelChanged: (Int) -> Void


private желательно под public по уровню доступа

teanet · 2021-04-12T07:34:39Z

Sources/MapGL/Model/FloorPlan.swift

+			return _currentLevelIndex
+		}
+		set {
+			guard newValue >= 0 && newValue < levels.count else {


self.levels тоже желательно тут и ниже, для единообразия

teanet · 2021-04-12T07:36:26Z

Sources/MapGL/Model/GeographicalBounds.swift

+
+	///Extend the bounds to include a given point.
+	public func extend(point: CLLocationCoordinate2D) -> GeographicalBounds {
+		return GeographicalBounds(


return self.extend(points: [point])?

teanet · 2021-04-12T07:39:12Z

Sources/MapGL/Model/FloorPlan.swift

+import Combine
+import Foundation
+
+public class FloorPlan {


комбайн с 13 iOS а мы поддерживаем с 10, нужно обмазать @available(iOS 13.0, *) как минимум

никакой документации нет, нужно класс и пропрертя публичные обмазать документацией

Комбайн не нужен тут - я его уберу.

teanet · 2021-04-12T07:48:23Z

Sources/MapGL/Extensions/JS+Extensions.swift

+		return
+			"{" +
+			(self.map({ (key, value) -> String in
+			return "\"\(key)\": \(value)"}) as Array).joined(separator: ",") +


очень тяжело читается,
давай так перепишем

let dictionary = self.map { (key, value) -> String in """ "\(key)": \(value) """ }.joined(separator: ",") return "{" + dictionary + "}"

Там ещё, выше по коду, есть IJSOptions, в котором делается то же самое :)

не, там не экранируется ключ, а так, да очень похоже)

teanet · 2021-04-12T07:49:41Z

Sources/MapGL/Model/FloorPlan.swift

+		self.id = id
+		self.levels = levels
+		self._currentLevelIndex = currentLevelIndex
+		self.currentLevel = LiveData<String>(levels[currentLevelIndex])


нет защиты от currentLevelIndex > levels.count

teanet · 2021-04-12T07:51:02Z

Sources/MapGL/Model/Padding.swift

+	public var bottom: CGFloat = 0.0
+	public var left: CGFloat = 0.0
+	public var right: CGFloat = 0.0
+	public var top: CGFloat = 0.0


можно еще публичный init для удобства

Сделал

t-artikov · 2021-04-12T10:41:14Z

Sources/MapGL/Model/LiveData.swift

@@ -0,0 +1,21 @@
+import Foundation
+
+public class LiveData<T> {


Мимо проходил.
LiveData - стандартный класс из Android, в iOS он будет чуждым, лучше было бы использовать то что принято на платформе/в проекте.

Да, я сделал некий аналог класс-обертку. в iOS я нашел Observable, но он доступен только с iOS 13.
А моя реализация сделана на основе didSet, который стандартный iOS-сный.

Думаю тебе с Женей нужно решить, как быть - написание своего observable-типа - непустяковое дело.
Если возможности iOS 13 недоступны, то может обойтись тогда колбеками, как сделано с MapView.centerDidChange, MapView.zoomDidChange.
По сравнению с Android-овской у твоей LiveData есть недостатки, делающие использование неочевидным/опасным -

value можно устанавливать снаружи, нарушая инкапсуляцию (в Android против этого есть разделение на LiveData/MutableLiveData)

нет поддержки нескольких слушателей

нельзя задать поток срабатывания callback-a (в Android тоже нельзя, но там срабатывание передается на UI-поток, что для простых случаев годится)

установка значения непотокобезопасная

Понял, спасибо, буду обсуждать тогда.

Сделаю все таки колбеками. Так лучше будет, действительно.

teanet · 2021-04-13T04:31:28Z

Sources/MapGL/Model/FitBoundsOptions.swift

+	/// The amount of padding in pixels to add to the given bounds.
+	public var padding: Padding?
+
+	public init() {}


лучше убрать init и переписать

public init(considerRotation: Bool = false, padding: Padding? = nil) { self.considerRotation = considerRotation self.padding = padding }

Действительно, так и правда лучше

teanet · 2021-04-13T04:33:02Z

Sources/MapGL/Model/FitBoundsOptions.swift

+	public var considerRotation: Bool = false
+
+	/// The amount of padding in pixels to add to the given bounds.
+	public var padding: Padding?


кажется это лучше сделать через let,
public let considerRotation: Bool
public let padding: Padding?
так как выставление этих параметров ни на что не влияет и лучше эту структуру создавать только через конструктор

teanet · 2021-04-13T04:34:51Z

Sources/MapGL/Model/Padding.swift

+	public var right: CGFloat = 0.0
+	public var top: CGFloat = 0.0
+
+	public init() {}


тут тоже лучше удалить init и сделать один с defaultValues

teanet · 2021-04-13T04:35:58Z

Sources/MapGL/Model/GeographicalBounds.swift

@@ -16,6 +16,31 @@ public struct GeographicalBounds: Decodable {
 		self.northEast = northEast
 		self.southWest = southWest
 	}
+
+	///Extend the bounds to include a given point.


/// [пробел] Extend the bounds to include a given point.
без пробела это некорректная документация, которая не распарсится нашей утилитой

teanet · 2021-04-13T04:36:17Z

Sources/MapGL/Model/GeographicalBounds.swift

+	}
+
+	///Extend the bounds to include given points.
+	public func extend(points: [CLLocationCoordinate2D]) -> GeographicalBounds {


пробел после ///

teanet · 2021-04-13T04:48:04Z

Sources/MapGL/MapView.swift

+
+	/// Tests whether the current browser supports MapGL and returns the reason if not
+	public func notSupportedReason(options: MapSupportOptions? = nil) -> String? {
+		if options?.failIfMajorPerformanceCaveat == true {


я пытался понять логику, и не до конца понял, вот я хочу проверить совместимость, создам
MapView вызову этот метод до инициализации карты, и он мне вернет true,
я создаю карту, и она по итогу не создается кажется это не ожидаемое поведение

я точно не знаю как это работает, но кажется нужно возвращать этот статус как результат инициализации карты, кажется только в этот момент мы можем точно быть уверены что у нас все корректно отработает

этот вопрос так и остался без ответа

aapng · 2021-04-12T13:25:42Z

Example/HelloSDK/HelloVC.swift

@@ -97,6 +104,17 @@ final class HelloVC: UIViewController {
 			self?.mapPitchLabel.text = "Map pitch: \(pitch)"
 		}

+		self.map.floorPlanChange = { [weak self] currentLevelIndex in
+			if let currentLevelIndex = currentLevelIndex {


Думаю, по смыслу нужно объединить проверки у первого и второго if-а. Пишется через запятую.

aapng · 2021-04-12T13:27:27Z

Example/HelloSDK/HelloVC.swift

+		self.map.floorPlanChange = { [weak self] currentLevelIndex in
+			if let currentLevelIndex = currentLevelIndex {
+				if self?.map.floorPlan?.currentLevelIndex == currentLevelIndex {
+					self?.mapFloorPlanLabel.text = "Floor plan \(currentLevelIndex) shows."


Suggested change

self?.mapFloorPlanLabel.text = "Floor plan \(currentLevelIndex) shows."

self?.mapFloorPlanLabel.text = "Floor plan: \(currentLevelIndex)"

aapng · 2021-04-12T13:27:50Z

Example/HelloSDK/HelloVC.swift

+				}
+			}
+			else {
+				self?.mapFloorPlanLabel.text = "Floor plan hides."


Suggested change

self?.mapFloorPlanLabel.text = "Floor plan hides."

self?.mapFloorPlanLabel.text = "Floor plan: hidden"

aapng · 2021-04-12T13:32:17Z

Sources/MapGL/Extensions/JS+Extensions.swift

@@ -93,3 +93,14 @@ extension CGSize: IJSValue {
 extension CGPoint: IJSValue {
 	func jsValue() -> String { "[\(self.x),\(self.y)]" }
 }
+
+extension Dictionary: IJSValue where Key: ExpressibleByStringLiteral, Value: IJSValue {


А зачем ExpressibleByStringLiteral? Это же не используется в тексте функции.

aapng · 2021-04-12T13:33:22Z

Sources/MapGL/JSBridge.swift

+
+	func setStyleState(styleState: [String: Bool]) {
+		let js = """
+		window.setStyleState(\(styleState.mapValues { value in value.jsValue() }.jsValue()));


Вытащи наружу выражение в \(...). Приходится вчитываться, чтобы рассмотреть суть.

aapng · 2021-04-13T09:45:12Z

Sources/MapGL/Model/MapSupportOptions.swift

+
+/// Options for Map.isSupported method.
+public struct MapSupportOptions {
+	/// Causes isSupported method to return false if the performance of MapGL would be


Suggested change

/// Causes isSupported method to return false if the performance of MapGL would be

/// Causes `isSupported` to return false if performance of MapGL would be

aapng · 2021-04-13T09:46:31Z

Sources/MapGL/Model/Padding.swift

@@ -0,0 +1,29 @@
+import Foundation
+
+/// Padding in density independent pixels from the different sides of the map canvas.


То есть в логических точках?

Suggested change

/// Padding in density independent pixels from the different sides of the map canvas.

/// Padding in points from the different sides of the map canvas.

aapng · 2021-04-13T09:48:56Z

Sources/MapGL/Model/Padding.swift

+	public var right: CGFloat = 0.0
+	public var top: CGFloat = 0.0
+
+	public init() {}


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

aapng · 2021-04-13T09:50:19Z

Sources/MapGL/Model/Padding.swift

+
+/// Padding in density independent pixels from the different sides of the map canvas.
+public struct Padding {
+	public var bottom: CGFloat = 0.0


Лишняя инициализация. Конструктор всё сделает сам.

aapng · 2021-04-13T10:07:00Z

Sources/MapGL/Extensions/JS+Extensions.swift

+
+extension Dictionary: IJSValue where Key: ExpressibleByStringLiteral, Value: IJSValue {
+	func jsValue() -> String {
+		let dictionary = self.map { (key, value) -> String in


Имя не очень хорошее. По сути это не словарь, а ключи-значения через запятую. Переименуй в keyValues.

Вот со скобками уже словарь.

aapng · 2021-04-15T18:37:27Z

Example/HelloSDK/HelloVC.swift

+		self.map.floorPlanDidChange = { [weak self] currentLevelIndex in
+			if let currentLevelIndex = currentLevelIndex,
+			   self?.map.floorPlan?.currentLevelIndex == currentLevelIndex {
+				self?.mapFloorPlanLabel.text = "Floor plan: \(currentLevelIndex)"


Тут лишний отступ. Не нужно стесняться, что строки сливаются. Если в общепринятом стиле есть проблемы, пусть все это видят. :)

Suggested change

self?.mapFloorPlanLabel.text = "Floor plan: \(currentLevelIndex)"

self?.mapFloorPlanLabel.text = "Floor plan: \(currentLevelIndex)"

aapng · 2021-04-15T18:39:22Z

Example/HelloSDK/HelloVC.swift

@@ -97,6 +104,16 @@ final class HelloVC: UIViewController {
 			self?.mapPitchLabel.text = "Map pitch: \(pitch)"
 		}

+		self.map.floorPlanDidChange = { [weak self] currentLevelIndex in
+			if let currentLevelIndex = currentLevelIndex,
+			   self?.map.floorPlan?.currentLevelIndex == currentLevelIndex {


В новом икскоде стали странно выравнивать. Получается смесь пробелов и табов. Не буду просить это править.

aapng · 2021-04-15T18:41:22Z

Sources/MapGL/JSBridge.swift

+
+	func patchStyleState(styleState: [String: Bool]) {
+		let js = """
+		window.patchStyleState(\(styleState.mapValues { value in value.jsValue() }.jsValue()));


Тут тоже хотелось бы сделать читаемее. На твоё уже усмотрение.

Да, точно. По аналогии же нужно. Как я проглядел(

aapng · 2021-04-15T18:42:06Z

Sources/MapGL/JSBridge.swift

+
+	func fitBounds(bounds: GeographicalBounds, options: FitBoundsOptions?) {
+		let js = """
+		window.fitBounds(\(bounds.jsValue()), \((options != nil) ? options.jsValue() : "null"));


Вынеси наружу и убери лишние скобки вокруг условия.

aapng · 2021-04-15T18:43:53Z

Sources/MapGL/JSBridge.swift

+
+	func fitBounds(bounds: GeographicalBounds, options: FitBoundsOptions?) {
+		let js = """
+		window.fitBounds(\(bounds.jsValue()), \((options != nil) ? options.jsValue() : "null"));


Тут интересно. Раз вызываешь options.jsValue, значит Optional<FitBoundsOptions> тоже реализует IJSValue. Тогда он дожен сам превращаться в "null" из nil.

А, уже есть
extension Optional: IJSValue where Wrapped: IJSValue {
Так что можно оставить просто options.jsValue()

aapng · 2021-04-15T18:48:13Z

Sources/MapGL/JSBridge.swift

+				let data = body["value"] as? [String: String?]
+				let notSupportedReason = data?["notSupportedReason"] as? String
+				let notSupportedWithGoodPerformanceReason = data?["notSupportedWithGoodPerformanceReason"] as? String
+				delegate.js(self, supportedReason: notSupportedReason,


В другом месте уже иначе переносишь:

self._support = MapSupport( notSupportedReason: notSupportedReason, notSupportedWithGoodPerformanceReason: notSupportedWithGoodPerformanceReason )

Пусть тут будет так же.

aapng · 2021-04-15T18:50:56Z

Sources/MapGL/MapView.swift

@@ -21,6 +21,11 @@ public class MapView : UIView {
 		static let mapDefaultCenter = CLLocationCoordinate2D(latitude: 55.750574, longitude: 37.618317)
 	}

+	struct MapSupport {


На практике могут оба поля быть ненулевыми? Enum бы тут подошёл?

Да, конечно могут. Даже больше - в основном одновременно и будут ненулевыми.

aapng · 2021-04-15T18:54:01Z

Sources/MapGL/MapView.swift

+
+	/// Tests whether the current browser supports MapGL.
+	/// Use our raster map implementation https://api.2gis.ru/doc/maps/en/quickstart/ if not.
+	public func isSupported(options: MapSupportOptions? = nil) -> Bool? {


Не хватает документации на возвращаемое значение. Что означает nil?

aapng · 2021-04-15T18:57:06Z

Sources/MapGL/MapView.swift

@@ -75,6 +85,10 @@ public class MapView : UIView {
 	public var pitchDidChange: ((Double) -> Void)?
 	/// Notifies of the map click event.
 	public var mapClick: ((CLLocationCoordinate2D) -> Void)?
+	/// Notifies of the floor plan change.
+	public var floorPlanDidChange: ((Int?) -> Void)?


А почему бы нам сразу всю структуру FloorPlan не отправить?

aapng · 2021-04-15T19:02:27Z

Sources/MapGL/Model/HTML.swift

@@ -48,6 +48,9 @@ enum HTML {
 				value: args
 			});
 		};
+
+		try {
+
 		var objects = new Map();


Ага. Ещё хочу уточнить: а мы помельче try можем сделать? Интересная нам ошибка прямо откуда угодно вылетает?

teanet · 2021-04-16T03:07:38Z

Sources/MapGL/JSBridge.swift

+	}
+
+	func setStyleState(styleState: [String: Bool]) {
+		let styleState = styleState.mapValues { value in value.jsValue() }.jsValue()


можно просто styleState.mapValues { $0.jsValue() }.jsValue()

teanet · 2021-04-16T03:07:53Z

Sources/MapGL/JSBridge.swift

+	}
+
+	func patchStyleState(styleState: [String: Bool]) {
+		let styleState = styleState.mapValues { value in value.jsValue() }.jsValue()


можно просто styleState.mapValues { $0.jsValue() }.jsValue()

teanet · 2021-04-16T03:11:25Z

Sources/MapGL/MapView.swift

+
+	/// Tests whether the current browser supports MapGL and returns the reason if not
+	public func notSupportedReason(options: MapSupportOptions? = nil) -> String? {
+		if options?.failIfMajorPerformanceCaveat == true {


этот вопрос так и остался без ответа

maxal9999 changed the title ~~[WIP][SDK-511] New method in MapGL~~ [SDK-511] New method in MapGL Apr 9, 2021

maxal9999 changed the title ~~[SDK-511] New method in MapGL~~ [SDK-511] New methods in MapGL Apr 9, 2021

ktoto requested review from teanet and aapng April 12, 2021 06:24

teanet reviewed Apr 12, 2021

View reviewed changes

t-artikov reviewed Apr 12, 2021

View reviewed changes

teanet reviewed Apr 13, 2021

View reviewed changes

aapng requested changes Apr 13, 2021

View reviewed changes

aapng reviewed Apr 15, 2021

View reviewed changes

[SDK-511] New method in MapGL

0e4ccd1

maxal9999 force-pushed the feature-SDK-511-floor-plan-padding-lng-lat-bounds branch from c80a33e to 0e4ccd1 Compare April 15, 2021 22:24

teanet reviewed Apr 16, 2021

View reviewed changes

maxal9999 closed this Apr 16, 2021

maxal9999 deleted the feature-SDK-511-floor-plan-padding-lng-lat-bounds branch April 16, 2021 11:16

	self?.mapFloorPlanLabel.text = "Floor plan \(currentLevelIndex) shows."
	self?.mapFloorPlanLabel.text = "Floor plan: \(currentLevelIndex)"

	/// Causes isSupported method to return false if the performance of MapGL would be
	/// Causes `isSupported` to return false if performance of MapGL would be

		@@ -0,0 +1,29 @@
		import Foundation

		/// Padding in density independent pixels from the different sides of the map canvas.

	/// Padding in density independent pixels from the different sides of the map canvas.
	/// Padding in points from the different sides of the map canvas.

[SDK-511] New methods in MapGL #18

[SDK-511] New methods in MapGL #18

Conversation

maxal9999 commented Apr 8, 2021 • edited Loading

teanet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teanet Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxal9999 Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxal9999 commented Apr 8, 2021 •

edited

Loading

teanet Apr 12, 2021 •

edited

Loading

maxal9999 Apr 12, 2021 •

edited

Loading