search

Unact / yandex_mapkit

Flutter implementation of YandexMapkit
MIT License
132 stars 151 forks source link

Composite icons for placemarks #152

Closed cream-cheeze closed 2 years ago

cream-cheeze commented 2 years ago

Ответвился от мастера, перенёс в новую ветку свой ранее написанный код из ветки placemarks (саму placemarks пока не чистил - сделаю это, когда закончим с этим пулреквестом) с минимальными изменениями, связанными с проведённым в мастере рефакторингом. Для начала только iOS, чтобы время зря не тратить.

Ничего пока не стал менять - у тебя были вопросы по поводу стиля плейсмарков... Я всё ещё не вижу логики в том, что иконка - это атрибут стиля, считаю, что скорее наоборот, стиль - это атрибут иконки. В нативной библиотеки есть сущность Icon и IconStyle, что я и пытался повторить. Давай обсуждать за и против.

Пока переделывал, наткнулся на ошибку при удалении плейсмарков в мастере - поправил. Не понял, к чему были эти изменения, и зачем передавать весь объект плейсмарка при его удалении, если раньше передавался только id, и этого достаточно?

Также, я пока что закомментировал код в enableCameraTracking, требующий style в качестве аргумента. Во-первых, нужно сначала определиться с архитектурой, чтобы не переделывать по десять раз. А во-вторых, ты говорил, что готов переосмыслить эту фичу - предлагаю как раз здесь этим и заняться. Как уже говорил, данный функционал является частным случаем, и может быть легко реализован пользователями плагина самостоятельно - всё для этого имеется.

DCrow commented 2 years ago

Ответвился от мастера, перенёс в новую ветку свой ранее написанный код из ветки placemarks (саму placemarks пока не чистил - сделаю это, когда закончим с этим пулреквестом) с минимальными изменениями, связанными с проведённым в мастере рефакторингом. Для начала только iOS, чтобы время зря не тратить.

Ничего пока не стал менять - у тебя были вопросы по поводу стиля плейсмарков... Я всё ещё не вижу логики в том, что иконка - это атрибут стиля, считаю, что скорее наоборот, стиль - это атрибут иконки. В нативной библиотеки есть сущность Icon и IconStyle, что я и пытался повторить. Давай обсуждать за и против.

Так я не говорю, что у иконки не может быть стиля. Я говорю, что у плейсмарка есть стиль, у стиля есть набор иконок со своими стилями. Если дать пример, то вот так

Placemark(
  style: PlacemarkStyle(
    icons: [
      Icon(
        style: IconStyle()
      )
    ]
  )
)

Пока переделывал, наткнулся на ошибку при удалении плейсмарков в мастере - поправил. Не понял, к чему были эти изменения, и зачем передавать весь объект плейсмарка при его удалении, если раньше передавался только id, и этого достаточно?

Ты же ее сам и добавил. В мастере id это строка, а ты в этом PR переопределил.

Также, я пока что закомментировал код в enableCameraTracking, требующий style в качестве аргумента. Во-первых, нужно сначала определиться с архитектурой, чтобы не переделывать по десять раз. А во-вторых, ты говорил, что готов переосмыслить эту фичу - предлагаю как раз здесь этим и заняться. Как уже говорил, данный функционал является частным случаем, и может быть легко реализован пользователями плагина самостоятельно - всё для этого имеется.

Закомменченный код потом надо будет убрать. Я в целом не против убрать сам стиль из этого метода, но не тут. Сейчас оно ничем не мешает, кроме того, что ты решил полностью изменить, то как плейсмарк создается. Если стиль оставить как он был и добавить только часть с композитными иконками внутрь него, то трогать это место и не придется.

cream-cheeze commented 2 years ago

Так я не говорю, что у иконки не может быть стиля. Я говорю, что у плейсмарка есть стиль, у стиля есть набор иконок со своими стилями. Если дать пример, то вот так

Значит, я тебя сначала неправильно понял. Так я тоже не возражаю - доработаю.

Ты же ее сам и добавил. В мастере id это строка, а ты в этом PR переопределил.

Сорян, похоже при отделении кода что-то лишнее хватанул.

Закомменченный код потом надо будет убрать. Я в целом не против убрать сам стиль из этого метода, но не тут. Сейчас оно ничем не мешает, кроме того, что ты решил полностью изменить, то как плейсмарк создается. Если стиль оставить как он был и добавить только часть с композитными иконками внутрь него, то трогать это место и не придется.

Ок.

DCrow commented 2 years ago

