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

refactor pc player panel #634

Merged
merged 5 commits into from
Jan 20, 2025
Merged

refactor pc player panel #634

merged 5 commits into from
Jan 20, 2025

Conversation

ErBWs
Copy link
Contributor

@ErBWs ErBWs commented Jan 19, 2025

要修改的东西非常多,目前只改了 PC 端的界面,先传上来 review 一下,多平台的适配后面再慢慢改,有可能需要复制一份 player_item_panel.dart 单独展示手机端的界面

PopUpMenuButton 官方文档推荐改为使用 MenuAnchor,我试了一下 hover 的 highlight 看着好看了很多,于是我全部改了(

1.mp4
2.mp4

@Predidit
Copy link
Owner

新的界面真的非常漂亮

目前的代码看上去质量也相当不错

感谢你的工作

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

稍微修改了一下,本来想在 video_page.dart 做弹幕开关的,但是有不少 bug,干脆不做了(

simulator_screenshot_0DFA69A6-D91D-4E19-9F5E-A1D8BAC36F2F

simulator_screenshot_B92D5E3E-27B8-42F8-B469-903F89A2DEBC

image image image

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

iPad

simulator_screenshot_935FC010-AB11-4367-9B02-0EFF84786231

@ErBWs ErBWs marked this pull request as ready for review January 20, 2025 08:49
@Predidit
Copy link
Owner

这个 PR 太庞大了,我可能需要一些时间来审核

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

确实改动很大,我自己看着都头大

@Predidit
Copy link
Owner

我们现在还需要在弹幕发送成功后显示 toast 吗

似乎会遮挡面板

@Predidit
Copy link
Owner

我们为什么需要在 smallest_panel.dart 中重新复制一遍 sendDanmaku 函数

这个函数不是已经传入 player_item.dart 了吗

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

没删干净(

@Predidit
Copy link
Owner

抱歉, sendDanmaku 的相关问题是我看错了

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

我们现在还需要在弹幕发送成功后显示 toast 吗

似乎会遮挡面板

是会遮挡,感觉可以去掉

@Predidit
Copy link
Owner

那我们可以去掉它

Github 网页面板查看这种庞大的 PR 不是很好用,容易滚动到其他的文件

我拉到本地慢慢看

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

抱歉, sendDanmaku 的相关问题是我看错了

我也看错了(

diff 太多被折叠了,我直接看到的 video_page

可以考虑利用 GitHub 的 review comment 评论,可以少打点文件名和函数名

@Predidit
Copy link
Owner

这个PR在我看来质量很高,已经没有其他的问题

在解决 toast 问题之后我想我们就可以进行合并

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

目前弹幕输入框提示是照搬的 B 站,我总记得以前 B 站弹幕提示是随机的,本来也想写随机但是我想不出写什么好

@Predidit Predidit merged commit 577b57b into Predidit:main Jan 20, 2025
6 checks passed
@Predidit
Copy link
Owner

@ErBWs

在实际测试编译产物之后,我发现了两个问题

  1. 默认弹幕开启功能被破坏,现在在设置选项里打开相关选项并不生效

  2. MenuAnchor 会随着 播放器控制面板的隐藏动画移动,并最终移出屏幕外

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 20, 2025

  1. 弹幕大概是我删多余代码的时候误删了,那个获取 settingkey 的函数可能被我误删了。
  2. 有具体触发方法吗?我有观察到会随隐藏面板移动,不过没观察到移出屏幕。我看看有没有办法优化一下面板隐藏的逻辑。

@Predidit
Copy link
Owner

  1. 在弹幕可以正常默认启用之后,这个地方需要修改为在播放器加载完成后才允许发送弹幕,在解析时允许发送弹幕会导致问题 (仅移动设备,桌面设备加载时有遮罩层阻止发送弹幕)

  2. 超分辨率选项就可以,总之会随之移动不符合预期

  3. 同样由 MenuAnchor 导致的问题,在桌面设备上,之前我们可以符合直觉地在已经有 menu 打开的情况下,点击画面其他地方来关闭 menu 。现在上面的行为会触发播放器暂停

如果我们实在解决不了 MenuAnchor 相关问题,可以回退之前的实现

@bggRGjQaUbCoE
Copy link
Contributor

MenuAnchor 使用体验没有比 PopUpMenuButton 好,因为没有展开收起动画
MenuAnchor animations broken after 3.19

@bggRGjQaUbCoE
Copy link
Contributor

MenuAnchor 使用体验没有比 PopUpMenuButton 好,因为没有展开收起动画 MenuAnchor animations broken after 3.19

PopUpMenuButtoninitialValue 也可以突出当前状态值

@ErBWs
Copy link
Contributor Author

ErBWs commented Jan 23, 2025

动画可以等 flutter 后续更新修复。

MenuAnchor 是现在官方的推荐实现,用来代替 PopUpMenuButton,PopUpMenuButton 目前看来不符合 Material 3 规范。

MenuAnchor 的接口更加完善,可以很方便的支持二级菜单,比如手机端现在就需要用到 MenuAnchor 的二级菜单。视觉效果更加协调。尽管缺失动画但我认为切换到 MenuAnchor 是更好的实现。

目前只有规则管理界面还在使用 PopUpMenuButton。不过那个界面没有什么视觉上的不协调。

实际上上面提到的 issue 中也给出了一种 workaround,用 AnimationController 手动实现展开效果

@bggRGjQaUbCoE
Copy link
Contributor

动画可以等 flutter 后续更新修复。

MenuAnchor 是现在官方的推荐实现,用来代替 PopUpMenuButton。

MenuAnchor 的接口更加完善,可以很方便的支持二级菜单,比如手机端现在就需要用到 MenuAnchor 的二级菜单。视觉效果更加接近普通的 TextButton,paddings 也更加统一,和播放器界面其他的 IconButton 看着更加协调。尽管缺失动画但我认为切换到 MenuAnchor 是更好的实现。

目前只有规则管理界面还在使用 PopUpMenuButton。不过那个界面没有什么视觉上的不协调。

那有的等,虽说 PopUpMenuButton 也可以手动实现二级菜单

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.

3 participants