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

Migrate to Jetpack Compose #16

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

JingYiJun
Copy link

No description provided.

dependencies:
use Hilt instead of koin for better performance
use Ktor, Ktorfit, CIO instead of OkHttp and Retorfit for overall async network
use DataStore, EncryptedDataStore instead of SharedPreference for async preference load and store
use Version Catalog, kts script, java 17
remove PermissionX, AboutPage

style:
Single Activity architect. Jetpack Compose Navigation for routing.
@w568w w568w self-requested a review February 8, 2024 15:49
@w568w w568w self-assigned this Feb 8, 2024
@w568w
Copy link
Member

w568w commented Feb 8, 2024

Thanks for your wonderful work! This is a huge PR, and I need more time to review it during holiday.

Copy link
Member

@w568w w568w left a comment

Choose a reason for hiding this comment

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

Having not reviewing all features and layouts carefully, but mostly LGTM. I suggest adding some tests to ensure stability.

I will approve it after playing with it and finding no errors.


旦夕是面向多类型账户系统的,不应想当然地把 `PersonInfo` 视作只包含 UIS 账号密码的数据类型,也不要在设置中过多出现「复旦」有关的字样
旦夕是面向多类型账户系统的,`PersonInfo` 是只针对 UIS 账号密码的数据类型。
Copy link
Member

Choose a reason for hiding this comment

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

Is this sentence contradicting itself? Why does PersonInfo only contain Fudan's UIS information if we are aiming at a general-use account? Are there other PersonInfo-like data classes in the future? Clarify it.

Copy link
Author

Choose a reason for hiding this comment

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

There should not be any general-use account. UIS account UISInfo and FDUHole account OTJWTToken should be kept separate since they can be logged in and logged out independently. Also, any data class like PersonInfo, which I guess means detailed info of an account, should load from network every time app open. Only those info binding to an account token or credential, like id, name, can be stored in persistent storage.

I will update this sentence.

android:exported="false"/>
<activity
android:name=".SingleFragmentActivity"
android:exported="false" /> <!-- Opt-out the usage statistics collection by Google. -->
Copy link
Member

Choose a reason for hiding this comment

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

Keep this comment about android.webkit.WebView.MetricsOptOut.

Copy link
Author

Choose a reason for hiding this comment

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

finished, may some bug of GitHub

Comment on lines -18 to -21
/**
* @return a list whose size is 6.
* The sequence is 文科馆、理科馆、医科馆1-6层、张江馆、江湾馆、医科馆B1
*/
Copy link
Member

@w568w w568w Feb 8, 2024

Choose a reason for hiding this comment

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

Keep this comment for future reference.

if (showPermissionDialog) {
RequestSinglePermissionDialog(
permission = Manifest.permission.ACCESS_COARSE_LOCATION,
rationale = "我们向您请求大致位置权限,是因为当前页面正在调用 Geolocation API 来获知您的大致位置。",
Copy link
Member

Choose a reason for hiding this comment

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

I18n?

@JingYiJun JingYiJun requested a review from w568w February 24, 2024 16:35
@w568w
Copy link
Member

w568w commented Feb 25, 2024

I cannot log in UIS; it shows "Channel was closed".

IMG_20240225_122423

Edit: the problem is on my side. I am using a custom DNS resolver (Adguard) and it resolves the server address to an IPv6 address, which is unsupported in our LAN.

@JingYiJun
Copy link
Author

I cannot log in UIS; it shows "Channel was closed".

多试几次呢

@w568w
Copy link
Member

w568w commented Feb 25, 2024

I cannot log in UIS; it shows "Channel was closed".

多试几次呢

多试几次也没用,是瞬间失败,没有过程。网络流量里看到有连接过 <uis.fudan.edu.cn>。

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.

2 participants