Также, я пока что закомментировал код в enableCameraTracking, требующий style в качестве аргумента

Вот тут убрал этот стиль #155, да и то что обсуждали насчет MapObjectCollection тоже добавил. В более удобной форме.

cream-cheeze commented 2 years ago

Вот тут убрал этот стиль #155, да и то что обсуждали насчет MapObjectCollection тоже добавил. В более удобной форме.

Да, видел ветку - отлично 👍

cream-cheeze commented 2 years ago

Ты же ее сам и добавил. В мастере id это строка, а ты в этом PR переопределил.

И всё же, сейчас вижу в мастере следующее:

  /// Does nothing if passed `Placemark` wasn't added before
  Future<void> removePlacemark(Placemark placemark) async {
    if (placemarks.remove(placemark)) {
      await _channel.invokeMethod('removePlacemark', placemark.toJson());
    }
  }

Зачем передавать весь json плейсмарка для его удаления?

Изменено это было вот этим твоим коммитом.

cream-cheeze commented 2 years ago

Ещё вижу, что isDraggable и zIndex плейсмарка у тебя идут вне стиля - хочется понять, по какому принципу ты решаешь, что является атрибутом плейсмарка, а что - стиля? Вижу, что zIndex унаследован от MapObject, что повторяет нативную либу, но в нативной либе и draggable - тоже атрибут MapObject, но ты его почему-то оставил в плейсмарке.

cream-cheeze commented 2 years ago

Доработал, можно смотреть. Пока iOS only.

DCrow commented 2 years ago

Зачем передавать весь json плейсмарка для его удаления?

Тут да, не зачем, можно только id, менял для удобства работы с сериализацией объекта. После #155 этого уже не будет

Ещё вижу, что isDraggable и zIndex плейсмарка у тебя идут вне стиля - хочется понять, по какому принципу ты решаешь, что является атрибутом плейсмарка, а что - стиля? Вижу, что zIndex унаследован от MapObject, что повторяет нативную либу, но в нативной либе и draggable - тоже атрибут MapObject, но ты его почему-то оставил в плейсмарке.

zIndex унаследован, так как он в Polyline, Circle и тп изначально был. draggable был только в Placemark поэтому там его и оставил(Его вообще надо отдельно дорабатывать, так как к нему еще требуются каллбэки onDrag, onDragStart, onDragEnd) Определял по принципу, что влияет на взаимодействие с Placemark - zIndex/draggable/visible(это все влияет на onTap, координаты и тп), то и является стилем, остальное, что только визуально меняет, то и есть стиль.

cream-cheeze commented 2 years ago

Капец там опять конфликтов вылезло...

cream-cheeze commented 2 years ago

Капец там опять конфликтов вылезло...

Смержил, конфликты разрешил, вроде норм. Можно смотреть.

Правда в ходе тестирования на андроид заметил некоторые косяки в примерах не связанных с плейсмарками - переключился на мастер, чтобы проверить - там воспроизводится:

  1. Построение маршрута лагает очень сильно - интерфейс зависает на несколько секунд в примере, даже прелоадер перестаёт крутиться.
  2. В Layers example по кнопке Show user layer иконка то появляется, то нет, иногда секунд через 5-10 сама, иногда после повторного нажатия. Get user point периодически выдаёт ошибку (как раз в момент, когда иконки нет на карте): 
    E/flutter: [ERROR:flutter/lib/ui/ui_dart_state.cc(209)] Unhandled Exception: NoSuchMethodError: The method '[]' was called on null.
Receiver: null
Tried calling: []("point")
#0      Object.noSuchMethod (dart:core-patch/object_patch.dart:63:5)
#1      YandexMapController.getUserTargetPoint (package:yandex_mapkit/src/yandex_map_controller.dart:216:15)
<asynchronous suspension>
#2      _LayersExampleState.build.<anonymous closure> (package:yandex_mapkit_example/examples/layers_page.dart:80:33)
<asynchronous suspension>
  3. Search выдаёт ошибку, прелоадер продолжает крутиться: 
    E/MethodChannel#yandex_mapkit/yandex_search: Failed to handle method call
