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

feat: bili danmaku #706

Closed
wants to merge 4 commits into from
Closed

Conversation

bggRGjQaUbCoE
Copy link
Contributor

@bggRGjQaUbCoE bggRGjQaUbCoE commented Feb 9, 2025

支持检索哔哩哔哩弹幕

Signed-off-by: bggRGjQaUbCoE <[email protected]>
Signed-off-by: bggRGjQaUbCoE <[email protected]>
Signed-off-by: bggRGjQaUbCoE <[email protected]>
@Predidit
Copy link
Owner

Predidit commented Feb 9, 2025

感谢你的PR

我阅读了这个PR的内容并进行了测试,代码质量相当高

但bili弹幕相关功能增加了复杂度的同时没有带来明显的好处,B站从几年前开始新番就已经非常少了

你愿意就弹幕偏移功能进行单独PR吗,可以的话可以放置到 弹幕设置的 bottom_sheet 中

@bggRGjQaUbCoE
Copy link
Contributor Author

感谢你的PR

我阅读了这个PR的内容并进行了测试,代码质量相当高

但bili弹幕相关功能增加了复杂度的同时没有带来明显的好处,B站从几年前开始新番就已经非常少了

你愿意就弹幕偏移功能进行单独PR吗,可以的话可以放置到 弹幕设置的 bottom_sheet 中

OK,那先revert弹幕偏移

@bggRGjQaUbCoE
Copy link
Contributor Author

感谢你的PR

我阅读了这个PR的内容并进行了测试,代码质量相当高

但bili弹幕相关功能增加了复杂度的同时没有带来明显的好处,B站从几年前开始新番就已经非常少了

你愿意就弹幕偏移功能进行单独PR吗,可以的话可以放置到 弹幕设置的 bottom_sheet 中

新番无所谓,主要是我这dandanplay很多番剧都显示获取弹幕番剧ID失败

@Predidit
Copy link
Owner

Predidit commented Feb 9, 2025

那个可能是网络问题,并且这个PR似乎破坏了 dandanplay 的弹幕获取,不勾选bilibili弹幕就会获取失败。

@bggRGjQaUbCoE
Copy link
Contributor Author

那个可能是网络问题,并且这个PR似乎破坏了 dandanplay 的弹幕获取,不勾选bilibili弹幕就会获取失败。

debug倒没问题,pr编译的就不行

@bggRGjQaUbCoE
Copy link
Contributor Author

那个可能是网络问题,并且这个PR似乎破坏了 dandanplay 的弹幕获取,不勾选bilibili弹幕就会获取失败。

PR编译的问题

@Predidit
Copy link
Owner

Predidit commented Feb 9, 2025

但是不包含这个PR的CI产物似乎是没问题的

@bggRGjQaUbCoE
Copy link
Contributor Author

但是不包含这个PR的CI产物似乎是没问题的

这个也获取不了
https://github.com/Predidit/Kazumi/actions/runs/13194957885

@Predidit
Copy link
Owner

Predidit commented Feb 9, 2025

我的问题,在 PR workflow 中注入发布版本的 dandnaplay 密钥

当 PR 来自分支仓库时,会获取不到 secrets 中的密钥

@Predidit
Copy link
Owner

Predidit commented Feb 9, 2025

感谢你的提醒,我已经修复了这个问题

但对于这个PR本身,除了上面提到的在没有明显好处的情况下增加了复杂度

这个PR合并后会让使用者困惑。例如勾选 bili 弹幕的复选框,会让使用者认为这表明检索结果是否包含 bilibili 弹幕。以及弹幕设置中的弹幕来源的冲突

我现在不能接受这个PR

@Predidit
Copy link
Owner

Predidit commented Feb 9, 2025

非常抱歉这个庞大的PR没能合并,浪费了你的时间

如果有机会的话,计划提交这种庞大的PR前可以在 Issue 中进行更多的讨论

@bggRGjQaUbCoE
Copy link
Contributor Author

非常抱歉这个庞大的PR没能合并,浪费了你的时间

不会,我的改动都是基于自用前提

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