Fix record navigation when results are loaded with js #4196
Conversation
There was a problem hiding this comment.
Thanks for this, @ThoWagen -- see below for one question, and sorry if I'm misunderstanding something!
| if ($this->resultScrollerActive()) { | ||
| $this->resultScroller()->init($results); | ||
| } | ||
| $this->resultScroller()->init($results); |
There was a problem hiding this comment.
Why has the initialization been made unconditional? Isn't this going to cause initialization in contexts where it is supposed to be disabled? And shouldn't we also check for an active result scroller in the AJAX handler?
There was a problem hiding this comment.
There is no obvious way to access the 'resultScrollerActive' method in 'GetSearchResults' because it is configured for each search and record controller.
I figured that always initializing the result scroller does not cause any problems but avoids introducing new complexities. There might be possible applications in the future where the result scroller is used but Record->next_prev_navigation is false.
But if I'm missing something here I can also try to refactor this so that the decision if the scroller should be initialized is made on the results of the searches instead of a method in the search controller.
There was a problem hiding this comment.
Thanks, @ThoWagen, I think I'm inclined to agree with you, but I'd love to get another opinion (maybe from @EreMaijala, if and when he has time) in case there's something we're overlooking. A couple of related thoughts:
1.) The only potential disadvantage I can see to unconditionally initializing the scroller is that in a system with a mix of different search types, if a user is performing a mix of different search types in multiple tabs/windows, there is a possibility that the result scroller data will get evicted more quickly, since we only store a limited amount in the session. I'm not sure how likely this scenario is (I suspect not very), but it at least seems like a possible problem.
2.) If we don't conditionally initialize the result scroller results, I think we might be able to eliminate the resultScrollerActive method from all of the search controllers, which would nicely reduce redundancy in the code (I've never liked having to override this in two places to make it work right). But maybe that's also a sign that this decision belongs somewhere else, like in the search params object -- if we moved it there, we could potentially make more decisions in the ResultScroller plugin instead of the controllers, and that might be the best of both worlds. But it would also require more refactoring, and some of the edge cases where the behavior is based on factors other than just the search backend might be ugly. I'm not strongly advocating for this much change, but just brainstorming!
There was a problem hiding this comment.
Regarding 1.) We could change that, so that the limit is applied separately for each backend.
Regarding 2.) I'm willing to do refactoring here, since that should certainly be an improvement. But let's see if Ere has an opinion on this.
There was a problem hiding this comment.
Regarding 1.) I think if we have a backend-specific limit, it defeats the purpose of a limit since the goal is to prevent excessive session growth, and a system with a lot of different backends could end up with huge sessions. Maybe this could be offset by having a configurable overall limit and configurable backend-specific limits, but that could get ugly and complicated, and would people actually know to configure it correctly?
Regarding 2.) I agree that we should wait and see what Ere thinks. I know he's having a busy month, though, so we shouldn't expect to hear from him right away. Maybe we should plan to talk about this on the next Community Call if it hasn't been addressed by then, just so we don't lose track of it! I'll give this an 11.0 milestone to ensure it ends up on the agenda.
This PR fixes the error, that next prev navigation is not included after the results are loaded via JS.