Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: iOS 에서 getMeterPerDp 사용시 생기는 크래시 수정 #125

Merged
merged 6 commits into from Oct 21, 2023

Conversation

Soogyo-In
Copy link
Contributor

@Soogyo-In Soogyo-In commented Oct 19, 2023

fixes #85

원인

이슈 제보하신 분이 보내주신 로그를 보니 옵셔널 값을 언래핑하면서 nil 이 반환되어 생긴걸로 확인되었습니다.

변경사항

  • _NaverMapControlSender.getMeterPerPixelAtLatitude 함수를 추가했습니다.
    아무래도 간접적인 getMetersPerDp 함수에 파라미터들을 nullable 로 두어 간접적으로 오버로딩의 형태를 의도하신 것 같은데 파라미터가 두 개라 하나만 값이 제공될 때 모두 제공될거라는 보장이 없습니다. 따라서 파라미터가 있는 함수, 없는 함수 각각을 명확하게하는게 좋겠다고 생각했습니다. 함수명은 네이버 지도 iOS SDK 의 API reference 의 metersPerPixelAtLatitude:zoom: 에서 따왔습니다.
    • 버전 하위 호환성을 위해 getMetersPerDp 의 함수 시그니처를 변경하지 않았습니다. PR 이 반영된다면 파라미터에 Deprecated 어노테이션을 다는게 어떨까 싶습니다.
  • Flutter 코드 변경에 맞춰 Swift 코드의 플랫폼 메서드 매핑 코드를 변경했습니다.
    • getMeterPerPixelAtLatitude 를 추가했습니다.
    • 문제가 됐던 getMetersPerDp 의 옵셔널 언래핑을 제거했습니다.
    • 버전 하위 호환성을 위해 latitude, zoom 파라미터가 모두 전달되었다면 오버로딩된 NMFProjection.metersPerPixel(atLatitude latitude: Double, zoom: Double) 함수를 사용하도록했습니다.

확인 방법

example/main.dartonMapReady 함수에 아래와 같이 코드를 첨부하고 실행하면 확인하실 수 있습니다.

  void onMapReady(NaverMapController controller) async {
    mapController = controller;

    final position = await controller.getCameraPosition();
    print('[###] zoom: ${position.zoom}'); //[###] zoom: 14.0

    double value = 0.0;

    // 수정 전에는 여기서 에러가 발생합니다.
    value = await controller.getMeterPerDp();
    print('[###] getMetersPerDp: $value'); // [###] getMetersPerDp: 3.786541713960559
    value = await controller.getMeterPerDp(latitude: 72.0);
    print('[###] getMetersPerDp with only latitude: $value'); // [###] getMetersPerDp with only latitude: 3.786541713960559
    value = await controller.getMeterPerDp(zoom: 13.0);
    print('[###] getMetersPerDp with only zoom: $value'); // [###] getMetersPerDp with only zoom: 3.786541713960559

    // 아래 두 요청은 같은 값을 반환합니다.
    value = await controller.getMeterPerDp(latitude: 72.0, zoom: 13.0);
    print('[###] getMetersPerDp with params: $value'); // [###] getMetersPerDp with params: 2.952542592454751
    value =
        await controller.getMeterPerDpAtLatitude(latitude: 72.0, zoom: 13.0);
    print('[###] getMeterPerDpAtLatitude: $value'); // [###] getMeterPerDpAtLatitude: 2.952542592454751
  }

