[HOTFIX/#193] 네비게이션 스택 수정 및 자잘한 디테일을 수정했습니다.

[angryPodo]

Related issue 🛠

• closed [HOTFIX] 스택 정상화 #193

Work Description ✏️

• 네비게이션 스택 정상화
• 이스터에그 추가
• UI 디테일 추가

To Reviewers 📢

천천히 하십시오~

[angryPodo]

이건 어디서...?

[chattymin]

앱잼 끝난지 얼마 안됐는데도 고생하셨습니다!!
크리티컬한 수정사항은 없고 의견 공유해보고 싶은 부분은 있네요 :)

누가 만들었는지 Modifier 확장함수 기깔나네요 🚀

[chattymin]

p4) 주석 위치가 이상한 것 같아요~

[Hyobeen-Park]

이게 왜 여기있지..??

[angryPodo]

내려가라~

[chattymin]

p2) navigateToExplore라는 하나의 함수를 두고 navOption을 매개변수로 받아오는 방법이 아닌 두개의 함수를 만든 이유가 있을까요??
두개의 다른 함수에서 차이점이 navOption밖에 없다면 매개변수를 받아오는 의미가 없는 것 같아요

[Hyobeen-Park]

인정합니다..!! 이거 숙소에서도 얘기가 나왔었는데 아마도 가독성때문에..?? 함수 나눴던 걸로 기억합니다(사실 잘 기억 안남ㅜ 민점맨 나와라!!) 근데 지금 다시 보니 그냥 navigateToExplore라는 함수를 두고 navOptions를 받아오는 방식이 더 나을 것 같아요

navOption을 매개변수로 받아오는 이유는 여러 곳에서 재사용할 수 있도록 하기 위함이라고 생각했는데 지금처럼 navigateRegisterToExplore / navigateMapToExplore 이런식으로 각각 함수로 나누면 다른 스크린에서 Explore로 이동할 때 또 다른 함수를 만들어야 하기 때문에 재사용성의 장점을 갖지 못한다고 생각합니다. 그리고 함수 개수가 불필요하게 너무 많아지는 것 같기도 하구요!!

[angryPodo]

지금 기억을 더듬어 보자면, 당시에 제가 함수를 분리했던 건 명확한 스택 추적을 위해서였는데, 요즘 고민하면서 보니 함수화가 단순히 재사용성만을 위한 게 아니라 네임스페이스를 부여하는 방향도 좋아보였습니다. 그래서 현재 구조가 꽤 괜찮지 않나 하는 생각도 들었는데요,

하지만 이 케이스를 좀 더 깊게 생각해보면, navOption 말고는 다른 파라미터가 없는 상황에서 함수를 분리하는 게 오히려 과도한 추상화가 될 것 같네요. 그래서 단일 함수에 파라미터를 받는 방식이 더 깔끔할 것 같습니다.

물론 앞으로 사용자 정보 같은 추가 파라미터가 필요해진다면, 그때 가서 함수를 분리하는 것도 좋은 방법이 될 것 같네요! 지금은 심플하게 가는 게 더 나아 보입니다 😊

[Roel4990]

제 생각에도 navOption 을 받아옴으로서 분기 여러 함수를 처리할 분기 처리가 가능하다면 확장성을 생각해서라도 더 좋을 듯 싶습니다. 해당 navOption 을 parameter 으로 보내면 너무 복잡해지지 않을까 걱정했던 부분이 있었지만 크게 문제가 되진 않을 것 같습니다.

[chattymin]

p4) 추후 활용을 위해 남겨둔 주석인가요?
맞다면 코멘트를 추가로 작성해주시고, 아니라면 지워주세욥~

[angryPodo]

이거 이중호출 되던 문제였던가..???????????? userId가 사용자 본인 id로 반환됐던 문제가 기억이 나는데,, 요건 요청 쏴보고 수정하겠습니다!

[Hyobeen-Park]

아 이거 서버에서 글쓴이 userId를 받고 난 이후에 함수를 호출해야 해서 뷰모델 init 안에서는 없어야 해요!!

