Open matteocontrini opened 4 years ago
clarkwinkelmann
commented
4 years ago Thanks for the report.
Post numbers alone probably won't solve the issue since we can't rely on numbers to be subsequent or even be in order.
The best solution would probably to read the full list of post IDs for the discussion, which is already loaded by the scrubber, calculate the positions based on that list, then identify the posts by their IDs in the stream and use that to identify where to insert ads.
matteocontrini
commented
4 years ago Hi Clark!
we can't rely on numbers to be subsequent
You're right, especially with hidden/deleted posts, I didn't consider that.
or even be in order
Could this actually happen? I know that the post stream is sorted by post creation date (not sure why honestly), but the post number is still incremented at each post and recomputed when you split/merge. It doesn't seem to behave very differently from the autoincrementing post ID, unless I'm missing something.
Anyway, you're right that the post ID solution is better, at least for the first reason above.
clarkwinkelmann
commented
4 years ago Could this actually happen?
At this moment, with the existing community extensions, and from a forum that started fresh with Flarum, probably very unlikely.
My own Author Change extension can be used to create such anomalies. Other extensions might as well.
This can also happen with faulty imports. I'm actually dealing with this problem right now and creating https://github.com/migratetoflarum/renumber-posts to rectify Flarum databases.
Not sure when we'll be able to implement this into Ads however :grimacing:
matteocontrini
commented
4 years ago Ok! I'll probably send a PR at some point, it doesn't seem hard to implement
Currently, ad slots between posts are calculated by considering only the currently rendered posts (
PostStreamchildren):https://github.com/FriendsOfFlarum/ads/blob/44f401a207ef2ca39684367a4339a7b6ea7b9bb2/js/src/forum/addAdBetweenPosts.js#L13-L26
This has the side effect that when new posts are loaded "above", ad slots are recalculated and might be positioned differently, since the code assumes that the first post of the discussion is the first vnode in the stream.
One solution to this would be to use the actual post number (
post.attrs['data-number']) instead of the index of the vnode (i), and the total posts count (this.count()) instead of the total number of loaded comments (commentPosts.length). The main difference would be that all kinds of posts would be taken into account for the "between" count, instead of only the comment posts. I'm not even sure if this is better or worse in terms of where you would expect ads to be placed... But it is a solution.