Allow CollectionViewModel/Controller queries without SectionIdentifier

Allow CollectionViewModel/Controller queries without SectionIdentifier

This CL replace the method to get the indexPath of an item knowing its section
identifier in the CollectionViewModel by a method returning the indexPath of an
item even if its section identifier is unknown.
It adds a method to know if an item is present in a CollectionViewModel.
It also replace the reconfigure method of CollectionViewController to allow reconfiguring
without knowing the section identifier of the items.

BUG=703652
Review-Url: https://codereview.chromium.org/2761263003
Cr-Commit-Position: refs/heads/master@{#468964}
Committed: https://chromium.googlesource.com/chromium/src/+/61dc89e9c94a7d2874e05c1ac9b0a02cf6d3bb7b

[gambard, 2017-03-21 14:30:09]

gambard@chromium.org changed reviewers:
+ lpromero@chromium.org, rohitrao@chromium.org

[gambard, 2017-03-21 14:30:10]

This CL will not be committed as is. This is intended to be a discussion to know
if this method make sense and if we should keep the other method (with the
section identifier).

I think it is OK to keep the old method, as it is faster (this one is doing a
look up through all the items).

WDYT?

[rohitrao (ping after 24h), 2017-03-21 14:36:21]

What's the use case for this?  Why don't you know the section identifier?

[gambard, 2017-03-21 14:48:32]

On 2017/03/21 14:36:21, rohitrao wrote:
> What's the use case for this?  Why don't you know the section identifier?

The use case is: an item ask for an update of its image, with a callback. The
callback update the image then reconfigure the cell. To reconfigure the cell I
need to know its indexPath.
As the item is launching the fetch, it does not know its index path or section.
I need a way to get its index path knowing only the item.

If you see another way to do it, lmk.

[rohitrao (ping after 24h), 2017-03-21 15:13:30]

If you have a pointer to the cell, you might be able to use
https://developer.apple.com/reference/uikit/uicollectionview/1618094-indexpat....

One note, though: we designed this model to work with "dumb" items, which should
be as close to PODs as we can get.  Triggering image fetches might be a job for
the VC, not for the Item itself.

Louis, does this match your understanding?  Do we have settings Items that
trigger data fetches?  How about in bookmarks or history?

[gambard, 2017-03-21 15:29:38]

Yes, but as the items don't know the cell displayed, I need to find the index
path based on the item, not the cell.
The item will notify the VC which will fetch the image then update the item. The
item doesn't have any other logic here is an example
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/content_suggestion...
(note that in this case I was able to get the section but it won't be the case
every time).
As the callback can take time, the item might be moved to another section. I
don't see how the VC can know in which section the item is.

[lpromero, 2017-03-22 14:04:22]

Regarding the new method:
- conceptually, it takes the concept of
-indexPathForItem:inSectionWithIdentifier: a step further. So if we found the
first one useful, this one should also be.
- I'd even remove -indexPathForItem:inSectionWithIdentifier:, but it provides a
nice optimization so far. (Actually, if perf becomes an issue, should we
consider a reverse table internally?)

Regarding the dumb item:
- In the case of content suggestion, I think the call to [self.delegate
loadImage:self] could be replaced with a call from within the controller's
cellForItemAtIndexPath, no? The item doesn't seem to be needed in the loop here.

https://codereview.chromium.org/2761263003/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/collection_view_model.mm (right):

https://codereview.chromium.org/2761263003/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/collection_view_model.mm:285: DCHECK(NO);
Use NOTREACHED.

[gambard, 2017-03-24 12:04:20]

I will let rohit answer on the new method.

Concerning the usage in ContentSuggestions, I think it is better that the item
is initializing the fetch for two reasons:
- We need access to the item to update its image and to get the ID, so having
directly the item instead of getting the item based on the index path of the
configuring cell. And I don't see how you would handle the delay of the callback
(i.e. you would still need to update the item and reconfigure it).
- Not all items want to update their image. It means I would need to check the
type of the item in cellForItemAtIndexPath:.

[gambard, 2017-03-28 08:15:09]

I have added a method to reconfigure a cell based on the item, using this new
function.

WDYT?

[lpromero, 2017-03-28 09:09:08]

Please update the description and add tests.

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/collection_view_model.mm (right):

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.mm:271: for
(SectionItems* sectionItems in _sections) {
Iterate on _sectionIdentifiers and use haseItem:inSectionWithIdentifier:.

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.mm:279: for
(NSUInteger section = 0; section < [_sections count]; section++) {
Idem, use the same methods that use the sectionIdentifier.

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.mm:285: DCHECK(NO);
Use NOTREACHED().

[gambard, 2017-03-28 13:34:45]

Description was changed from

==========
Add a query method for item in CollectionViewModel

This CL adds two methods to CollectionViewModel, allowing to query the index
path of items without knowing their SectionIdentifier.

BUG=703652
==========

to

==========
Allow CollectionViewModel/Controller queries without SectionIdentifier

This CL adds two methods to CollectionViewModel, allowing to query the index
path of items without knowing their SectionIdentifier.
It also adds a method to CollectionViewController to allow reconfiguring without
knowing the section identifier of the items.

BUG=703652
==========

[gambard, 2017-03-28 13:35:23]

Thanks, I have updated the CL.
It was intended as a discussion, this is why it was slightly messy :)

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/collection_view_model.mm (right):

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.mm:271: for
(SectionItems* sectionItems in _sections) {
On 2017/03/28 09:09:08, lpromero wrote:
> Iterate on _sectionIdentifiers and use haseItem:inSectionWithIdentifier:.

I wanted to offer a solution without the current implementation in case we
wanted to remove the current implementation.
Done.

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.mm:279: for
(NSUInteger section = 0; section < [_sections count]; section++) {
On 2017/03/28 09:09:08, lpromero wrote:
> Idem, use the same methods that use the sectionIdentifier.

This method was slightly more efficient (lookup only once in the section
containing the item).
Done anyway.

https://codereview.chromium.org/2761263003/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.mm:285: DCHECK(NO);
On 2017/03/28 09:09:08, lpromero wrote:
> Use NOTREACHED().

Done.

[gambard, 2017-04-04 16:15:28]

rohitrao: ping

[lpromero, 2017-04-07 12:10:22]

Gauthier and I talked IRL and came up with this idea:

1. The collection view controller keeps a dictionary of item → indexPath when
-collectionView:willDisplayCell:forItemAtIndexPath: and
-collection​View:​did​End​Displaying​Cell:​for​Item​At​Index​Path:​ are called.
Items are added/removed as they are displayed.
2. We can even remove the current lookup method from the model altogether.

Hypothesis is that the item → indexPath mapping is only for visible items.
Perf is good because we never have more items than there are cells displayed
(and bookeeping is just inserting/removing from the dict).

Rohit, what do you think?

[rohitrao (ping after 24h), 2017-04-07 12:15:06]

On 2017/04/07 12:10:22, lpromero wrote:
> Gauthier and I talked IRL and came up with this idea:
> 
> 1. The collection view controller keeps a dictionary of item → indexPath when
> -collectionView:willDisplayCell:forItemAtIndexPath: and
> -collection​View:​did​End​Displaying​Cell:​for​Item​At​Index​Path:​ are
called.
> Items are added/removed as they are displayed.
> 2. We can even remove the current lookup method from the model altogether.
> 
> Hypothesis is that the item → indexPath mapping is only for visible items.
> Perf is good because we never have more items than there are cells displayed
> (and bookeeping is just inserting/removing from the dict).
> 
> Rohit, what do you think?

If the controller keeps this mapping, does that allow us to remove the
indexPathForItem method from the model?  Will that work for favicons?  (And
why?)

[lpromero, 2017-04-07 15:26:22]

On 2017/04/07 12:15:06, rohitrao wrote:
> On 2017/04/07 12:10:22, lpromero wrote:
> > Gauthier and I talked IRL and came up with this idea:
> > 
> > 1. The collection view controller keeps a dictionary of item → indexPath
when
> > -collectionView:willDisplayCell:forItemAtIndexPath: and
> > -collection​View:​did​End​Displaying​Cell:​for​Item​At​Index​Path:​ are
> called.
> > Items are added/removed as they are displayed.
> > 2. We can even remove the current lookup method from the model altogether.
> > 
> > Hypothesis is that the item → indexPath mapping is only for visible items.
> > Perf is good because we never have more items than there are cells displayed
> > (and bookeeping is just inserting/removing from the dict).
> > 
> > Rohit, what do you think?
> 
> If the controller keeps this mapping, does that allow us to remove the
> indexPathForItem method from the model?  Will that work for favicons?  (And
> why?)

Yes, that was point number 2.

Gauthier can confirm that the code works as I say, but:
- the item is updated on the favicon callback whatsoever.
- we need the index path only to reload/reconfigure the cell for the given item,
but it's a no-op when the index path is not visible anyway. Items not visible,
when scrolled into view, will just work as if their favicon was always there.

[gambard, 2017-04-10 12:17:11]

On 2017/04/07 15:26:22, lpromero wrote:
> On 2017/04/07 12:15:06, rohitrao wrote:
> > On 2017/04/07 12:10:22, lpromero wrote:
> > > Gauthier and I talked IRL and came up with this idea:
> > > 
> > > 1. The collection view controller keeps a dictionary of item → indexPath
> when
> > > -collectionView:willDisplayCell:forItemAtIndexPath: and
> > > -collection​View:​did​End​Displaying​Cell:​for​Item​At​Index​Path:​ are
> > called.
> > > Items are added/removed as they are displayed.
> > > 2. We can even remove the current lookup method from the model altogether.
> > > 
> > > Hypothesis is that the item → indexPath mapping is only for visible items.
> > > Perf is good because we never have more items than there are cells
displayed
> > > (and bookeeping is just inserting/removing from the dict).
> > > 
> > > Rohit, what do you think?
> > 
> > If the controller keeps this mapping, does that allow us to remove the
> > indexPathForItem method from the model?  Will that work for favicons?  (And
> > why?)
> 
> Yes, that was point number 2.
> 
> Gauthier can confirm that the code works as I say, but:
> - the item is updated on the favicon callback whatsoever.
> - we need the index path only to reload/reconfigure the cell for the given
item,
> but it's a no-op when the index path is not visible anyway. Items not visible,
> when scrolled into view, will just work as if their favicon was always there.

For favicons, yes this would work. 
But looking more closely at my code, there is a use case for the
indexPathForItem:, when the mediator/coordinator ask for the deletion of a
specific item (for example, the user sign out and we want to remove the
associated suggestions). The collection needs to know the index path of the item
to remove it.

[lpromero, 2017-04-10 12:25:10]

On 2017/04/10 12:17:11, gambard wrote:
> On 2017/04/07 15:26:22, lpromero wrote:
> > On 2017/04/07 12:15:06, rohitrao wrote:
> > > On 2017/04/07 12:10:22, lpromero wrote:
> > > > Gauthier and I talked IRL and came up with this idea:
> > > > 
> > > > 1. The collection view controller keeps a dictionary of item → indexPath
> > when
> > > > -collectionView:willDisplayCell:forItemAtIndexPath: and
> > > > -collection​View:​did​End​Displaying​Cell:​for​Item​At​Index​Path:​ are
> > > called.
> > > > Items are added/removed as they are displayed.
> > > > 2. We can even remove the current lookup method from the model
altogether.
> > > > 
> > > > Hypothesis is that the item → indexPath mapping is only for visible
items.
> > > > Perf is good because we never have more items than there are cells
> displayed
> > > > (and bookeeping is just inserting/removing from the dict).
> > > > 
> > > > Rohit, what do you think?
> > > 
> > > If the controller keeps this mapping, does that allow us to remove the
> > > indexPathForItem method from the model?  Will that work for favicons? 
(And
> > > why?)
> > 
> > Yes, that was point number 2.
> > 
> > Gauthier can confirm that the code works as I say, but:
> > - the item is updated on the favicon callback whatsoever.
> > - we need the index path only to reload/reconfigure the cell for the given
> item,
> > but it's a no-op when the index path is not visible anyway. Items not
visible,
> > when scrolled into view, will just work as if their favicon was always
there.
> 
> For favicons, yes this would work. 
> But looking more closely at my code, there is a use case for the
> indexPathForItem:, when the mediator/coordinator ask for the deletion of a
> specific item (for example, the user sign out and we want to remove the
> associated suggestions). The collection needs to know the index path of the
item
> to remove it.

Right, even if the item is not currently displayed :(
This scenario is a valid use case.
So this leaves the  solutions:
1. Reload the entire sections when the coordination layer asks for mutations.
2. Keep slow lookup in the model (possibly with both methods or only the new).
3. Support fast lookup in the model (only the new method).

[lpromero, 2017-04-19 16:24:19]

Kindly ping, Rohit

[gambard, 2017-05-02 12:46:16]

PTAL: Following our discussion at the convergence, I removed the old methods
taking the items and the section identifier. I replaced it with methods taking
only items.

[lpromero, 2017-05-02 13:18:08]

LGMT. Please update the description.

https://codereview.chromium.org/2761263003/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/collection_view_model.h (right):

https://codereview.chromium.org/2761263003/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.h:173:
inSectionWithIdentifier:(NSInteger)sectionIdentifier;
Shouldn't we remove that one too?

[lpromero, 2017-05-02 13:18:25]

lgtm

[gambard, 2017-05-02 13:24:21]

Thanks!

https://codereview.chromium.org/2761263003/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/collection_view_model.h (right):

https://codereview.chromium.org/2761263003/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.h:173:
inSectionWithIdentifier:(NSInteger)sectionIdentifier;
On 2017/05/02 13:18:08, lpromero wrote:
> Shouldn't we remove that one too?

Well, I hesitated and it would make sense to have a lookup to know in which
section an item is no?

[gambard, 2017-05-02 13:27:37]

Description was changed from

==========
Allow CollectionViewModel/Controller queries without SectionIdentifier

This CL adds two methods to CollectionViewModel, allowing to query the index
path of items without knowing their SectionIdentifier.
It also adds a method to CollectionViewController to allow reconfiguring without
knowing the section identifier of the items.

BUG=703652
==========

to

==========
Allow CollectionViewModel/Controller queries without SectionIdentifier

This CL replace the method to get the indexPath of an item knowing its section
identifier in the CollectionViewModel by a method returning the indexPath of an
item even if its section identifier is unknown.
It adds a method to know if an item is present in a CollectionViewModel.
It also replace the reconfigure method of CollectionViewController to allow
reconfiguring
without knowing the section identifier of the items.

BUG=703652
==========

[lpromero, 2017-05-02 13:43:21]

Thanks~

https://codereview.chromium.org/2761263003/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/collection_view_model.h (right):

https://codereview.chromium.org/2761263003/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model.h:173:
inSectionWithIdentifier:(NSInteger)sectionIdentifier;
On 2017/05/02 13:24:21, gambard wrote:
> On 2017/05/02 13:18:08, lpromero wrote:
> > Shouldn't we remove that one too?
> 
> Well, I hesitated and it would make sense to have a lookup to know in which
> section an item is no?

Makes sense, especially in your case.

[gambard, 2017-05-03 14:12:47]

rohit@: ping

[rohitrao (ping after 24h), 2017-05-03 14:22:54]

lgtm

[gambard, 2017-05-03 14:23:42]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-05-03 14:23:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 14:36:51]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493821422836370,
"parent_rev": "dd799306dbacf94b73b935d9870d0119f06c2304", "commit_rev":
"61dc89e9c94a7d2874e05c1ac9b0a02cf6d3bb7b"}

[commit-bot: I haz the power, 2017-05-03 14:37:07]

Description was changed from

==========
Allow CollectionViewModel/Controller queries without SectionIdentifier

This CL replace the method to get the indexPath of an item knowing its section
identifier in the CollectionViewModel by a method returning the indexPath of an
item even if its section identifier is unknown.
It adds a method to know if an item is present in a CollectionViewModel.
It also replace the reconfigure method of CollectionViewController to allow
reconfiguring
without knowing the section identifier of the items.

BUG=703652
==========

to

==========
Allow CollectionViewModel/Controller queries without SectionIdentifier

This CL replace the method to get the indexPath of an item knowing its section
identifier in the CollectionViewModel by a method returning the indexPath of an
item even if its section identifier is unknown.
It adds a method to know if an item is present in a CollectionViewModel.
It also replace the reconfigure method of CollectionViewController to allow
reconfiguring
without knowing the section identifier of the items.

BUG=703652

Review-Url: https://codereview.chromium.org/2761263003
Cr-Commit-Position: refs/heads/master@{#468964}
Committed:
https://chromium.googlesource.com/chromium/src/+/61dc89e9c94a7d2874e05c1ac9b0...
==========

[commit-bot: I haz the power, 2017-05-03 14:37:10]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/61dc89e9c94a7d2874e05c1ac9b0...