[Roel4990]

제가 실수한 부분입니다..!

[Roel4990]

와우 디테일 수정,,, 수고하셨습니다! 제가 실수한 부분까지 수정해주셨네요.. 감사합니다!

[Hyobeen-Park]

와 벌써 작업하는 열정!! 멋있다!!!!

저도 크리티컬하다기보다는 의견 나누고 싶은 부분이 있어서 코리 잔뜩 남기고 갑니다ㅎㅎ

[Hyobeen-Park]

이게 왜 여기있지..??

[Hyobeen-Park]

P5: 와 한 줄 바로 적용되는거 대박이다

[Hyobeen-Park]

인정합니다..!! 이거 숙소에서도 얘기가 나왔었는데 아마도 가독성때문에..?? 함수 나눴던 걸로 기억합니다(사실 잘 기억 안남ㅜ 민점맨 나와라!!) 근데 지금 다시 보니 그냥 navigateToExplore라는 함수를 두고 navOptions를 받아오는 방식이 더 나을 것 같아요

navOption을 매개변수로 받아오는 이유는 여러 곳에서 재사용할 수 있도록 하기 위함이라고 생각했는데 지금처럼 navigateRegisterToExplore / navigateMapToExplore 이런식으로 각각 함수로 나누면 다른 스크린에서 Explore로 이동할 때 또 다른 함수를 만들어야 하기 때문에 재사용성의 장점을 갖지 못한다고 생각합니다. 그리고 함수 개수가 불필요하게 너무 많아지는 것 같기도 하구요!!

[Hyobeen-Park]

P4: MainTab.MAP 이랑 Map() 이렇게 하는거랑 어떤 점에서 차이가 있을까요..?? 현재 두 가지 방식이 같이 사용되고 있는데 하나로 통일시키면 좋을 것 같아요..!!

[angryPodo]

음,, 굳이 따지자면 반환하는 객체의 차이는 있네요. MainTab.MAP은 여러 정보를 담은 enum객체인데 Map()은 순수 라우트만 포함하는 객체인디, 결국 라우트를 표현하는 방식의 차이지만 명시한 route인 Map()으로 통일하는게 좋겠습니다:)

[angryPodo]

소소팁:
MainTab.MAP은 객체를 앱 시작시 생성 후 재사용
Map()은 호출할 때마다 새로운 객체 생성 가능, GC대상이 될수도?

결론은 정~말 미미하지만 Enum Class가 성능상 더 유리하다네요~~

[Hyobeen-Park]

Suggested change

	navOptions: NavOptions = navOptions {
	popUpTo(MainTab.MAP) {
	inclusive = true
	}
	launchSingleTop = true
	}
	navOptions: NavOptions = navOptions {
	popUpTo(Register) {
	inclusive = false
	}
	launchSingleTop = true
	}

P3: 등록뷰로 이동하는 화면이 Map만 있는 것은 아니니까 popUpTo(Register)로 하고 inclusive를 false로 해주는게 더 맞지 않을까 싶은데 어떤가요??

[Hyobeen-Park]

Suggested change

	navOptions: NavOptions = navOptions {
	popUpTo(MainTab.MAP) {
	inclusive = true
	}
	launchSingleTop = true
	}
	navOptions: NavOptions? = null

P3: 그리고 MainNavigator에서 이미 navOpotions를 주고 있기 때문에 여기에서는 navOptions는 없어도 될 것 같아요!!

[angryPodo]

일단 먼저 첫번째 방식은 기대하는 동작과 다르게 될 것 같아요. 지금 navigateToRegister의 시나리오 하나를 가정해보자면, Map -> Explore -> Map -> Register로 이동 시에 불러오는 함수인데, 이렇게 하면 Register는 아직 스택에 없는 상태이므로 popUpTo 동작이 아무 효과가 없게 된다고 예상이 됩니다. :0

