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

修正逻辑错误 #41

Closed
wants to merge 4 commits into from
Closed

修正逻辑错误 #41

wants to merge 4 commits into from

Conversation

dbFlower
Copy link
Contributor

@dbFlower dbFlower commented Sep 2, 2015

此处逻辑应该为 先验证是否stime < time 再验证此条弹幕是否能通过filter的验证, 否则当补全validate方法之后会产生让人抓狂的bug。

此处逻辑应该为 先验证是否stime < time 再验证 此条弹幕是否能通过filter的验证, 否则当补全validate的时候会让人抓狂。
rewrite CCL CommentFilter function
@dbFlower
Copy link
Contributor Author

dbFlower commented Sep 2, 2015

bug 具体内容:
当我实现完整的validate方法之后, 若按照原来的逻辑, 会执行else 的语句, 但是else 中直接break, 所以并未执行for的第三句话。 所以导致this.position一直未自增

repair addrule module,
@jabbany
Copy link
Owner

jabbany commented Sep 3, 2015

有道理。确实是bug。

不过你这改的似乎是build完的版本?CCL的代码是通过src里面的CommentCoreLibrary部分编译得到的。
方便的话在PR里面改成编辑那个文件,然后build一下。(还有Filter好像现在带了一些默认项目?最好默认没有自带的filter)

@dbFlower
Copy link
Contributor Author

dbFlower commented Sep 3, 2015

那个filter 还是残缺的版本, 等我写完了再git上完整的版本。 然后我去src里改代码吧。

修正src目录中CommentManager 中的逻辑错误
@jabbany
Copy link
Owner

jabbany commented Sep 3, 2015

Ok.
也可以开成两个独立的pr分支?让filter基于bug修正,那样merge进来就比较快了
:)

@dbFlower
Copy link
Contributor Author

dbFlower commented Sep 3, 2015

OK。

@dbFlower
Copy link
Contributor Author

dbFlower commented Sep 3, 2015

噗, 不怎么会用git。 导致把我那个CommentFilter 也pr了。 要不你自己改下src中的源码吧。。

@jabbany
Copy link
Owner

jabbany commented Sep 3, 2015

你可以把第一个第三个commit给cherrypick出去。。。
(或者只是第三个,然后用grunt去build一下就自动产生 .js和 .min.js了

@dbFlower
Copy link
Contributor Author

dbFlower commented Sep 3, 2015

get

@dbFlower dbFlower closed this Sep 3, 2015
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.

None yet

2 participants