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

[#259, 위젯 기능 추가] #260

Merged
merged 3 commits into from
Sep 10, 2023
Merged

Conversation

jeongth9446
Copy link
Contributor

Issue

Overview (Required)

[작업내용]

  • Glance Library 추가 (targetSDK 버전 제한으로 beta01 버전 적용)
  • 북마크된 세션 표시 위젯 기능 개발
    • 타이틀 클릭 시 앱 실행
    • 앱 내 세션 북마크 추가 시 위젯에 리스트 업데이트 하도록 구현 - 위젯에서 세션 클릭 시 앱 - 해당 세션 상세 정보 화면 이동 - 세션 카드 - 타이틀, 시작 ~ 종료 시간, 발표자 표시

[TODO]

  • 발표자가 2인 이상일 경우 이름 표시할 수 있도록 로직 수정 (현재 발표자가 2인 이상인 경우 없음)
  • 위젯 내 북마크 해제 버튼 추가

Links

Screenshot

Before | After
:--: | :-KakaoTalk_Photo_2023-09-06-23-20-06
-:

|

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

Test Results

18 tests   18 ✔️  6s ⏱️
11 suites    0 💤
11 files      0

Results for commit 0ad9362.

♻️ This comment has been updated with latest results.

[작업내용]
  - Glance Library 추가 (targetSDK 버전 제한으로 beta01 버전 적용)
  - 북마크된 세션 표시 위젯 기능 개발
    - 타이틀 클릭 시 앱 실행
    - 앱 내 세션 북마크 추가 시 위젯에 리스트 업데이트 하도록 구현
    - 위젯에서 세션 클릭 시 앱 - 해당 세션 상세 정보 화면 이동
    - 세션 카드 - 타이틀, 시작 ~ 종료 시간, 발표자 표시

[TODO]
  - 발표자가 2인 이상일 경우 이름 표시할 수 있도록 로직 수정 (현재 발표자가 2인 이상인 경우 없음)
  - 위젯 내 북마크 해제 버튼 추가
@laco-dev
Copy link
Contributor

laco-dev commented Sep 7, 2023

@jeongth9446
행사가 다음주로 다가오면서, 깃허브 팔로우업이 늦어진 점 죄송합니다.
병합되기 전이라도, 작업하신 내용을 바탕으로 발표자료에 참고자료로 활용하셔도 괜찮습니다.

작성해주신 내용은, 확인 후 의견 드리겠습니다.

@jeongth9446
Copy link
Contributor Author

jeongth9446 commented Sep 7, 2023

@laco-dev
PR 드린지 몇 시간 되지 않았는데도 피드백 주심에 감사드립니다 :)

Copy link
Contributor

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

감사합니다. 확인이 늦었습니다.

Galance 기능을 통해 많은 개발자들에게 좋은 레퍼런스가 될 수 있을 것 같습니다. 👍🏻
그 과정에서 몇가지 같이 개선해보면 좋을 부분이 있어서 남겨두었습니다.

@@ -41,6 +41,8 @@ androidxDatastore = "1.0.0"
ossLicenses = "17.0.1"
ossLicensesPlugin = "0.10.6"

androidxGlance = "1.0.0-beta01"
Copy link
Contributor

Choose a reason for hiding this comment

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

Glance Library 추가 (targetSDK 버전 제한으로 beta01 버전 적용)

베타를 적용함으로써 이 작업에서 처음 의도한 것과 달리 기능을 모두 드러내지 못한 부분이 있었나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

의도했던 부분을 드러내지 못한 것은 없습니다.
실제 제가 개발하고 운영중인 서비스에도 beta01 버전이 적용되어 있습니다.

