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

Control lyrics scrolling with mouse wheel and Adjust radio playback logic #173

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

Chilly-Blaze
Copy link
Contributor

@Chilly-Blaze Chilly-Blaze commented Jan 13, 2024

更改1

resolve #170

@mousewheel 鼠标滚轮控制歌词上下滚动预览,播放到下一句歌词时跳回

2024-01-13.12-29-30.mp4

更改2

resolve #152

调整私人fm新歌曲更新逻辑:

  1. 当播放到最后一首歌时,若未获取到新歌曲,则将播放列表第一首歌曲移动到列表末尾而不是在播放完毕后跳转到第一首歌曲
  2. 当获取到重复歌曲时,照常添加到播放列表的最后,但将播放列表旧的重复歌曲从列表中删除,而不是直接不添加重复歌曲

@Chilly-Blaze Chilly-Blaze marked this pull request as draft January 14, 2024 06:02
@Chilly-Blaze Chilly-Blaze marked this pull request as ready for review January 14, 2024 06:12
@Chilly-Blaze Chilly-Blaze changed the title renderer: Control lyrics scrolling with mouse wheel Control lyrics scrolling with mouse wheel and Adjust radio playback logic Jan 14, 2024
@Chilly-Blaze
Copy link
Contributor Author

Chilly-Blaze commented Jan 14, 2024

这俩其实都是我个人在使用软件的时候碰到的问题,发现之前就有人提出来的issue还没close就顺便做了一下:)

@rocka
Copy link
Member

rocka commented Feb 7, 2024

感谢你的贡献!很抱歉拖这么久才回复,最近没什么时间来维护这个项目 ...

有点小建议,关于 更改1 ,个人感觉用鼠标上下滚动的时候,如果歌曲仍在播放,“播放到下一句歌词时跳回“的这个行为有可能会打断滚动浏览的过程,体验不是特别好。如果改成”鼠标移出歌词区域,且播放到下一句时跳回“可能会更符合直觉。

@Chilly-Blaze
Copy link
Contributor Author

Chilly-Blaze commented Feb 7, 2024

鼠标移出歌词区域,且播放到下一句时跳回

非常好建议,我个人翻歌词主要是为了清唱练习之类的所以一般会先暂停,确实没有考虑到播放时滚动的体验,最新commit中已经改为此逻辑

顺便不知道是计算错误还是特性,歌曲刚开始(index===-1时)的offset似乎有些不对,导致我鼠标滚动边界值计算也出现了问题,所以我按照“初始定位在第一句歌词”的方案进行了调整

@Chilly-Blaze Chilly-Blaze marked this pull request as draft February 7, 2024 15:58
@Chilly-Blaze Chilly-Blaze marked this pull request as ready for review February 7, 2024 17:05
@rocka
Copy link
Member

rocka commented Feb 8, 2024

刚开始 index === -1 时应用 transform: translateY(164px) 的目的是,在播放到第一句歌词之前,让第一句歌词处于“在播放第一句歌词时第二句歌词”的位置,这样可以让歌词滚动的速度看起来更平均一些。当然取的这个数值可能不是很合适,只考虑了两行歌词(一行原文一行翻译)的情况,也没有测试不同的字体,而且有些歌词的前一两句可能还是“作词/作曲”之类的信息,用这种写死的 magic number 也不合适。

试用了一下感觉体验还不错,但似乎还有个小问题,如果鼠标放在歌词区域,即使没有手动滚动过,歌词也不会随时间滚动。我加了句判断只有当 lyricScrollOffset !== 0 的时候才更新 lyricScrollOffset

@rocka
Copy link
Member

rocka commented Feb 8, 2024

私人 FM 歌曲列表更新的逻辑中, state.list.map(s => s.id).indexOf(t.id) 这样在每次查找的时候会都会创建临时的数组,换成 findIndex() 会更好。

另外我觉得应该 rebase 一下,把这些更改分别合并成两个 commit ,这样会让历史更好看;当然也可以拆成两个 PR 来做,毕竟涉及到两个不同的模块。你更倾向于那种方式?(如果觉得麻烦的话可以让我来 rebase )

@Chilly-Blaze
Copy link
Contributor Author

Chilly-Blaze commented Feb 9, 2024

辛苦了!一早起来看到一堆邮件吓我一跳

但似乎还有个小问题,如果鼠标放在歌词区域,即使没有手动滚动过,歌词也不会随时间滚动

这确实是个问题,前天晚上测试各种歌词情况下的那个offset花了将近一个小时,困得不行就没有进行全面的测试,是我疏忽了

state.list.map(s => s.id).indexOf(t.id) 这样在每次查找的时候会都会创建临时的数组,换成 findIndex() 会更好

其实是我忘记了findIndex方法,不过看到您加上了duplicate<=state.index(虽然后来又撤回了),我其实当时写的时候也考虑过duplicate===state.index的情况,假设新获取到的曲子与当前播放的曲子相同的确会出现非预期的问题(可能的现象是播放了很短的时间就跳到下一首曲子?虽然从听曲子的角度也很难感知到),我当时的一个想法是只在duplicate===state.index时删除tracks中的新歌曲,不过因为概率非常低而且感知不明显,我就没有添加额外的判断

另外我觉得应该 rebase 一下,把这些更改分别合并成两个 commit ,这样会让历史更好看;当然也可以拆成两个 PR 来做,毕竟涉及到两个不同的模块。你更倾向于那种方式?(如果觉得麻烦的话可以让我来 rebase )

您太客气了,我只是单纯的希望能够帮上忙,顺便改善一下自己的使用体验,倒也不是怕麻烦,不过毕竟是您的项目,还是由您自己决定和管理rebase还是pr比较合理(不过确实需要整理一下log,需要我的话也可以和我说),我完全没有任何意见(硬要我说的话 rebase 相对更节省时间?),希望没有耽误到您的日常工作,除夕快乐!:)

@rocka rocka merged commit 149190d into Rocket1184:master Feb 9, 2024
1 check passed
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.

UI建议: 在歌词界面增加滚动条 [BUG] 私人 FM 播放完时从头播放
2 participants