This introduces a separate HistoryDB type at the PagedHistoryProvider
layer, that doesn't need to deal with positions. Positioning logic in
HistoryDataSource becomes a type conversion between the new type and an
existing History type that UI and ItemKeyedDataSource API is written against.
With this refactor, we entirely eliminate nullability from these types.
We were converting Long timestamps into Ints (and getting negative
numbers back), and treating that as, basically, a position for the
paging API; the paging API would pass us back the obscure negative
number back as an offset, and we'll mishandle it resulting in an
infinite loop.
This patch removes all of the Long -> Int conversions, and introduces an
explicit 'position' that is calculated once we have a full page of
results completed.
This is needed to prevent removed search groups from coming back after
user interacts with the tab. E.g. if we don't fire this action, upon
interaction with the tab after a corresponding search group was removed
we will end up creating metadat with the deleted 'searchTerms' (they'll
be read from the 'historyMetadata' state on the tab).
At this point we probably want to start encapsulating all of this in a
use case - let's do that separately in
https://github.com/mozilla-mobile/android-components/issues/11309, and
just fix the bug for now.
This moves the group removal logic to the place where the groups are
actually formed. This helps clean-up the fragment code a bit, and
removes the awkward 'allow mutate some random internal state' API from
the provider.
Our boundary conditions for matching search groups to visits was wrong.
This change switches the boundary buffer to only be applied to history
items, not the metadata items.
In other words, when checking if any of the metadata items match the
current "page" of history, we'll be looking to see if the item falls
within this time window:
buffer - oldest history item <= metadata item <= newest history item +
buffer
There's a separate problem with buffer though: it's reset to 0 when requested
offset is >0, but that requires a larger refactor of this code, for a
separate PR.
When deciding if we should include a history group within the "page of
history" results on the History View UI, we used to look at the most
recent timestamp of the metadata items within the group, and see if that
falls within the range of the timestamps of the history page, +/- some
buffer.
This assumes that each metadata entry will have a corresponding history
item. However, that's not true - when restarting the app, the selected
tab will be restored, and when opening History View right after we'll
record some metadata for it. However, we won't record a history visit
during the app restore for the selected tab.
That's all correct, but the assumption around group matching to history is now incorrect.
This patch changes the logic to instead look at every item within the
group, and see if any of them match the time window of the current
history page. This has a side-effect of also displaying search groups
multiple times on diffenent pages of history, if it makes sense to do so chronologically.
I think that's fine, it reflects reality at least (e.g. items within the
group may have been visited at very different points in time).
Co-authored-by: Christian Sadilek <christian.sadilek@gmail.com>
We currently have a 15s buffer to match metadata to its corresponding
visit. However, a existing metadata record can be updated more than
15s after it was created e.g. when closing the tab and updating
the view time.
Ryan VanderMeulen
Roger Yang
mcarare
mike a
Grisha Kruglov
Arturo Mejia
Christian Sadilek
Gabriel Luong
MarcLeclair
Tiger Oakes
ekager