Box(modifier = GlanceModifier.padding(bottom = 16.dp, end = 16.dp)) {
Column(
modifier = GlanceModifier.padding(16.dp).fillMaxWidth()
.cornerRadius(12.dp).background(GlanceTheme.colors.tertiaryContainer).clickable(
Copy link
Contributor

Choose a reason for hiding this comment

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

마이너 의견입니다)
위젯을 클릭하면 앱이 바로 실행되고 롱 클릭은 위젯 수정으로 이루어져서 클릭 효과(Ripple)이 어색하게 발생하네요

Screen_recording_20230909_210243.webm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compose 와 다르게 아직 Glance 에서는 ripple 효과에 대한 커스터마이징을 지원하고 있지 않습니다. ㅠㅠ
추후에 기능 추가가 이루어진다면 좋겠네요 ..

@@ -0,0 +1,76 @@
package com.droidknights.app2023
Copy link
Contributor

Choose a reason for hiding this comment

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

widget 모듈에서 제공하는 기능이기 때문에 widget 패키지를 suffix로 추가 해주면 좋을 것 같습니다. (이외 동일)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

com.droidknights.app2023.widget 로 수정하도록 하겠습니다. !

Comment on lines 25 to 26
fun WidgetSessionCard(session: Session) {
val context = LocalContext.current
Copy link
Contributor

Choose a reason for hiding this comment

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

이 컴포저블에 대해서 Preview를 제공해주세요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glance Component에 대한 Preview 는 아직 공식 지원하지 않는 것으로 확인하였습니다 ㅠ

https://github.com/google/glance-experimental-tools/tree/main/appwidget-host

실험적 라이브러리가 있기는 하나, 최근 업데이트가 되지 않은 라이브러리이며, glance beta01 버전과 호환도 되지 않는 것 같습니다

Comment on lines 21 to 23
color = GlanceTheme.colors.onSurface
),
modifier = GlanceModifier.clickable(
Copy link
Contributor

Choose a reason for hiding this comment

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

Glance 라이브러리에 대해 컨텍스트가 부족해서 질문 드립니다.
GlanceModifier, GlanceTheme를 사용해야 하는 이유가 있나요?

Copy link
Contributor Author

@jeongth9446 jeongth9446 Sep 10, 2023

Choose a reason for hiding this comment

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

Key Point: Glance provides a modern approach to build app widgets using Compose, but is restricted by the limitations of AppWidgets and RemoteViews. Therefore, Glance uses different composables from the Jetpack Compose UI.

https://developer.android.com/jetpack/compose/glance/build-ui

Glance 라이브러리는 Compose와 유사한 코드 스타일로 RemoteView를 개발하기 위해 개발된 라이브러리로 알고 있습니다.

Compose Compiler를 공유하지만, 근본적으로 일반 View와 RemoteView는 다르기 때문에 기존 Compose Function을 가져와서 사용하지 못하고 같은 이름의 새로운 함수들로 라이브러리가 구성되어 있습니다.

그렇기에, Glance Compoenent ( ~ RemoteView) 에 대응 하는 GlanceModifier를 사용해야 하는 것으로 알고 있고,

컬러의 경우 Glance는 ColorProvider라는 인터페이스를 활용하여 실제 화면에 표시될 컬러를 컨텍스트에 맞게 가져오도록 설계되어 있기 때문에, Color 를 반환하는 MaterialTheme 을 그대로 적용하는 데에는 무리가 있을 것으로 생각됩니다.

비슷하면서도 다른... Glance의 묘미로 봐주시면 좋을 것 같습니다 ㅎㅎ

color = GlanceTheme.colors.onSurface
),
modifier = GlanceModifier.clickable(
actionLaunchIntentForPackage(context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Widget 관련된 UI 컴포저블이 화면을 실행하기 위한 Context 및 Galance에 커플링이 생기는 아쉬움이 있는데요.
UI는 그리는 역할을 하고, 이 컴포지션이 만들어지게 된 곳에서 Galance에 대한 의존성을 같이 처리 해주면 어떨까요?

지금은 Widget 컨트롤러와 컴포저블의 역할이 조금씩 섞여있는 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어떤 말씀이신지 잘 이해하지 못했습니다 ㅠ

이해한 바에 대해 아래와 같이 코멘트 드립니다.

Composable 함수와 Glance, context 간의 커플링을 제거
--> @composable 어노테이션이 붙어있긴 하지만 해당 함수의 컴포넌트는 모두 glance의 것입니다. glance 자체가 RemoteView에 대한 UI 구현 시 코드 스타일을 compose 스럽게 할 수 있게 하는 라이브러리입니다.

context 는 해당 Glance 컴포넌트가 동작하는 컨텍스트를 가지고 와서 처리하고 있습니다. 일반적으로 Compose 구현 시에도 유사한 방식으로 처리하곤 했는데, 더 나은 방향이 있을까요?!

Copy link
Contributor

Choose a reason for hiding this comment

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

처음에는 이런 그림을 기대했었는데요
말씀주신 내용 이후로 찾아보니 정말 모든 컴포넌트가 Galance로 동작을 해야만 하는 차이가 있네요 😱
*이름이 똑같아서 Galance라는걸 알아차리지 못함

@Composable
fun WidgetTitle(
    textStyle: TextStyle,
    color: Color,
    onClick: () -> Unit,
) {
    Text(
        context.getString(R.string.widget_title),
        style = textStyle,
        modifier = Modifier.clickable {  }
    )
}

Comment on lines 23 to 25
companion object {
const val KEY_SESSION_IDS = "SESSION_IDS"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

코틀린 코딩 컨벤션에 따라 companion object의 경우 클래스 가장 하단에 위치하도록 해 주시면 좋습니다
https://kotlinlang.org/docs/coding-conventions.html#class-layout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

컨벤션을 잘 숙지해야 겠네요. 감사합니다.

@jeongth9446
Copy link
Contributor Author

jeongth9446 commented Sep 10, 2023

@laco-dev 수정 후 PR를 업데이트 하였습니다. 확인 부탁드립니다.

Copy link
Contributor

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

확인 감사합니다.
한 가지 의견 추가했습니다. 🙏🏻

@@ -17,7 +17,7 @@
tools:targetApi="31">

<receiver
android:name=".DroidKnightsWidgetReceiver"
android:name=".widget.DroidKnightsWidgetReceiver"
Copy link
Contributor

Choose a reason for hiding this comment

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

widget 모듈에서 manifest를 작성하게 되면 app 모듈에서 자연스럽게 병합이 가능할 것으로 보이는데요.
이와 관련해서도 제약사항이 따로 존재하나요?
*이 방법이라면 DroidKnightsWidgetReceiver 를 internal로 만드는 것도 가능함

<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools">

    <application tools:targetApi="31">

        <receiver
            android:name=".DroidKnightsWidgetReceiver"
            android:exported="true">
            <intent-filter>
                <action android:name="android.appwidget.action.APPWIDGET_UPDATE" />
            </intent-filter>
            <meta-data
                android:name="android.appwidget.provider"
                android:resource="@xml/widget_info" />
        </receiver>

    </application>

</manifest>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이런 식으로도 처리할 수 있군요 수정해보겠습니다 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영하였습니다

[작업내용]
  - App module에 정의되어 있던 widget receiver 별도 manifest 정의
  - 위젯 모듈 내 노출 불필요한 class internal 처리
Copy link
Contributor

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다.

별도 레퍼런스로 브랜치를 따로 생성할까 생각했는데 앱을 실제로 생성하고 확인할 때 포함되면 좋은 기능이라고 생각되어 메인 브랜치로 바로 병합하겠습니다.
대신 레퍼런스 소개하기 위한 리드미에는 이번 PR 과 같이 포함하도록 하겠습니다.

@laco-dev laco-dev merged commit 6e61505 into droidknights:main Sep 10, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

위젯 기능 추가 제안
3 participants