limit number of items on list tile & collection tile

[fredvd]

The list tile is now hard coded to 5 items by default in tiles/list.py:ListTile with limit = 5 . The set_limit() method on the same class is marked with a TODO, the count field is not used/ignored:

# TODO: get rid of this by replacing it with the 'count' field
def set_limit(self):
    for field in self.get_configured_fields():
        if field and field.get('id') == 'uuids':
            self.limit = int(field.get('size', self.limit))

I checked with some debugging but afaics the field.get('size', self.limit) will never find a size value since it's not there. Is this some legacy code that would break installed base if we implement the TODO? Maybe in the CarouselTile that is inheriting from ListTile?

The Collection tile has a similar hard coded value where the number_to_show field value is chcked but otherwise set to 4. At least here you can override the default value, but the schema has a note that number_to_show should be renamed to count (to be on par with naming in the ListTile).Another installed base migration.

Last, the Layout configuration of the List tile shows a 'Position' label next to the "Number to Show" field name, but this seems to be the ordering of the fields, not the field for count. See attached screenshot.

Questions:

• What are the risks of fixing these fields for installed base?
• Should we create fields for these default limitations for list and collection in the cover settings on the plone control panel instead of hard coding them in the python file so that we can override them per site and set using plone.app.registry?
• Should the default limit field for list tiles be overridable in the List Tile Layout config just as is now the case for the collection Tile?
• Should it be one value for both field limits, or separate limits for List/Carousel / Collection?

[fredvd]

The "Position" label on the Number of items to display field in the Tile configuration view is coming from a modification done last year (8ff071bd) to implement an offset value for the collection tile. And as the collection tile is inheriting from the list tile this also modifies the list.

[hvelarde]

I think your question is really complex and involves too many things; we need to break this in various issues in order to solve them easily; anyway, I will try to answer you.

as you already know, collective.cover code is really complex and nobody understands all of it well enough; some decisions made on the beginning are showing not being the best ones. that's normal and we have to fix them, trying to disrupt the less possible and move on.

one of these decisions is the use of an adapter to store on persistent annotations the configuration of tiles. I don't know how to solve this but we need to simplify the implementation. as an example, we are now having an issue with the checkout of a very complex front page in one of our customers taking too long (like 30 seconds).

the problem with some of the fields and their default values is that the default value is not always initialized. IIRC, the default value is only initialized if you edit the tiles first, instead of configure it. I think this is an issue with the configuration of tiles. I had to include some funky code to initialize a tile in some cases because of this.

on a perfect world we should have extended z3c.form to have a configuration mode, but I think at that point nobody knew what we were doing.

so, on the list tile: we should get rid of the hard-coded limit; as long as I can see, the limit attribute is only used internally and in one place on the list tile template. we can provide a function called limit returning the value of the count field with a deprecation warning message to limit the damage. we should also write a long changelog entry explaining this; that's all we can do.

same thing with the collection tile, we are setting it to 4 in case it has not taken the default value for some reason. I would love to change the fields name, but I think that could be just a little bit harder here.

so, resuming:

• a count field should be used to limit the number of items in a list or carousel, or the results shown from a collection; we should provide this feature by tile instance and we should provide deprecation warning when possible.
• so, we should not create a site wide configuration for the count field on these tiles; that should remove functionality on the package and we don't want that. anyway, there was an idea (from @quimera) of using a configuration registry at some point. we should think about it: a place to store all the configuration of tiles and other information like items stored on tiles and their order.
• we need to find out how to solve the lack of default values on configuration widgets and configuration widgets themselves as they are basically broken.
• yes, there are some risk involved but they are not evident for me; we should fix these issues now and wait for the results; people should test their own stuff before upgrading the package on a production site.

[hvelarde]

BTW, the position label you're seeing on the configuration screen of the list tile should be a bug. probably the Int widget inherits from the TextLine one and we are setting that string on it.

[djowett]

As you know, I am just creating a new carousel tile: covertile.cycle2. As with the old carousel tile, it will be based on the list tile, but since this new tile does not have any existing user base, what can I do now to avoid issues mentioned here in the future?

[hvelarde]

we must fix the issue :-)

[djowett]

Note: the actual commit for "funky code to initialize the tile"

[hvelarde]

yes, that's the kind of crap I want to avoid :-)

[djowett]

A couple of comments:

• collection tile does not inherit from listtile
• 'Position' label is coming from the textline.pt template in configuration_widgets
• this is because configuration_widgets are overridden based on widget type and there is a switch in the ITextline template based on field type - so ANY zope.schema.Int field in the configuration will have a "Position" label. That's a wee bit horrible IMO
• @fredvd is correct to say that count is not used in list tile. In covertile.cycle2 I have just had to change the limit to 100 to remove it (for now)

Things that might help (and I'm just throwing out ideas from the top of my head here):

• Use z3c.form subforms or ObjectWidgets for the Configuration widgets (but beware of object-caveat for the latter)
• Use form.hidden more than form.omit,

e.g.

form.mode(autoplay='hidden')
form.mode(ITileEditForm, autoplay='input')

instead of:

form.omitted('autoplay')
form.no_omit(ITileEditForm, 'autoplay')

UPDATE: Better link for Multiwidget / ObjectWidgets looks the right thing to use, this gives an example of a "Minmax widget" http://docs.plone.org/develop/plone/forms/z3c.form.html#combined-widgets

[hvelarde]

I think we will have some resources to work on this at some point in the following weeks; also, I think this is not a blocker for 1.0b1.

@rodfersou please check this when you have some spare time and we can discuss later how to aboard the issue.

[fredvd]

@djowett Currently Line 47 in tiles/carousel.py has "class CarouselTile(ListTile):" . What do you mean with 'class collection tile does not inherit from listtile', for your carousel implementation?

[djowett]

@fredvd you seem to be mixing up references to Carousel Tile & Collection Tile ...

In tiles/collection.py:

class CollectionTile(PersistentCoverTile):

In tiles/carousel.py:

class CarouselTile(ListTile):

Not a major point, but I'm just saying (in response to your second comment on 29th Aug) that the "Position" label change creeps in to Carousel tile because it shares the same Widget (template) as the Collection tile, not because they both inherit from ListTile.

[fredvd]

@djowett Mixup: you are absolutely right, I was blogging from PLOG with the carousel in mind and didn't read. :-)

The position/number weird naming in the layout model dialog for configuring tiles is indeed coming from the shared widget. template. If I'm correct the 'position' label was introduced by my former colleague Kuno he added the support in the collection tile to start displaying items from a certain position (but didn't realise the widget for a 'number' field is shared).

The use case for this position is that you can create a coer for exmple for news items where you create one collection tile that highlights the 2 items in a different layout and then you could use another instance of the collection tile that lists new-items 3 and following in a smaller list style.

[hvelarde]

and that is related with #298

[jeanferri]

IMHO we should not have limit to list tile and carousel tile. If the user want to have 2 or 52 items in the list is a personal decision and he just need to drag the items he wants to the list or carousel. Although the collection need a limit field because de number of result items depends on how many items have in the portal.

[hvelarde]

@cleberjsantos why did you close the issue?

[cleberjsantos]

@hvelarde I'm Sorry, I didn't do it for wanting!

[cleberjsantos]

Initial fix for limit number of items on list tile & collection tile here https://github.com/collective/collective.cover/commit/1f9f8f706ea67491287c38011bbaebd78bbaa144

[cleberjsantos]

Solves this issue with #802