ElkArte Community

Elk Development => Bug Reports => Topic started by: scripple on June 04, 2015, 11:43:15 pm

Title: New icons wrong on multipage posts
Post by: scripple on June 04, 2015, 11:43:15 pm
So I've seen this a lot on my forum, and just saw it here so now I know it's not unique to my setup.  If I go to "new" posts in a topic and that topic has enough new posts that the first one it takes me to is not on the last page and I read all the way to the end using the next page button then when I go back to the forum index the board containing the topic is still marked as having unread posts even though there aren't any.

Something about going to the next page is not properly updating the "new" display.  I tried a few experiments with my home setup a while ago using really short pages and had trouble reproducing it.  My home setup doesn't use any kind of caching.  It's possible this is caching problem but the experiments were not thorough enough so I don't make that statement definitively.

The new can be cleared by going back into that topic, and I think by going into the board containing the topic but I'm not 100% certain on that as I don't typically go into boards.
Title: Re: New icons wrong on multipage posts
Post by: emanuele on June 05, 2015, 02:12:48 am
If I read it correctly, this is the ooooooooold SMF behaviour kicking in.
Boards are marked "as read" only and only if you either open the board, or click on the latest message on the board index. Any other way to reach an unread topic will result in not marking the board as read (because you didn't "enter" the board specifically, but you read a topic if it makes sense).

Now, this behaviour has few merits, but is frequently misunderstood as broken, so I guess it should be improved.
Fix it "properly" requires at least another query I think anywhere there is a board, or a db schema change storing somewhere the number of unread topics in a board for each user.
On my forum I have "the query", along with a new "state" of the boards that can be summarized in "you have entered the board, but you still have unread messages".
Title: Re: New icons wrong on multipage posts
Post by: Spuds on June 05, 2015, 09:04:15 am
I think this is also common with sub-boards. 

You may have read everything but until you click on the sub board itself, it does not clear the new posts indicator.  But that's not the new icon so ... never mind me :P
Title: Re: New icons wrong on multipage posts
Post by: scripple on June 05, 2015, 10:29:26 am
I guess I don't see any merit to having a board tell me there are new posts in it when there aren't.  Whether I read them by entering that board or not.  I believe it does happen with sub-boards too. 
Title: Re: New icons wrong on multipage posts
Post by: Flavio93Zena on June 17, 2015, 11:48:19 pm
Can this help you? http://custom.simplemachines.org/mods/index.php?mod=3380
Title: Re: New icons wrong on multipage posts
Post by: scripple on July 03, 2015, 11:03:12 am
Hmm.  Somehow I missed this reply until you pointed it out in the count unread thread.  I'll have to take a look at it.  Thanks.
Title: Re: New icons wrong on multipage posts
Post by: scripple on July 03, 2015, 04:21:06 pm
Wow.  Looking at that mod the "new" posts logic is byzantine.  It certainly isn't a model I would have ever chosen.  Explains some other what to me is odd behavior I've seen.  I simply don't understand the model SMF and by extension elkarte built in terms of "new" posts.  I've never understood the "new posts" vs "All Unread" logic either.  On my forum I just ditched the new posts button and replaced it with the all unread as I don't really understand how it decides what is "new".

For example, looking at the code and then testing to see if it was true, if a board says "new" at the board index, and I click the "go to last post" button from the board index for that board (as opposed to the new icon), it not only takes me the last post in that topic but it also marks the entire board as having no new posts regardless of how many other (formerly) new posts are still in that board.

I guess I prefer a simple "unread = new" model.  That's not what SMF/elkarte uses though.  Honest question, can someone explain the model they do use and how it is helpful?

Regardless of the model though, the fact that I can go from "All unread, to a post, to another (and final) page of the post, to the board index and have the board say there are still new posts when I have in fact read them all (i.e. there are neither new nor unread posts) is still a bug.  Especially since I can clear it (sometimes) by going back to the "All unread" page again and then back to the forum.  This all seems to stem from the complexity of the new != unread model.
Title: Re: New icons wrong on multipage posts
Post by: Flavio93Zena on July 03, 2015, 04:45:32 pm
Got to admit you gave me a headache #_#

This is what the mod does: http://www.simplemachines.org/community/index.php?topic=524378.msg3712984#msg3712984 so technically it fixes the behavior you are complaining about...?
Title: Re: New icons wrong on multipage posts
Post by: scripple on July 03, 2015, 04:59:53 pm
Yes, thank you.  The "new" post code is more distributed and complicated that I would have expected.  I'm hoping Spuds or Ema can explain what the model for "new" is supposed to be.
Title: Re: New icons wrong on multipage posts
Post by: Flavio93Zena on July 03, 2015, 05:14:20 pm
I think it's the default from SMF... It's explained in the topic I linked as well ;)
Title: Re: New icons wrong on multipage posts
Post by: scripple on July 03, 2015, 05:18:13 pm
I'm asking what is the difference between "new" and "unread" in SMF speak.
Title: Re: New icons wrong on multipage posts
Post by: Flavio93Zena on July 03, 2015, 05:24:22 pm
Ooh, hmm, there is none as far as I know...
Title: Re: New icons wrong on multipage posts
Post by: scripple on July 03, 2015, 05:31:19 pm
There definitely is.  Otherwise "New Posts" and "All Unread Posts" wouldn't exist as separate calls.

