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

[Optimize] Migrate to use Settings & SwiftUI #212

Merged
merged 7 commits into from
Dec 29, 2023

Conversation

Kyle-Ye
Copy link
Collaborator

@Kyle-Ye Kyle-Ye commented Oct 30, 2023

image

Less and clean code :)

@tisfeng
Copy link
Owner

tisfeng commented Nov 3, 2023

你好,最近可能要发一个简单的修复版本,为避免影响,建了一个 swift-migration 分支,后续关于 swift 的迁移代码可以推到这个分支上。

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Nov 6, 2023

你好,最近可能要发一个简单的修复版本,为避免影响,建了一个 swift-migration 分支,后续关于 swift 的迁移代码可以推到这个分支上。

好的,那我把相关 PR 的 target 都从 tisfeng/dev 改到 tisfeng/swift-migration,等时机合入后再统一 tisfeng/swift-migration 合入 tisfeng/dev?

@tisfeng
Copy link
Owner

tisfeng commented Nov 6, 2023

是的,目前只有你这两个在进行中的 PR 需要转到 swift-migration,其他我已经弄过来了。

This was referenced Nov 19, 2023
@Kyle-Ye Kyle-Ye marked this pull request as ready for review December 28, 2023 16:49
@Kyle-Ye Kyle-Ye requested a review from tisfeng December 28, 2023 16:50
@Kyle-Ye Kyle-Ye changed the title [WIP][Optimize] Migrate to use Settings & SwiftUI [Optimize] Migrate to use Settings & SwiftUI Dec 28, 2023
@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Dec 28, 2023

This is ready for a merge. I'd like to get this merged to avoid future conflict since the new Key Config GUI depends on this PR. @tisfeng

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Dec 28, 2023

It is guarded by a beta toggle "Enable New App". So the impact will be minimal.

@Kyle-Ye Kyle-Ye requested a review from tisfeng December 29, 2023 02:40
@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Dec 29, 2023

@tisfeng 这块还有什么其他建议吗?看下是哪边先合入(要不这个先合,然后你帮忙解决下冲突?)

@tisfeng
Copy link
Owner

tisfeng commented Dec 29, 2023

@tisfeng 这块还有什么其他建议吗?看下是哪边先合入(要不这个先合,然后你帮忙解决下冲突?)

没什么问题,就是新的菜单项和设置页内容还不完整,直接对外开放不太好,要不我们先在 debug 环境启用,等后面完善了开放?

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Dec 29, 2023

可以呀,不完整是符合预期的,目前没有预期让大部分用户使用。

现在已经有一个 Beta 开关,默认关闭,需要用户手动开启才会进入,所以你是想咱们只在 Debug 包里才展示 Beta 开关吗?

@tisfeng
Copy link
Owner

tisfeng commented Dec 29, 2023

可以呀,不完整是符合预期的,目前没有预期让大部分用户使用。

现在已经有一个 Beta 开关,默认关闭,需要用户手动开启才会进入,所以你是想咱们只在 Debug 包里才展示 Beta 开关吗?

不不,这里还是用 #if DEBUG 吧

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Dec 29, 2023

不不,这里还是用 #if DEBUG 吧

没太理解你的request change诉求,是希望再所有代码处都用 #if DEBUG 包裹吗?还是在仅 DEBUG 下展示 BETA 开关即可。按照后者修改了下

建议直接在对应代码处 comment

@tisfeng
Copy link
Owner

tisfeng commented Dec 29, 2023

没问题,就是这样,是我没说清楚。

因为目前设置页还未完成,暂时还不想暴露给用户,所以在 DEBUG 环境我们自己测试使用就好。

而 Beta 项主要面向用户测试一些新功能,这些功能也都是完整的。

@tisfeng
Copy link
Owner

tisfeng commented Dec 29, 2023

这个我就先合并了,既然有了一部分基础,设置页也相对独立,后面我们就先将设置页的代码都迁移到 Swift/SwiftUI。

@tisfeng tisfeng merged commit 89bc797 into tisfeng:dev Dec 29, 2023
2 checks passed
@Kyle-Ye Kyle-Ye deleted the optimize/settings branch December 29, 2023 12:08
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