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

Apply callback queue to downloader as well #240

Merged
merged 9 commits into from
Feb 17, 2016
Merged

Apply callback queue to downloader as well #240

merged 9 commits into from
Feb 17, 2016

Conversation

onevcat
Copy link
Owner

@onevcat onevcat commented Feb 16, 2016

@dopcn

在你的基础上改动了一些:

  1. ImageViewUIButton 的 extension 中因为涉及的是 UI 操作,所以必须是 main thread;
  2. ImageCache 里从内存缓存中直接取的时候并不需要进行 dispatch,否则将造成刷新时的闪烁;
  3. 因此调整了一下 ThreadHelper 中的内容,依然还是进行一下判定,如果已经在 main queue 的话就不再做 dispatch 了;

因为从磁盘缓存读取为了性能考虑需要放在另外的线程执行,所以在内存无缓存但磁盘有缓存时刷新的闪烁似乎是不可避免的,不过因为这种情况并不是很常见或者致命。现在并不想添加像是在主线程读取磁盘缓存的选项,所以决定暂时先维持现状这样。稍后会在 optionsInfo 中添加一个强制进行 transition 的标识,如果用户觉得磁盘缓存读取的闪烁不可接受的话可以用像是 fade 之类的过度来缓解。

如果没有什么其它问题的话可以应该 merge 了。

@codecov-io
Copy link

Current coverage is 75.68%

Merging #240 into master will increase coverage by +0.13% as of 9d7f0d2

@@            master    #240   diff @@
======================================
  Files           11      11       
  Stmts         1252    1250     -2
  Branches         0       0       
  Methods          0       0       
======================================
  Hit            946     946       
  Partial          0       0       
+ Missed         306     304     -2

Review entire Coverage Diff as of 9d7f0d2

Powered by Codecov. Updated on successful CI builds.

@onevcat
Copy link
Owner Author

onevcat commented Feb 17, 2016

@dopcn

  1. 这个测试确实可以移除,一会儿我弄。
  2. 将 queue dispatch 的逻辑全部封到 ImageDownloader 是不可取的,因为 ImageCache 可能被单独使用,而且 ImageCache 从 disk 进行读取的话也会走独立的 thread。
  3. 重新审视了一下,在 ImageDownloader 中使用额外的 NSOperationQueue (也就是 sessionDelegationQueue) 来作为 delegate queue 似乎没有必要,主线程就可以很好地担当 delegate callback queue。此外,直接使用 main queue 做 delegate queue,配合 ThreadHelper 还可以在需要主线程回调时减少一次额外的 dispatch。我会把它改回去。

其他应该没什么了。您想要一个项目的 commiter 权限么?这样以后讨论和改动也会方便一些。如果 OK 的话我稍后把你加成项目 maintainer。

@onevcat
Copy link
Owner Author

onevcat commented Feb 17, 2016

Fix:
又想了一下,测试还是保留吧,作为 image extension 回调必定在 main queue 的保证 (与 options 中指定的 queue 无关。)

@onevcat
Copy link
Owner Author

onevcat commented Feb 17, 2016

I will merge it.

onevcat added a commit that referenced this pull request Feb 17, 2016
Apply callback queue to downloader as well
@onevcat onevcat merged commit 94615fc into master Feb 17, 2016
@dopcn
Copy link
Contributor

dopcn commented Feb 17, 2016

ImageDownloader 里的 sessionDelegationQueue 是模仿 AFNetworking 里的处理,其实说起来整个改动的起因都是觉得 AFNetworking 里对 queue 的处理方式非常 elegant,从使用者的角度来说只需要配置好库选项,使用接口时完全不需要再考虑 queue dispatch,所以希望能把这一特性引入 KingfisherManager,目前看来应该是不成功,可能是因为 AF 里核心功能基本都在 manager 里,然而 Kingfisher 的核心逻辑是分在两个不同的类里 cache 和 downloader,所以不能直接照搬。接下来再进行尝试吧

另外对于这个 image extension 回调必定在 main queue 的保证 (与 options 中指定的 queue 无关。)我觉得如果从一个要提供给别人用的库角度讲,把这个逻辑写死在库里不一定是一个好选择。库提供的是不同的配置选项和用户选择了某个选项之后能够成功地实现该选项的效果,用户应该自己负责在使用库时选择正确的选项。在这个例子里就是 Kingfisher 提供默认和自定义两个选项,选择默认回调就会在主线程,选择自定义就会在自定义 queue。如果用户在 UIKit extension 这种应该在主线程回调的方法里却自定义了非 main queue 那么这是一个用户错误,应该改的是他自己的调用代码

最后我不是很清楚成为项目 maintainer 有什么其他权限,不过很荣幸可以成为 maintainer 的一员

@onevcat
Copy link
Owner Author

onevcat commented Feb 17, 2016

Image Extension 因为是 UI 相关,回调是不应该有主线程以外的需求的,理念上来说涉及正误,而并不是灵活性的问题,我认为在库里直接帮助用户处理掉会更友好一些。现在我使用文档进行对此进行了一些说明,不过其实更好的做法是直接杜绝在 UIKit extension 方法中传入 queue 选项的可能性,添加 assert 也许是更好的选择。

maintainer 就是可以直接在这个项目里做 branch 和 commit,以及合并和关别人 issue 等等,大概就和自己的 repo 是一样的。

P.S. 可能的话还是尽量用英语交流,这样别的开发者也能参与进来。谢谢。

@dopcn
Copy link
Contributor

dopcn commented Feb 18, 2016

OK

@onevcat onevcat deleted the dopcn-master branch March 24, 2016 02:42
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