The page describing the mod say it's to fix going to a board removing it's "new" even if you don't read the new posts.  I'm not 100% confident it will fix the page turning bug I reported as there are so many conditionals in front of where this code is called and I don't know when they're all set or if this code will be called.  It is more concerned with making sure "new" remains set when there are more posts not making sure it's cleared when there aren't any.
Title: Re: New icons wrong on multipage posts
Post by: Flavio93Zena on July 03, 2015, 05:40:48 pm
Oh alright, it's indeed confusing. I think I remember it but just partially so we'd better for wait da big guyz to explain us :)
Title: Re: New icons wrong on multipage posts
Post by: emanuele on July 03, 2015, 06:10:54 pm
Quote from: scripple – I'm asking what is the difference between "new" and "unread" in SMF speak.
I think I explained it somewhere else, though at the moment I'm trying to answer an email without finding the words to, so I have time to chat about it. :P

Actually, I'm not entirely sure what the question is because I'm a little sleepy and my head is numb due to the hot temperature. So I'll just describe some of the uses of "new" and "unread" around here, if I miss the one you are referring to feel free to tell me, I'll add some more tomorrow. ;D

"new" and "unread" can have different meanings depending on the context.
When you look at the message index (i.e. the content of a board) and you see the little "new" next to a message, that means that you have not read all the messages in that topic, so it is basically the same as "unread".

Another meaning has the "new" in the "new posts" menu above (that is partially shared with the "new posts" title of the icon next to the board name in the board index), in that case "new" means "from your last visit" (session-based), and you can see the case there are no "new posts", but clicking the "all unread" you get some messages.

Usually the icon next to the board name on the board index is also the most confusing and inconsistent.
Why? Because it actually has three meanings:
1) the status (the color and the title text) is defined by a flag in a table that checks if you have "entered" the board;
1a) the status is changed only if you directly enter the board, so if you clicked on a link pointing to ?board=x.yy, or for example on the last message of that board with the "boardseen" parameter in the url,
1b) if you enter a topic belonging to that board (but from an url without the "boardseen" parameter, for example from the info center) then the board is not "marked as read" and will show "new posts" even though there is not a single unread message in there;
2) if you click on the image then you are brought to an "action=unread;board=1.0;children" address, this address shares the same meaning of "new" of the "new posts" menu, so posts from last access (session-based), that means it may be there is a "new posts" icon, but clicking on it the list is empty and sends you to the "all unread" that may show something (older than your session);
3) ... I forgot while writing... darn hot!

I'm pretty sure there is more, but at the moment I may have missing something...
Title: Re: New icons wrong on multipage posts
Post by: scripple on July 03, 2015, 06:19:19 pm
Thanks for the reply.  Wow.  That's crazy convoluted.  If you (one of the authors) can't even remember/explain the state machine for "new" I think that's a strong indicator that it's too complex.  It's going to be (and is) very inconsistent to someone using the forum.

So a simple question, how is "last visit" defined?  If I come read a few pages don't log out, go somewhere else, come back and read a few more what defines my last visit?
Title: Re: New icons wrong on multipage posts
Post by: Flavio93Zena on July 03, 2015, 06:20:26 pm
I thought I had an headache, now I am 100% sure about it ;D
Title: Re: New icons wrong on multipage posts
Post by: emanuele on July 04, 2015, 02:21:50 am
Quote from: scripple – So a simple question, how is "last visit" defined?  If I come read a few pages don't log out, go somewhere else, come back and read a few more what defines my last visit?
At the moment I don't have the code in front of me, so I can't give you an exact answer, there is for sure a time threshold based on the last time you opened an url that defines the "last visit" moment, unless you logout (in which case that becomes your last visit). I don't remember the exact value (or odd behaviour lol).
Title: Re: New icons wrong on multipage posts
Post by: scripple on September 23, 2015, 09:57:28 pm
So I finally got annoyed enough with this behavior to poke at it a little.  Basically here's the clearest explanation of the problem I think I can provide.