@Soogyo-In Soogyo-In changed the title fix: iOS 에서 getMeterPerDp 사용시 생기는 크래시 수정 (#85) fix: iOS 에서 getMeterPerDp 사용시 생기는 크래시 수정 Oct 21, 2023
@note11g
Copy link
Owner

note11g commented Oct 21, 2023

안녕하세요. 먼저, 기여에 진심으로 감사드립니다.
수정의 방향은 매우 좋은 것 같습니다. 해당 부분은 설계 당시에 미처 고려하지 못했던 부분인 것 같습니다.
저 역시도, 헷갈리지 않도록 하는게 맞다고 생각합니다.

다만, 함수(메서드)의 이름과 관련해서 getMeterPerPixelAtLatitude 대신, getMetersPerDpWithLatitude로 변경을 부탁드립니다.
실제 값은 Pixel 단위가 아닌, LogicalPixel (dp) 단위로 반환되기 때문입니다. 이는 pixel이라는 단위가 Flutter에서 사용되지 않는만큼, 주의가 필요한 부분입니다.

그리고 수정사항을 확인해 보았을 때는, Android Native 측 코드가 없는 것으로 확인되는데 이 부분은 제가 직접 구현을 하면 되는 부분일지 여쭙고 싶습니다.

다시 한번 진심으로 기여해주심에 감사 말씀 남깁니다. 좋은 주말되세요.

@Soogyo-In
Copy link
Contributor Author

Soogyo-In commented Oct 21, 2023

@note11g

그리고 수정사항을 확인해 보았을 때는, Android Native 측 코드가 없는 것으로 확인되는데 이 부분은 제가 직접 구현을 하면 되는 부분일지 여쭙고 싶습니다.

아 안드로이드에도 추가하는 걸 잊었네요. 추가 커밋하겠습니다.

다만, 함수(메서드)의 이름과 관련해서 getMeterPerPixelAtLatitude 대신, getMetersPerDpWithLatitude로 변경을 부탁드립니다. 실제 값은 Pixel 단위가 아닌, LogicalPixel (dp) 단위로 반환되기 때문입니다. 이는 pixel이라는 단위가 Flutter에서 사용되지 않는만큼, 주의가 필요한 부분입니다.

pixel -> dp 는 수정하도록 하겠습니다. getMeterPerDpAtLatitude 로 변경하려 하는데 어떻게 생각하시나요? 조금 달라진 이유는 아래와 같습니다. 굳이 함수명에 대해 제안을 드리는 이유는 API 함수명은 한 번 정해두면 이름 변경하기가 까다로워서입니다.

  • meter 가 단수형과 복수형이 섞여서 쓰이는 건 좋지않은 것 같습니다. 저도 처음엔 meters 로 하려 했으나 단수형으로 적혀있어 변경하지 못했고 기존 함수명을 지우자니 breaking change 가 생기기 때문에 일단 meter 로 통일했습니다.
  • withLatitude 가 조금 어색하다는 생각이 듭니다. atLatitude 를 유지하는게 좋지 않을까 생각합니다. 사실 영어에는 자신이 없어 chatGPT 에게 물어본 내용을 첨부합니다.ㅎㅎ;
"Temperature with latitude"와 "temperature at latitude" 중 어떤 표현이 더 자연스러운지는 맥락에 따라 달라질 수 있습니다. 

1. "Temperature with latitude": 이 표현은 일반적으로 지구상의 위도와 관련된 온도 변화를 설명할 때 사용됩니다. 예를 들어, "Temperature tends to decrease with latitude as you move away from the equator" (온도는 적도에서 멀어질수록 위도에 따라 감소하는 경향이 있습니다)와 같이 사용될 수 있습니다.

2. "Temperature at latitude": 이 표현은 특정 위도에서의 온도를 나타낼 때 사용됩니다. 예를 들어, "The temperature at latitude 40 degrees North is typically colder in the winter" (북위 40도에서의 온도는 일반적으로 겨울에 냉하다)와 같이 사용될 수 있습니다.

두 표현은 문맥에 따라 자연스러울 수 있으며, 어떤 표현을 선택할지는 어떤 정보를 전달하려는지에 따라 달라집니다.

@note11g
Copy link
Owner

note11g commented Oct 21, 2023

with 부분은 잠시 latitude, zoom을 모두 포함하려다가 (withLatitudeAndZoom) 네이버 맵 라이브러리를 따라가는 것이 맞을 것 같아, 지웠었는데 꼼꼼히 확인하지 못했었네요. 말씀주신대로 AtLatitude가 더 바람직해보입니다.
그리고, 복수형 역시 괜찮아보이긴 하는데.. 제가 어떤 의도로 작성했는지는 정확히 기억이 나지는 않습니다만, 함수명 통일을 위해서 meter 단수형으로 결정하는 것이 나아보이네요. 지적해주셔서 감사드립니다.
제안주신대로 getMeterPerDpAtLatitude로 수정해주시면 될 것 같습니다.
감사합니다.

@note11g note11g changed the base branch from main to dev-1.0.3 October 21, 2023 09:26
Copy link
Owner

Choose a reason for hiding this comment

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

@Soogyo-In UIScreen.main.scale으로 나누지 않아도 mapView.projection.metersPerPixel 메서드에서 dp(Logical pixel) 단위로 반환되나요? 궁금합니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Soogyo-In
Copy link
Contributor Author

Soogyo-In commented Oct 21, 2023

@note11g
안드로이드 쪽 구현 중 이상한 iOS 와 android 의 결과값이 다른 현상을 발견했습니다. 확인해보니 iOS 네이티브 코드에서 얻어온 미터값을 scaleFactor 로 나누어줘서 생기는 현상이었습니다. 하지만 여기서 의아한 점은 안드로이드는 getMetersPerDp 함수를 이용해 dp 당 미터값을 가져오고 iOS 는 metersPerPixel 함수를 이용해 px 당 미터값을 가져오는데 두 값이 같다는 겁니다; 네이버지도 iOS SDK 의 함수명과 주석으로 보아선 metersPerPixel 의 값에 scaleFactor 를 곱해주어야할 것 같지만 그렇게 하면 안드로이드의 metersPerDp 에 scaleFactor 를 곱한 값이 나오게됩니다. 아무래도 네이버 쪽에서 기존엔 px 당 미터값을 반환하다가 중간에 pt 당 미터값을 반환하도록 수정하면서 함수명은 바꾸지 않은 것 같습니다. 그래도 이 부분은 한 번 확인해보시고 반영하시는게 좋을 것 같습니다.

추가적으로 기존의 로직이 metersPerPixel 을 scaleFactor 로 나두도록 되어있었는데 px 당 미터를 dp 당 미터로 바꾸어야하니 원래라면 곱하는게 맞습니다.

@note11g
Copy link
Owner

note11g commented Oct 21, 2023

이상한 현상이네요 ㅠㅠ..
알겠습니다. 테스트 후 반영을 위해, 다른 branch로 merge 하겠습니다. 정말 감사드립니다.

@Soogyo-In
Copy link
Contributor Author

Soogyo-In commented Oct 21, 2023

넵, 이미 확인하셨을 것 같은데 일단 iOS 에서 반환하는 결과값은 Android 에서 반환하는 값과 같도록 변경했습니다. 양 플랫폼에서 같은 결과를 반환하는게 맞다고 생각해서요.

@note11g note11g changed the base branch from dev-1.0.3 to pr/#125 October 21, 2023 12:02
@note11g note11g merged commit 6533ca4 into note11g:pr/#125 Oct 21, 2023
@Soogyo-In Soogyo-In deleted the fix/85-get-meter-per-dp-crash branch October 21, 2023 12:05
note11g added a commit that referenced this pull request Oct 22, 2023
Fix #85 (apply PR #125 adaptive)
@note11g note11g mentioned this pull request Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS에서 getMeterPerDp를 실행하는 경우, 크래시가 발생함
2 participants