[MGNLUI-3554] Thumbnail View in magnolia 5.4 doesn't react anymore to changes in the underlining datasource Created: 05/Sep/15  Updated: 15/Apr/16  Resolved: 06/Oct/15

Status: Closed
Project: Magnolia UI
Component/s: workbench
Affects Version/s: 5.4, 5.4.1
Fix Version/s: 5.4.3

Type: Bug Priority: Critical
Reporter: Fabrizio Giustina Assignee: Aleksandr Pchelintcev
Resolution: Fixed Votes: 0
Labels: regression
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File LazyThumbnailLayoutConnector-reinitialize.diff     Text File ThumbnailViewImpl.patch    
Issue Links:
causality
caused by MGNLUI-3373 Thumbnail View needs too much process... Closed
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Basel 14
Story Points: 5

 Description   

Since Magnolia 5.4 (after 5.3 m10) the thumbnail layout is not refreshed properly anymore when the datasource is changed.

The change in MGNLUI-3373 http://git.magnolia-cms.com/gitweb/?p=magnolia_ui.pub.git;a=commit;h=3183aa0b981bb48aadf6fd48686ea676d662a3db introduces a new "EscalatorThumbnailsPanel" implementation which only initializes the client side panel when the widget is built. The amount of thumbnails is never refreshed and cached thumbnails are not changed even if the item changes in the datasource.

This change totally broke our UIs based on the previous ThumbnailContainer, which should have been compatible with the new implementation. The worst thing is that at the moment it seems there is no way to force a refresh, even calling refresh() in LazyThumbnailLayout or clearing the cache in LazyThumbnailLayoutConnector is not useful at all.

For example if you use the thumbnail container to display the content of a specific folder, updating the datasource (selecting a different folder) without destroying/recreating the panel this is what happens:

  1. if you display the content of a folder containing 2 images and then switch to another one which contains 4 images the panel will still display 2 images only (LazyThumbnailLayoutConnector.refreshViewport() called, but cache is not refreshed and the number of html elements not recalculated)
  2. if you display the content of a folder containing 4 images and then switch to another one which contains 2 images only the panel will still show 4 images and only the first two will be refreshed (LazyThumbnailLayoutConnector.refreshViewport() called, but cache is not refreshed and the number of html elements not recalculated)
  3. if you display a folder containing 2 images and switch to another folder containing 2 images nothing happens (refreshViewport() is not even called since the number of items is not changed)

If not updating automatically there should be at least a way to force a real refresh of the content, however I am not sure what kind of event/notification would be better to use here.

As a partial patch we fixed it by reinitializing the panel when the "thumbnailAmount" property changes in LazyThumbnailLayoutConnector, however this is not perfect because it only fixes 1) and 2) but doesn't help when the datasource is refreshed without changing the total number of items.

This is really a blocker for us for properly upgrading to magnolia 5.4, and also patching the widgeset is a real pain...

It would be great to have at least the attached patch included for magnolia 5.4.2, and if somebody could review the LazyThumbnailLayout implementation in order to properly react even if the number of items is not changed.



 Comments   
Comment by Fabrizio Giustina [ 05/Sep/15 ]

another side note connected to the changes in thumbnail layout/container with magnolia 5.4: ThumbnailContainer has been deprecated and javadocs say it should be replaced by "info.magnolia.ui.workbench.thumbnail.data.DelegatingThumbnailContainer", but this class doesn't exist and I could't find any actual replacement... should it not have been deprecated or am I missing something?

/**
 * @deprecated since 5.3.10 in favor of {@link info.magnolia.ui.workbench.thumbnail.data.DelegatingThumbnailContainer}, this container
 *             should be avoided as it loads the items eagerly.
 */
public class ThumbnailContainer
Comment by Aleksandr Pchelintcev [ 01/Oct/15 ]

fgiust Thanks a lot for spotting the issue and providing such an in-depth analysis, we'll try to fix this one asap. However, in order to understand it better - could you pls elaborate a bit on how exactly you change the underlying data-source dynamically?

Btw info.magnolia.ui.workbench.thumbnail.data.DelegatingThumbnailContainer indeed doesn't exist - it used to but during refactroring it was renamed to info.magnolia.ui.workbench.thumbnail.JcrThumbnailContainer which backs the thumbnail view at the moment. JcrThumbnailContainer is almost the same container that is used for the list view now and is supposed to be more performant since it doesn't resolve all the thumbnail urls on initialisation (unlike the old one).

Comment by Fabrizio Giustina [ 12/Oct/15 ]

Hi @apchelintcev,

However, in order to understand it better - could you pls elaborate a bit on how exactly you change the underlying data-source dynamically?

Well, the use case here is having a tree which controls the thumbnail view. When switching from a folder to another the content is changed without recreating the thumbnail view.
For doing that we simply have a subclass of ThumbnailContainer which can be updated in order to set a new folder path. When the new path is set the number of items is recalculated, the getItemIds() methods returns the new identifier, and the fireItemSetChange() method is invoked... more or less something like:

...  extends AbstractContainer
    implements Refreshable, info.magnolia.ui.vaadin.layout.data.ThumbnailContainer 

public void setFolder(String workbenchPath)
    {
        this.workbenchPath = workbenchPath;
        ...
    }

    public Collection< ? > getItemIds()
    {
       // query the current folder, so items could change after the setFolder() call
       ...
    }

    public void refresh()
    {
        refreshTotalSize(); // recalculates the total size, which triggers the refresh with the patch described above
        fireItemSetChange(); // ignored at the moment by LazyThumbnailLayoutConnector
    }

Let me know if you need any additional detail, I would be glad to help or test any proposed fix.

Comment by Aleksandr Pchelintcev [ 13/Oct/15 ]

fgiust thanks for this. However, I managed to reproduce the issue myself with some quick and dirty solution that logically resembles yours. The branch fix/MGNLUI-3554-apch contains the fix which was quite stable for me (currently it is in review). What the fix does - is improves the client-side reaction on the item set change event, so should help in your situation as well.

Comment by Fabrizio Giustina [ 13/Oct/15 ]

great news, I'll try the fix asap, thanks Aleksandr!

Generated at Mon Feb 12 09:07:46 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.