1) I go to the /forum/index.php?action=unread;all;start=0 page.
2) I see a topic in that page.  I click on the "new" icon.  /forum/index.php?topic=NNNN.msgXXXX;topicseen#new
3) There are enough new replies in the topic that #new does not take me to the last page.  I read the entries on the page and at the bottom I click the next page button.  /forum/index.php?topic=NNNN.YY where YY is the start of the next page.
4) I go through all the pages then go back to the board index.  /forum/index.php

The board with the message I read shows as having unread messages even if that message was the only one in the board.  The reason, the "next" button doesn't set topicseen.  So if you're lucky and "new" takes you to the last page the seen state is updated correctly.  If you're not it's not.

This is very inconsistent behavior that basically makes it broken to use the next page button.  It's better to read a page, go to the unread page (I have that button at the bottom too) and then click the same topic again.  Ugh.  Perhaps we could fix this by having the next button set "topicseen", or at least have it set "topicseen" if topicseen was set on the original entry to the topic.

Something like this

Code: ("Display.controller.php") [Select]
    // Construct the page index, allowing for the .START method...
    $context['page_index'] = constructPageIndex($scripturl . '?topic=' . $topic . '.%1$d' . (isset($_REQUEST['topicseen']) ? ';topicseen' : ''), $_REQUEST['start'], $context['total_visible_posts'], $context['messages_per_page'], true, array('all' => $can_show_all, 'all_selected' => isset($_REQUEST['all'])));

Why is topicseen even an option?  Is keeping the seen state that expensive?
Title: Re: New icons wrong on multipage posts
Post by: scripple on September 23, 2015, 10:39:33 pm
Perhaps an even better solution is to use the check you're already performing to see if the page has new posts. 

Code: ("Display.controller.php") [Select]
			if ($mark_at_msg >= $topicinfo['new_from'])
markTopicsRead(array($user_info['id'], $topic, $mark_at_msg, $topicinfo['unwatched']), $topicinfo['new_from'] !== 0);

Change that to set topicseen if the page contains previously unseen posts.  Then it doesn't matter how you got there.  This check is even right above the part that uses topic seen.  So just something like

Code: ("Display.controller.php") [Select]
			if ($mark_at_msg >= $topicinfo['new_from']) {
markTopicsRead(array($user_info['id'], $topic, $mark_at_msg, $topicinfo['unwatched']), $topicinfo['new_from'] !== 0);
$_REQUEST['topicseen'] = 1;
}

Meh, hard to enter tabs.  The indentation shouldn't be too hard to understand.  :P
Title: Re: New icons wrong on multipage posts
Post by: emanuele on September 24, 2015, 02:02:14 am
Don't tell anyone, but the topicseen parameter is one of those I don't understand fully... probably not even at 50%... :-[
Title: Re: New icons wrong on multipage posts
Post by: scripple on September 24, 2015, 10:09:19 pm
Ok, the only place topicseen and topicseen_cache do anything other than get invalidated (set to array()) is in Display.controller.php.  It's there as a way to try and reduce calls to getUnreadCountSince and markBoardsRead inside Display.controller.php.

Basically if the topicseen flag is not set it will not call markBoardsRead unless the boardseen flag is set.  If the topicseen flag is set and the topicseen_cache for this session and board is > 5 it assumes there are still unread and doesn't call markBoardsRead.  If the cache is < 5 or not set it calls getUnreadCountSince and if there are no new topics calls markBoardsRead by setting the boardseen flag.

Everywhere else in the code it's either topicseen being set on a call to forum/index.php?topic or the topicseen_cache is being cleared because something that might affect the "new" icons has happened.

This seems very convoluted to avoid calling getUnreadCountSince and markBoardsRead and leads to breakage because there are lots of ways to get to a topic without topicseen being set which means markBoardsRead will not be called no matter whether it should be or not.  Thus it provides false indicators to the users.

My second fix uses an already computed value to check whether or not the page being displayed has new posts.  I think this is a much more sane check.  If it has new posts then it impacts the "new" icon.  Call getUnreadCountSince and if that returns empty call markBoardsRead.  If the page does not have new posts it obviously shouldn't change the "new" icon.  So don't bother with getUnreadCountSince and thus don't bother with markBoardsRead.

Then you can eliminate all the places that set topicseen and all the places that clear topicseen_cache and the whole topicseen_cache.  Even if you want to keep the cache there's still really no reason to set topicseen on a guess that you might be going to a page with new posts since you're already checking everytime whether or not the page has new posts.
Title: Re: New icons wrong on multipage posts
Post by: emanuele on September 25, 2015, 02:12:26 am
Nice reading! And thanks for the time spent digging this! :D
If you have any code handy and you want to contribute it, feel free to drop it here or at github. O:-)
Title: Re: New icons wrong on multipage posts
Post by: scripple on September 25, 2015, 02:25:46 am
This is the diff of Display.controller.php that I am testing.  I have removed topicseen and the cache.  I have not bothered to go clean it out from the rest of the code yet.

