regression: album view shows folders with lead image two times

[fredvd]

@rodfersou @mauritsvanrees With pull request #524 the album view now considers ILeadImage to show images inside a folder.

This works very well, until you add the ILeadImage to folders or other folderish content types as well. We have such a use case where we use a LeadImage on folders for page header image purpose. Here our folder now shows up twice: Once as an image and second as a folder containing images.

This is also what @mauritsvanrees warned about in a comment in the the issue: https://github.com/plone/plone.app.contenttypes/pull/499 . There are some screenshots there where you tested the ILeadimage, but that seems to be on on a document with and without an image, not with a folder.

The addition for ILeadImage to collections in pull request #524 has the right algorithm: it loops over the collection results, first checks for IFolder and adds folders to the album_folders list and if that check fails IImage and ILeadimage are considered. but the folder methods for album_images and album_folders work independently, which they shouldn't.

One odd thing: I just tested in coredev plone 5.1 (5.1.6>) A folder is only doubled as an image if a lead image has been uploaded.

In a customer project where the webmaster reported that the folders were doubled after we updated to Plone 5.1.6, the folders dont have an actual image in the leadimage field but still show up twice. :-S

[fredvd]

One odd thing: I just tested in coredev plone 5.1 (5.1.6>) A folder is only doubled as an image if a lead image has been uploaded.

In a customer project where the webmaster reported that the folders were doubled after we updated to Plone 5.1.6, the folders dont have an actual image in the leadimage field but still show up twice. :-S

Found out why coredev is different from my customer project: there's now also condition added in listling_album so that only folders with a valid scale (and thus filled in image) are shown. Didn't have that yet:

https://github.com/plone/plone.app.contenttypes/blob/e93187fc7b2fe56168461e7e85e812d1988b1edc/plone/app/contenttypes/browser/templates/listing_album.pt#L27-L29

Regression still remains that folders with Ileadimage and an actual filled in image (which isn't too logical but still possible) are displayed both as an image and a folder in the listing_album

[mauritsvanrees]

Oh my, I continued on Fred's branch to fix a problem with the tests, see PR #538, and ran down a rabbit hole, as they say.

Biggest thing is that I discovered a new problem, totally unrelated to the original PR #524, because the problem was already there before. The problem is: the album view only shows Folders from plone.app.contenttypes as albums. If you have a custom folder based on the class plone.dexterity.content.Container, it will not show up.

I made some sample content. Using the Dexterity control panel:

• Give the standard Folder the leadimage behavior.
• Add a CustomFolder type with class Container and the leadimage behavior.

Then I created a main folder, which contains every combination I can think of:

• an image
• a news item
• a standard folder without leadimage and without an image
• a standard folder without leadimage and with an image
• a standard folder with leadimage and without an image
• a standard folder with leadimage and with an image
• a custom folder without leadimage and without an image
• a custom folder without leadimage and with an image
• a custom folder with leadimage and without an image
• a custom folder with leadimage and with an image

This is the folder contents:

The folders with a red arrow are the ones that should not be visible as albums, because they have no leadimage and contain no images. All the other ones should be visible, either as album or as image.

In the following screen shots I have slightly changed the album view template to show headers for the list of Images and Albums, for clarity.

So what was the situation before the original PR that added leadimage support? With plone.app.contenttypes 1.4.15:

This probably fits the 80 percent use case, but otherwise it is pretty bad. Only the standard Image and Folder types work.

So Rodrigo comes along and supports lead images. With plone.app.contenttypes 1.4.16:

Much better! But there is a duplicate, which is why Fred created this issue. And custom folders are never used as albums, which is what I discovered. Some custom folders are in the images.

So Fred started fixing the duplicate case. After commit fc977acfcb3afea015e6ef9c700435aecaae2981:

The duplicate is gone, which is good. But the Folder with leadimage and no images is missing. And like before, custom folders are never used as albums.

So I tried to improve the custom folder handling. Problem is that the code only considers plone.app.contenttypes.interfaces.IFolder as possible albums. I tried letting this search for Products.CMFCore.interfaces._content.IFolderish, but this interface is blacklisted for the object_provides index by the catalog, so this resulted in finding no albums at all. So I changed the code to look for plone.dexterity.interfaces.IDexterityContainer:

This finds more albums, which is good. But the folder with leadimage and no images is still missing. Same for a custom folder with leadimage and no images. This is because they are now in the albums list, so Fred's deduplication makes sure they do not get added to the list of images. But the template fails to find an image inside these folders, so it does not show them. In other words:

• Such a folder has a leadimage, so it can be shown in the images list.
• But it is also a folder, so it should be shown in the albums list, so we remove it from the images list.
• But it contains no images, so we remove it from the albums list, so we should not have removed it from the images list...

This is tricky to get right, when you try to avoid loading many objects, and doing all kinds of calculations multiple times. And it is especially tricky to not fix things by breaking another unthought-of corner case.

A way out may be to use the catalog to get all (lead) images and (custom) folders in one go, and do all needed calculations in the view, without letting the template call the album view on all sub folders.

I have already spent far too much time on this. Is anyone up for improving this?

(BTW, photo credit: me.)

[rodfersou]

How about to add a new indexer with this calculation?