(analytics) add table records, loaded after initial results, to the results object

[Guillaume-Developer]

This commit fixes an issue where, in the ag-grid, records loaded after the initial results wouldn't be selectable or cannot be added to the collection or any other functionality requiring selection.

[ericleib]

There is an issue with this fix: the datasource might request you for records at an arbitrary page (like, go from page 1 directly to 100), so you cannot just append these new records at the end of searchService.results.records, that would be inconsistent. What is the issue with selection exactly ? Are you using storage = 'id' or storage = 'record' for your selectionOptions? (cf https://github.com/sinequa/sba-angular/blob/c537f46c5c0107efbd30d0a709f6465d6cf866da/projects/components/selection/selection.service.ts#L30)

[Guillaume-Developer]

I see, it makes sense. The issue is that records are not selected when using the select all button. I am using the storage = 'record'.

[ericleib]

The "select all" button is problematic because there is ambiguity about what "all" means: is it the current page of results ? or is it the entire result set (basically any document returned by the WHERE clause). It's hard to do something in between... In the case of AG Grid, this is all the more an issue because the concept of "results page" is hidden. One possibility is to just remove the "Select All" button, and let users select documents manually. If there is a need for selecting all the results set, then maybe go for a custom implementation (instead of tracking documents 1 by 1, track the WHERE clause?).

[Guillaume-Developer]

Removing the "Select All" button is not a possibility for us unfortunately. This is one of the specification that was accepted and we have to have a select all option. Now unfortunately, tracking the WHERE clause would work if my goal was only the download of all documents and we did not care of the visual impact on the interface, but we need to have the documents selected in the grid, which is only possible through the ag-grid, because they are only accessible there.

Now, how about keeping the part of the code I added, but adding a check on the rank of the records before adding them to the results.records object? The rank should give us the position of the records compared to other records, so I think we could use that to order them.

EDIT

I have tried this in my environment and it does work as expected. If you are okay with it, I will push the change so you can have a look. If you want to not include such a change at all, working or not, let me know, I'll scrap my pull request and only keep the code on my personal project.

[ericleib]

That solution makes sense. My concerns are with the potential sides effects:

• By mutating searchService.results, there could be some impacts on other views/components that take the results as an input (I think the risk is reasonably low).
• If you allow many documents to be selected and using "storage = 'record'", the list of selected records could grow pretty big in memory (1000 selected docs x 1MB = 1GB...), so watch the performance
• When the user scrolls to arbitrary locations, there will be "holes" in the list of records (like if you jump from page 1 to page 10, then pages 2-9 are missing): I wonder if this could cause some modes of selection to fail, like when you hold SHIFT to select a range of docs (ie. does AG Grid allow you to select records from page 1 to page 10 with this method?)
• From a UX point of view, the "Select All" button is going to select the pages that are loaded in the Grid (so in between 1 page and all the pages) => will it be clear for users? Some might be confused by what "all" means.

Anyway, let's keep the PR open, and feel free to update it with your solution. I am sure this problem will come up again, so this discussion will be very helpful. If all the risks are cleared, I'd be happy to merge it.

[Guillaume-Developer]

Regarding the points you brought up:

• For now I did not notice any difference in the selection between my modified version and the original version. The SHIFT selection mode works the same way in both cases, and the select all only works in the modified version. Although, the SHIFT selection mode is behaving strangely when skipping some pages, in both versions: selecting records before the skipped pages, then trying to select after the skipped pages only works after "reselecting" the first record to start from the SHIFT selection in the pages after the skip. If you want to discuss that I can open a bug report.
• It might need clarification at first, it is a good point. I'll make sure to bring this to my team and we'll include this detail in the user manual or bring it up directly with the customer

Alright, I'll keep the change on my side while we try to resolve the points we talked above. If you find anything else that might be shaky with this modification, let me know!

EDIT

I ran some tests and came up with the following conclusions about point 1 and 2.

• The memory use is a bit higher (+20~50MB) than before the modifications but the performances are not really impacted by it since the original use was quite low, even while loading 2000+ results. The client's environment is using the ag-grid to display data coming from a DB, which does not contain a lot of data per record, so I am not expecting 1MB per record here.
• I noticed a burst in used memory (150MB to 500+MB) when switching from the ag-grid view to a normal list view (two different tabs) when there were 2000+ results loaded. I believe it comes from the fact that the results are not directly reset to the new tab results, so the 2000+ records are trying to display with the extracts etc... leading to a surge in memory use that does not get flushed until a new page is loaded in the new tab. An easy solution is to reset the records on the search event "before-select-tab".

[ericleib]

About the burst in memory: AG-Grid only creates DOM elements for the rows/columns that are visible on the screen (even if it has 2000 records loaded in memory), so at most a few dozens of rows and columns. But when you switch to a normal result list, Angular just generates as many DOM elements as there are records, so it's very costly. Resetting the records when you leave the AG-Grid view is a good idea!

[Guillaume-Developer]

I see, I understand now why I am not seeing any burst while using the AG-Grid. I was also wondering, rather than forcing this update on everyone using the AG-Grid we could make it optional via an @Input, but this also requires to add a parameter to the constructor of the datasource.ts.

ag-grid-view.component.ts

/** Activates the possibility to select all the records in the AG-Grid using the SelectionService

• Please monitor performances when setting to true. This option saves the newly loaded records
• to the results object, so it can become costly in memory usage */ @Input() activateSelectAll: boolean = false; ... makeDatasource() : IDatasource { if(this.results) { return new SqDatasource(this.results, this.query, this.colDefs, this.searchService, this.appService, this.facetService, this.selectionService, this.activateSelectAll); } return {getRows: () => []}; }

datasource.ts

constructor( public results: Results, public query: Query | undefined, public colDefs: ColDef[], public searchService: SearchService, public appService: AppService, public facetService: FacetService, public selectionService: SelectionService, private selectAllActive: boolean = false )

[Guillaume-Developer]

Did you have a chance to take a look at the changes I pushed?

[ericleib]

Hi Guillaume, Sorry for the delay. Your implementation looks fine, but I had some concerns about the user experience, so I am curious to hear your feedback. Did you deploy these changes to users? Are they happy with it and not confused by the behavior of the select all button? One small implementation detail: rather than completely clearing the results in OnDestroy, I would rather truncate the list to the first pageSize, as it's possible to switch results views without triggering a new search.

[Guillaume-Developer]

Hi Eric, The change has not been tested by users yet, this is a functionality developed for a project not yet in production. Though, the functionality has been made optional with my last push, and it is off by default. Oh, you're right indeed, I'll push this fix next week as I am off until Tuesday.

[Guillaume-Developer]

Hi Eric, did you have a chance to get a look at the changes yet?

[ericleib]

Hi Guillaume, Yes, I did and I think your code is fine from a technical point of view. I am just worried about the side-effects we discussed above. If you tell me that you tried it in prod and users are happy, then let's merge this.

[Guillaume-Developer]

When I first pushed the changes for this pull request, everything was done on my master branch, which left me unable to update it since it was always on standby here. I have now created a dedicated branch for the ag-grid changes, and will create a new pull-request once I have sufficient user feedback.