Code: [Select]
diff Display.controller.php{,.orig}
478c478
<                       if ($mark_at_msg >= $topicinfo['new_from']) {
---
>                       if ($mark_at_msg >= $topicinfo['new_from'])
479a480,489
>
>                       updateReadNotificationsFor($topic, $board);
>
>                       // Have we recently cached the number of new topics in this board, and it's still a lot?
>                       if (isset($_REQUEST['topicseen']) && isset($_SESSION['topicseen_cache'][$board]) && $_SESSION['topicseen_cache'][$board] > 5)
>                               $_SESSION['topicseen_cache'][$board]--;
>                       // Mark board as seen if this is the only new topic.
>                       elseif (isset($_REQUEST['topicseen']))
>                       {
>                               // Use the mark read tables... and the last visit to figure out if this should be read or not.
480a491,492
>
>                               // If there're no real new topics in this board, mark the board as seen.
482a495,496
>                               else
>                                       $_SESSION['topicseen_cache'][$board] = $numNewTopics;
484,485c498,500
<
<                       updateReadNotificationsFor($topic, $board);
---
>                       // Probably one less topic - maybe not, but even if we decrease this too fast it will only make us look more often.
>                       elseif (isset($_SESSION['topicseen_cache'][$board]))
>                               $_SESSION['topicseen_cache'][$board]--;

Or for clarity.
Code: ("Original") [Select]
			if ($mark_at_msg >= $topicinfo['new_from'])
markTopicsRead(array($user_info['id'], $topic, $mark_at_msg, $topicinfo['unwatched']), $topicinfo['new_from'] !== 0);

updateReadNotificationsFor($topic, $board);

// Have we recently cached the number of new topics in this board, and it's still a lot?
if (isset($_REQUEST['topicseen']) && isset($_SESSION['topicseen_cache'][$board]) && $_SESSION['topicseen_cache'][$board] > 5)
$_SESSION['topicseen_cache'][$board]--;
// Mark board as seen if this is the only new topic.
elseif (isset($_REQUEST['topicseen']))
{
// Use the mark read tables... and the last visit to figure out if this should be read or not.
$numNewTopics = getUnreadCountSince($board, empty($_SESSION['id_msg_last_visit']) ? 0 : $_SESSION['id_msg_last_visit']);

// If there're no real new topics in this board, mark the board as seen.
if (empty($numNewTopics))
$_REQUEST['boardseen'] = true;
else
$_SESSION['topicseen_cache'][$board] = $numNewTopics;
}
// Probably one less topic - maybe not, but even if we decrease this too fast it will only make us look more often.
elseif (isset($_SESSION['topicseen_cache'][$board]))
$_SESSION['topicseen_cache'][$board]--;

// Mark board as seen if we came using last post link from BoardIndex. (or other places...)
if (isset($_REQUEST['boardseen']))
{

Code: ("New') [Select]
			if ($mark_at_msg >= $topicinfo['new_from']) {
markTopicsRead(array($user_info['id'], $topic, $mark_at_msg, $topicinfo['unwatched']), $topicinfo['new_from'] !== 0);
$numNewTopics = getUnreadCountSince($board, empty($_SESSION['id_msg_last_visit']) ? 0 : $_SESSION['id_msg_last_visit']);
if (empty($numNewTopics))
$_REQUEST['boardseen'] = true;
}

updateReadNotificationsFor($topic, $board);

// Mark board as seen if we came using last post link from BoardIndex. (or other places...)
if (isset($_REQUEST['boardseen']))
{
Title: Re: New icons wrong on multipage posts
Post by: Flavio93Zena on September 25, 2015, 02:33:33 am
It's even a LOT shorter O.o Nice!
Title: Re: New icons wrong on multipage posts
Post by: Spuds on September 25, 2015, 11:17:08 am
Please keep us informed on how the testing goes.
Title: Re: New icons wrong on multipage posts
Post by: emanuele on September 25, 2015, 02:38:37 pm
Cool!
Thanks!

I guess it can be included in 1.1, I always wanted to drop the topicseen param, that could be a very good opportunity! :D
Title: Re: New icons wrong on multipage posts
Post by: scripple on September 25, 2015, 08:37:49 pm
My forum is small, but so far things seem fine.  Nice to have read boards actually show as read.
Title: Re: New icons wrong on multipage posts
Post by: scripple on September 28, 2015, 09:09:56 pm
Well after a busy couple of days on my forum (relative of course) the mod is working great as far as I'm concerned.  It's so nice to be able to go to topics from any link and have the "new' icons do the right thing.  I've navigated several ways and not noticed anything strange.