지금 작성되어있는 MainTab.MAP또한 MAP을 경유하지 않으면 쓸모가 없네요 :(
때문에 저희가 의도했던 모든 백스택 제거를 하려면 다음처럼 수정해볼 수 있을 것 같아요

fun NavController.navigateToRegister(
    navOptions: NavOptions = navOptions {
        popUpTo(0) { inclusive = true }  // 모든 백스택을 완전히 제거
        // 또는 아래도 같은 동작입니다.
        // popUpTo(navController.graph.startDestinationId) { inclusive = true }
        launchSingleTop = true 
    }
) {
    navigate(Register, navOptions)
}

이렇게 하면 어떤 경로로 Register로 이동하든 이전 백스택이 모두 제거되어 Register에서 뒤로가기 시 앱이 종료됩니다.

[angryPodo]

두번째로, MainNavigator에서 이미 navOptions를 주고 있습니다! 따라서 기본값으로 지정하지 않아도 될 것 같은데, 다만 이런 생각이 들었습니다.

기능명세서에서 Register 화면에서 뒤로가기 시 앱이 종료되어야 한다는 기본 동작이 명시되어 있는데요, 이를 보장하기 위해 기본값으로 남겨두는 것은 어떨까요?

물론 지금은 Register로 이동하는 호출부가 두 군데뿐이라 기본값 없이 각 호출부에서 필요한 navOptions를 직접 지정하는 것도 충분해 보이기도 하고.. 여러 의견을 들어보고 싶습니다 :)

[angryPodo]

@chattymin @Roel4990 @Hyobeen-Park

[Hyobeen-Park]

헉스 맞네요 popUpTo(0)으로 하는게 맞네요ㅎㅎ 예리했다!!

그리고 두 번째 이슈에 대해서는 현재 기능명세서 상 Register 화면에서 뒤로가기 시 앱은 무조건 종료되기 때문에 기본값을 넣어주는 것도 정말 좋은 생각입니다!! 다만 그렇다면 저희는 화면을 이동해도 state가 유지되도록 구현했기 때문에 saveState / restoreState도 함께 해주어야 기본값을 지정했을 때 저희가 의도한대로 될 것 같습니다

또한 mainNavigator에 스택을 없애기 위해 navOption을 전달하고 있는데 이 부분들은 불필요하지 않을까 하는 생각이 드네요!! 스택관리를 위한 navOptions를 기본값으로 지정했는데 mainNavigator에서 navOptions를 직접 지정해주면 사실 같은 navOptions를 두 번 지정하는 것이라는 생각이 들어서요..!! 이 부분에 대해서는 어떻게 생각하시는지 의견이 궁금합니다!

[chattymin]

1. popUpTo(0) 죠씁니다~~ 그런데 0으로 popUp한다와 startDestination으로 popUp한다 중 후자가 더 가독성 측면에서 좋을 것 같긴 해요
2. 말씀해주신 것 처럼 Register화면의 기능 명세상 뒤로가기를 누르면 화면이 종료되어야 한다는 것이 고정이라면, 효빈님 말씀처럼 기본값을 설정하고 navOption을 넘겨주지 않는 방향으로 가는게 좋아보입니다. 매번 넣어주기보다는 기본값으로 있는게 더 적합해 보이네요

[angryPodo]

위의 대화 이후 생각해보니, Register 화면으로의 네비게이션을 extension 함수에서 기본값으로 강제하기보다는 호출하는 쪽에서 제어하도록 하는 게 더 좋을 것 같았습니다.

fun NavController.navigateToRegister(
    navOptions: NavOptions? = null
) {
    navigate(Register, navOptions)
}

이렇게 수정한 이유는 나중에 Register 화면의 네비게이션 동작을 수정해야 할 때도 호출하는 쪽만 수정하면 되니 유지보수도 더 쉬워질 것 같습니다.

물론 Register 화면에서는 뒤로가기 시 앱이 종료되어야 한다는 요구사항이 있지만, 이건 각 호출부에서 상황에 맞게 처리하는 게 더 명확할 것 같네요.