java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.Object java.util.Map.get(java.lang.Object)' on a null object reference
    at com.unact.yandexmapkit.YandexSearch.searchByText(YandexSearch.java:64)
    at com.unact.yandexmapkit.YandexSearch.onMethodCall(YandexSearch.java:42)
    at io.flutter.plugin.common.MethodChannel$IncomingMethodCallHandler.onMessage(MethodChannel.java:233)
    at io.flutter.embedding.engine.dart.DartMessenger.handleMessageFromDart(DartMessenger.java:84)
    at io.flutter.embedding.engine.FlutterJNI.handlePlatformMessage(FlutterJNI.java:865)
    at android.os.MessageQueue.nativePollOnce(Native Method)
    at android.os.MessageQueue.next(MessageQueue.java:335)
    at android.os.Looper.loop(Looper.java:206)
    at android.app.ActivityThread.main(ActivityThread.java:8512)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:602)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1130)
E/flutter: [ERROR:flutter/lib/ui/ui_dart_state.cc(209)] Unhandled Exception: PlatformException(error, Attempt to invoke interface method 'java.lang.Object java.util.Map.get(java.lang.Object)' on a null object reference, null, java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.Object java.util.Map.get(java.lang.Object)' on a null object reference
    at com.unact.yandexmapkit.YandexSearch.searchByText(YandexSearch.java:64)
    at com.unact.yandexmapkit.YandexSearch.onMethodCall(YandexSearch.java:42)
    at io.flutter.plugin.common.MethodChannel$IncomingMethodCallHandler.onMessage(MethodChannel.java:233)
    at io.flutter.embedding.engine.dart.DartMessenger.handleMessageFromDart(DartMessenger.java:84)
    at io.flutter.embedding.engine.FlutterJNI.handlePlatformMessage(FlutterJNI.java:865)
    at android.os.MessageQueue.nativePollOnce(Native Method)
    at android.os.MessageQueue.next(MessageQueue.java:335)
    at android.os.Looper.loop(Looper.java:206)
    at android.app.ActivityThread.main(ActivityThread.java:8512)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:602)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1130)
)
#0      StandardMethodCodec.decodeEnvelope (package:flutter/src/services/message_codecs.dart:607:7)
#1      MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:156:18)
<asynchronous suspension>
E/flutter: [ERROR:flutter/lib/ui/ui_dart_state.cc(209)] Unhandled Exception: PlatformException(error, Attempt to invoke interface method 'java.lang.Object java.util.Map.get(java.lang.Object)' on a null object reference, null, java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.Object java.util.Map.get(java.lang.Object)' on a null object reference
    at com.unact.yandexmapkit.YandexSearch.searchByText(YandexSearch.java:64)
    at com.unact.yandexmapkit.YandexSearch.onMethodCall(YandexSearch.java:42)
    at io.flutter.plugin.common.MethodChannel$IncomingMethodCallHandler.onMessage(MethodChannel.java:233)
    at io.flutter.embedding.engine.dart.DartMessenger.handleMessageFromDart(DartMessenger.java:84)
    at io.flutter.embedding.engine.FlutterJNI.handlePlatformMessage(FlutterJNI.java:865)
    at android.os.MessageQueue.nativePollOnce(Native Method)
    at android.os.MessageQueue.next(MessageQueue.java:335)
    at android.os.Looper.loop(Looper.java:206)
    at android.app.ActivityThread.main(ActivityThread.java:8512)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:602)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1130)
)
#0      StandardMethodCodec.decodeEnvelope (package:flutter/src/services/message_codecs.dart:607:7)
#1      MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:156:18)
<asynchronous suspension>
cream-cheeze commented 2 years ago

И ещё с кластерами что-то не так в моей ветке - пример не работает как должно, пока не понял почему. В мастере работает и на iOS в моей ветке тоже работает... Вроде я кластерный контроллер вообще не трогал.

DCrow commented 2 years ago

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

Оно и раньше так было. Объем данных очень большой.

В Layers example по кнопке Show user layer иконка то появляется, то нет, иногда секунд через 5-10 сама, иногда после повторного нажатия. Get user point периодически выдаёт ошибку (как раз в момент, когда иконки нет на карте):

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

Search выдаёт ошибку, прелоадер продолжает крутиться

Надо будет поправить

И ещё с кластерами что-то не так в моей ветке - пример не работает как должно, пока не понял почему. В мастере работает и на iOS в моей ветке тоже работает... Вроде я кластерный контроллер вообще не трогал.

У тебя не работает из-за этого. Ты обнуляешь id, и при поиске возвращаются одни null, из-за чего дальше ничего не вызывается.

cream-cheeze commented 2 years ago

У тебя не работает из-за этого. Ты обнуляешь id, и при поиске возвращаются одни null, из-за чего дальше ничего не вызывается.

Точно, заработало, спасибо.