gmg137 / netease-cloud-music-gtk

Linux 平台下基于 Rust + GTK 开发的网易云音乐播放器
GNU General Public License v3.0
1.51k stars 89 forks source link

Improve image loading, fix griditem and banner #221

Closed catsout closed 1 year ago

catsout commented 1 year ago

现在 master 的 griditem 和 banner 都是坏的,不然你可以试试清除缓存。
你是不接受这个 pr 吗,理由呢?
还是说不再接受我的 pr 了,是的话请说明,我也就不浪费时间了。

catsout commented 1 year ago

还有,之前给你说的需要检查 upgrade,你没改,现在会页面切快一点就容易崩溃(没有缓存的情况下

gmg137 commented 1 year ago

不接受这个PR是因为我觉得没必要再在这个功能上浪费时间了,无论使用那种方式加载图片在用户感知上都不会有明显区别,除非你的更改能带来显著的性能提升或功能改进。从代码层面来讲在sender上实现方法我是闻所未闻的。

sender.set_image_widget_source(
            &self.avatar,
            NcmImageSource::UserAvatar(li.uid, li.avatar_url.clone()),
        );

如上代码在一个方法中传入枚举作为参数,再在枚举中传入参数,这种使用方法我觉得很奇怪,远不如下面这样来的易于理解。

self.avatar.set_from_net(url, path, (50, 50), self.sender.get().unwrap());

再有争论代码优劣真的没什么意义,我不希望再在这上面浪费时间了。

现在 master 的 griditem 和 banner 都是坏的,不然你可以试试清除缓存。 这个问题在我的机器上没有测出来,我是设置的每日清理缓存,所以每次启动都会自动清理,包括手动删除缓存目录目前都没发现有问题。

还有,之前给你说的需要检查 upgrade,你没改,现在会页面切快一点就容易崩溃(没有缓存的情况下 这个问题在我机器上暂时没有测出来,我再测一下看看。

catsout commented 1 year ago

从代码层面来讲在sender上实现方法我是闻所未闻的

可以写成独立的函数啊,而且 traits 而已,这是非常常见的用法

如上代码在一个方法中传入枚举作为参数,再在枚举中传入参数,这种使用方法我觉得很奇怪

这种方法你一直在用哦

sender.send(Action::DownloadImage(url, path, width, height, callback))   

再有争论代码优劣真的没什么意义,我不希望再在这上面浪费时间了。

我说过了,我不想给不能写抽象的程序加功能,何况之前明明好好的,但是你 force push 了

catsout commented 1 year ago

现在的 banner 的尺寸是坏的,griditem 缺少 fallback icon

还有,之前给你说的需要检查 upgrade,你没改,现在会页面切快一点就容易崩溃(没有缓存的情况下 这个问题在我机器上暂时没有测出来,我再测一下看看。

其实不用看,你要是理解 gobject 和 sender.send 的话,一眼就能看出必须检查 upgrade

catsout commented 1 year ago

还有你的写法在 path 存在时,是需要给每个控件写设置的

而且之前的问题你也没有回答,path 和 size 单独设置有什么用,相同类型的 path 但是却是不同 size,可以认为是 bug.
我一开始就说了,单独写 path 很麻烦,查找和修改都很麻烦,编译器也没法检查, 而且统一了 path 和加载 texture,还可以输出 log.

catsout commented 1 year ago

而且就性能方面,之前的 background thread 明明更好, background thread 之后还能用来干其他活。

catsout commented 1 year ago

再有争论代码优劣真的没什么意义,我不希望再在这上面浪费时间了
我说过了,我不想给不能写抽象的程序加功能,何况之前明明好好的,但是你 force push 了

比如,之前 api 取 json 的代码,如果不改,不加点抽象的话,我是完全不想加什么新功能的。

catsout commented 1 year ago

我正在尝试看能不能用一种更简单的方法来实现图片加载,虽然使用 paintable 更高级一些,但感觉太复杂了,一眼看不明白背后的逻辑。

我不想用 paintable 主要是因为觉得为了加载个图片而另外多出两个文件不值得。而且整个加载逻辑过于复杂,可能我比较水看了好久也没明白 paintable 里边的下载触发逻辑。我做的封装其实只是想模仿你的用法,移动下代码,我也觉得没啥意义

虽然可以在 enum 的实现中使用 to_path 再次计算路径,但每次路径计算都是有性能损耗的,为了代码美观牺牲性能不划算。

目前我只能想到这样的写法,传入 image 的引用好处是没有额外的开销,整个业务处理流程也比较简单,对新人更友好。凡事都有取舍,代码也一样。

别改了,传入path和size更灵活一些。

你之前说的话,有一句靠谱吗,像什么“我一眼看不明白”,“我看不懂”,“format字符串消耗性能”,“不懂gobject,以为只有传引用才没有开销”,“意义不明的灵活”
你 force push 代码,但是改得让人火大,说的理由更让人没法接受,简直像是“我不懂,所以我乱改了”,“能跑就行了”。你认为我之前花时间改这些代码是吃饱了撑着吗

再说浪费时间,就这 pr,包含了 banner 和 griditem 的修复,和一个对你来说不痛不痒,但是对我来说却非常必要的修改,你原本就需要测试你之前 force push 导致的 banner 和 grideitem 的 bug,并不会浪费你什么时间

如果你还是坚持,也不能给出像样的理由,那我也认,我不想再碰这玩意了,我不用了。