Fixes #140 - Made it so that the search context is kept

[hunterwilliams]

So this one isn't actually as easy as it appears. #140

@joemfb if you can take a look. I'm debating moving the controller onto a service since this requires a new search each time as the results are kept higher up. However, not sure how that would work since it's being "extended". It may work, but worried about possible weird side effects. Any suggestions?

[hunterwilliams]

WIP - Note it currently will not keep the page however the search is kept for a while. Also not sure if possible issue keeping the service running as is without clearing it on logout...

[grtjn]

You also need to store the search result, and this service can hold only one context. Let me try something as well, I was looking at this for github-search as well anyhow..

[grtjn]

I was thinking of something like this: https://github.com/marklogic/slush-marklogic-node/compare/marklogic:master...grtjn:140-keep-search-state?expand=1

[hunterwilliams]

Ah makes sense. I'll pull yours down and finish anything that seems missing (if there even is anything).

[hunterwilliams]

@grtjn copied your changes and got the missing piece.

Looks like it works perfectly unless you hit the weird routing bug (as affecting other search items) #143 which only causes it to have an issue the first time.

[joemfb]

The search response is already persisted to mlSearch.results, so if you use that, you could simplify the implementation (at least be removing the override of ctrl.updateSearchResults().

As for the search behavior change, I'd like to see some more input from others as to whether or not it's desirable (and, if so, should it be optional?).

[grtjn]

Do you think it is possible to create a more generic factory or service that keeps a store of models by name. In the ctrl you would do something like var searchModel = modelService.get('search');. The service returns an empty model initially, but because it is a object reference it will automatically preserve any update the ctrl makes to it. If it could work that way at least..

[hunterwilliams]

sure but is there any thing else we want to keep state of atm or would this just be anticipating some changes?

[grtjn]

More of a useful thing SE's could use to preserve state in other controllers as well. Not really necessary if difficult..

[grtjn]

Regarding behavior: I am all for having options, especially within the same UI (pref all options in one UI). Maybe setup a webex, and discuss with a few?

[grtjn]

I had a chat with Hunter, and he suggested comparing cached state with url before deciding to do a backend call or not. I cleaned up my branch, and committed some changes that seems to handle that appropriately: https://github.com/marklogic/slush-marklogic-node/compare/marklogic:master...grtjn:140-keep-search-state?expand=1

Benefit is that if you redirect to search and add a facet param manually, that gets detected, and a search is triggered. My github-search implements this. You can select owner:marklogic, go to details of the first, then click on license or language there. That will make it jump back to search while selecting that license or language as facet filter. Just clicking Search on top will make it go back to search, where the caching will restore previous state.

If you are on search, and click Search on the top, the request params get removed, and that will kick in the locationChange event, and you get a clean search.

I also played with a reset param that forced the search control to ignore the cached state, but that param stayed on the url browser, and looked a bit ugly. It is possible though, to add that param on the Search button on top, and then have a Back button in the empty space on the left hand. The top Search should then show an clean search, the Back should show the cached results. That Back button did look nice I thought..

[grtjn]

Is it worth adding that extra check in the ctrl init into ml-search? If you don't cache mlSearch, you get what we had before. If you do cache mlSearch, it will recover automatically..

[joemfb]

If we want to cache by URI, might that be better implemented in middleware? I bet there's lots of node.js caching solutions ...

RE: extra URL params, there are hooks in the ml-search controller for this. See parseExtraURLParams() and updateExtraURLParams().

@grtjn, I'm a little confused; what exactly do you want to add to the default ctrl init?

[grtjn]

Caching in middleware won't stop doing unnecessary round-trips from browser to server. Perhaps 'caching' is the wrong word, I really mean storing the search state..

Re add to init:

      if (ctrl.mlSearch.results.results && !urlChanged()) {
        ctrl.response = ctrl.mlSearch.results;
        ctrl.updateURLParams();
      } else {
        ctrl.mlSearch.fromParams().then( ctrl._search.bind(ctrl) );
      }

[hunterwilliams]

Pausing as we need more discussion.

[grtjn]

Tried to summarize a bit in this ticket against ml-search-ng: joemfb/ml-search-ng#79. Part of the changes in this PR are still needed, but would be nice to be able to keep the SearchCtrl in slush clean..

[hunterwilliams]

merged in the latest master just to keep the PR from not getting too far behind

[grtjn]

Maybe you want to sync up with my branch once more? I played with detecting url changes like you suggested. Seems to work nicely in github-search: https://github.com/marklogic/slush-marklogic-node/compare/marklogic:master...grtjn:140-keep-search-state?expand=1

With that I'd be more than happy to merge. I'd say it is the best achievable compromise at the moment..

[hunterwilliams]

So if I'm going to pry this back open - should we actually keep the results cached?

I think it should do a fresh search each time you bring the search tab back otherwise some users will end up looking at stale data that they think is fresh. I realize it's more requests but it could also get annoying to swap back to search and always have to conduct a new search or reload to get the fresh results.

[grtjn]

Keeping the results in memory was the whole point of this PR. Are you suggesting to not merge this??

[hunterwilliams]

I think we should keep the url possibly but not the results

[grtjn]

Then why did you create this PR??

[grtjn]

I still strongly believe we should keep the results as part of the state. The results will not be preserved indefinitely, because as soon as the user clicks any facet, types a new search term, or paginates, the result is updated. But, if the user jumps to a detail, and start moving back and forth to look at multiple results in row, then repeating the search each time in between will be slowing navigation down. You may not notice much locally, but that will be different when running on a remote server..

[joemfb]

I think there's opportunities for optimization here. In particular, I like the idea of some kind of conditional breadcrumbs on the details page, only visible when you've navigated to it from a search result. It could make sense for search results to be restored when navigating back with the breadcrumbs or browser back button.

However, I think it has to be limited to that kind of use case, so that all other actions load fresh data as expected. Additionally, there should probably be some kind of timeout so that results aren't stale if a user leaves the details page open for awhile.

And I should note - all that together is a fair amount of complexity; I'm still not sure that it's warranted for demo applications.

[grtjn]

I was thinking about a timeout for the in memory state as well, but not all apps have data that change in high pace, so it might be less big issue depending on that pace.

Regarding complexity: we are pretty close already with just a limited number of changes in search ctrl. Could be less if part could be added to the superctrl in ml-search-ng.

Perhaps the biggest catch at the moment is that we cannot properly distinguish between the use cases yet. I added a Back button on my github-search, but that doesn't behave different from the Search button at the top (it is currently literally doing the same). That is partly because of how Angular handles routes, partly because of params parsing and such. I played with passing in an extra param, but that caused odd side effects as well, like them ending up in the url, where I didn't want them..

Tech summit is getting too close. Starting to think we need to push this into 0.2.1..