Skip to main content ↑ ↓
New icons wrong on multipage posts Started by scripple · · Read 13668 times 0 Members and 1 Guest are viewing this topic. previous topic - next topic

Re: New icons wrong on multipage posts

Reply #15 –

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?

Re: New icons wrong on multipage posts

Reply #16 –

I thought I had an headache, now I am 100% sure about it ;D
~ SimplePortal Support Team ~

Re: New icons wrong on multipage posts

Reply #17 –

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).
Bugs creator.
Features destroyer.
Template killer.

Re: New icons wrong on multipage posts

Reply #18 –

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?
Last Edit: September 23, 2015, 10:08:00 pm by scripple

Re: New icons wrong on multipage posts

Reply #19 –

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

Re: New icons wrong on multipage posts

Reply #20 –

Don't tell anyone, but the topicseen parameter is one of those I don't understand fully... probably not even at 50%... :-[
Bugs creator.
Features destroyer.
Template killer.

Re: New icons wrong on multipage posts

Reply #21 –

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.

Re: New icons wrong on multipage posts

Reply #22 –

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:-)
Bugs creator.
Features destroyer.
Template killer.

Re: New icons wrong on multipage posts

Reply #23 –

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']))
			{

Re: New icons wrong on multipage posts

Reply #24 –

It's even a LOT shorter O.o Nice!
~ SimplePortal Support Team ~

Re: New icons wrong on multipage posts

Reply #25 –

Please keep us informed on how the testing goes.

Re: New icons wrong on multipage posts

Reply #26 –

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
Bugs creator.
Features destroyer.
Template killer.

Re: New icons wrong on multipage posts

Reply #27 –

My forum is small, but so far things seem fine.  Nice to have read boards actually show as read.
 

Re: New icons wrong on multipage posts

Reply #28 –

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.