Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#12936 closed enhancement (fixed)

Large and deep list support for mobile lists

Reported by: Chris Mitchell Owned by: Eric Durocher
Priority: blocker Milestone: 1.9
Component: DojoX Mobile Version: 1.6.1
Keywords: 1.9 Cc: cjolif
Blocked By: Blocking:

Description

need to improve ability to handle very large lists of items, similar to grid (maybe gridx can be used as the basis for this work.

Needs to use a paging/caching store, but be simple to mixin to components similar to DataList? in 1.7

Without this support, too many dom elements get created, and webkit's checkerboard kicks in.

Attachments (9)

12936.2.patch (16.3 KB) - added by Eric Durocher 7 years ago.
12936.patch (16.6 KB) - added by Eric Durocher 7 years ago.
Renamed to LazyScrollList?, fixed dojo style, added doc comments - Eric Durocher (IBM, CCLA)
12936.3.patch (12.2 KB) - added by Eric Durocher 6 years ago.
New implementation that loads items by pages, more efficient and fluid - Eric Durocher (IBM, CCLA)
12936.4.patch (20.3 KB) - added by Eric Durocher 6 years ago.
Final patch including DOH test - Eric Durocher (IBM, CCLA)
12936_StoreList.patch (5.8 KB) - added by Eric Durocher 6 years ago.
Fixed typo in class name + bugs when used with a StoreList? - Eric Durocher (IBM, CCLA)
12936_clearItems.patch (1.6 KB) - added by Eric Durocher 6 years ago.
Clear items when loading an empty store - Eric Durocher (IBM, CCLA)
12936_startup_destroy.patch (858 bytes) - added by Eric Durocher 6 years ago.
Fix 2 bugs in LongListMixin?: startup fails if called twice and generateList must destroy items - Eric Durocher (IBM, CCLA)
12936_destroy2.patch (1.0 KB) - added by Eric Durocher 6 years ago.
Fix previous patch which does not destroy all children... - Eric Durocher (IBM, CCLA)
test_LongListMixin_deviceTheme.patch (705 bytes) - added by Eric Durocher 6 years ago.
Use deviceTheme in test_LongListMixin.html as in other tests - Eric Durocher (IBM, CCLA)

Download all attachments as: .zip

Change History (35)

comment:1 Changed 7 years ago by Colin Snover

Priority: highblocker

Bulk update of open ticket priorities.

comment:2 Changed 7 years ago by ykami

Is this a duplicate of #13789?

comment:3 Changed 7 years ago by Eric Durocher

Cc: cjolif added

The attached patch is a first attempt at implementing quick, load-on-demand scrolling for long dojox.mobile lists.

The main code in dojox/mobile/_QuickScrollMixin is meant to extend a dojox.mobile list class so that items are loaded on demand when the list is contained in a ScrollableView, to ensure that only a small subset of the items are actually contained in the DOM. The feature is optional, to use it the dojox/mobile/QuickScrollList module must be required by the application, this will extend the RoundRectList class with the load-on-demand code. A 'quickScroll' attribute is then available on the list to enable quick scrolling.

Overall the solution makes long lists really responsive, while a list of more than a few hundred items is unusable without it on most devices.

One point of discussion is that the mixin overrides addChild and removeChild to update the items loaded in the DOM. This means that the original (implied) semantics can be changed: addChild may result in an item not being actually added to the DOM. Note that getChildren and _getSiblingOfChild are also overridden for consistency (but not the item's getParent()...). An alternate solution would be to define a specific API like addItem/removeItem, but the current solution is much less intrusive, all you have to do is require the additional module and set the 'quickScroll' flag an any list.

Comments very welcome! (code, extend scheme, naming, etc...)

comment:4 Changed 7 years ago by cjolif

Eric, maybe more than "QuickXXX" shouldn't it be "OnDemandXXX" following dgrid pattern? See: https://github.com/SitePen/dgrid/blob/master/OnDemandGrid.js or https://github.com/SitePen/dgrid/blob/master/OnDemandList.js

Also I know that is not the case of dojox.mobile, so maybe that should be kept like that for now but I think longer term we should not prefix with underscore classes that are actually used by the user. See: https://docs.google.com/document/d/1qjU9hm1HUnZIv051yC01VS81EmciA27GuqOkQPXmY3E/edit

Also why requiring a quickScroll attribute? Either the user mixin the class in his List and he has the functionality or he is not and he doesn't?

Also I'm not sure the QuickScrollList module is really needed? One can easily do (just as this is done in TreeMap or Calendar or dgrid):

var myList = new (declare([RoundRectList, OnDemandListMixin]))();

or

<div data-dojo-type="RoundRectList" data-dojo-mixin="OnDemandListMixin"></div>

comment:5 Changed 7 years ago by Eric Durocher

  • Quick -> OnDemand: yes, maybe, although "on-demand" feels more like we are loading from a data store, which is the case for dgrid but not here. Also it seems we tend to remove the Mixin postfix. If we go that way, some candidates: OnDemandScrollList, OnDemandScrollableList, OnDemandList?
  • underscore: my idea was to keep the underscored class private, and expose only the actual subclasses or mixins. If we keep only one class and let the user extend himself, of course the underscore goes off.
  • QuickScrollList: I agree, not very useful, if we go for the "mixin per instance" pattern.
  • quickScroll attribute: We would need it if we chose to extend at the class level, for example RoundRectList?.extend(...), so we can choose to activate or not per instance.

Changed 7 years ago by Eric Durocher

Attachment: 12936.2.patch added

comment:6 Changed 7 years ago by Eric Durocher

New patch, mixin name changed to OnDemandScrollList (can still be discussed...) and Christophe's feedback taken into account - thanks.

comment:7 Changed 7 years ago by ykami

Eric, thank you for the patch. Looks interesting approach. Reducing the number of DOM elements is effective in improving the scrolling performance. Some questions and comments:

  • Does this work with RoundRectDataList/RoundRectStoreList?
  • I don't think real apps load thousands of items all at once. Can this be used with a paging store just like what test_EdgeToEdge*-auto-*.html are doing?
  • I can't come up with a good naming, but by 'OnDemandScrollList', I guess most people imagine "a list widget that loads data and creates items on-demand while user is scrolling". But that's not true, right?
  • Does this work with variable-height items? In particular, you may need to be careful about icons on variable-height items. Their layout often breaks when we do something tricky.
  • Does this work with the edit mode?
  • I like the idea of making this an optional feature without adding additional weight to the base widget.

comment:8 Changed 7 years ago by Eric Durocher

Kamiyama-san, thanks for the feedback.

  • Yes it should work with all lists, I tested by modifying test_EdgeToEdgeDataList-auto-sv.html, seems OK.
  • On naming: I agree, as I said previously OnDemand seems strongly correlated with data store loading which is not the case here. Maybe LazyScrollList ?
  • Yes it works with variableHeight items, I took care not to rely on fixed item height in the implementation. The only limitation is that the size of the scrollbar might not be 100% exact in the variable height case, since I have to approximate the size of the items not yet loaded, but this should not be really noticeable.
  • No it does not work with edit mode yet (the feature is disabled if editable = true), because reordering is done by calling DOM functions directly, not removeChild/addChild. I did not find a reasonable solution for this so this is a limitation that should be documented (including the fact that the DOM must not be modified directly).
Last edited 7 years ago by Eric Durocher (previous) (diff)

Changed 7 years ago by Eric Durocher

Attachment: 12936.patch added

Renamed to LazyScrollList?, fixed dojo style, added doc comments - Eric Durocher (IBM, CCLA)

comment:9 Changed 7 years ago by ykami

Milestone: 1.81.9

comment:10 Changed 7 years ago by Colin Snover

Milestone: 1.92.0

Current plan is that we are going from 1.8 to 2.0, so bulk reassigning and removing the 1.9 milestone so nobody tries to use it.

comment:11 Changed 7 years ago by Colin Snover

Keywords: 1.9 added

comment:12 Changed 7 years ago by Eric Durocher

Owner: changed from ykami to Eric Durocher
Status: newassigned

comment:13 Changed 6 years ago by cjolif

Milestone: 2.01.9

Let's try to have that in 1.9?

comment:14 Changed 6 years ago by Eric Durocher

This new implementation loads/unloads items by pages, to reduce layout calculations. It is also simpler. Also changed the name to LongListMixin.

Changed 6 years ago by Eric Durocher

Attachment: 12936.3.patch added

New implementation that loads items by pages, more efficient and fluid - Eric Durocher (IBM, CCLA)

comment:15 Changed 6 years ago by Eric Durocher

fixed a bug + slight simplications/optimizations.

Changed 6 years ago by Eric Durocher

Attachment: 12936.4.patch added

Final patch including DOH test - Eric Durocher (IBM, CCLA)

comment:16 Changed 6 years ago by cjolif

I will commit that one, but I think we should create another ticket for a future release that deals with store lists which goes one step further by creating a similar mixin for store list that leverages paging to create DOM elements only when needed. Can you create it and reference it here edurocher?

comment:17 Changed 6 years ago by cjolif

In [30583]:

refs #12936. Long list support, this will only put in DOM the displayed elements. Thanks Eric Durocher (IBM, CCLA).

comment:18 Changed 6 years ago by cjolif

will be closed once put in the release notes and documented in the reference guide.

Changed 6 years ago by Eric Durocher

Attachment: 12936_StoreList.patch added

Fixed typo in class name + bugs when used with a StoreList? - Eric Durocher (IBM, CCLA)

comment:19 Changed 6 years ago by Eric Durocher

See also #16705 which is a continuation to this, using paging at the store level.

comment:20 Changed 6 years ago by cjolif

In [30589]:

refs #12936. Fixed a typo in class name + bugs when used with a StoreList. Thanks Eric Durocher (IBM, CCLA)

comment:21 Changed 6 years ago by cjolif

In [30600]:

refs #12936. Fix when used with _StoreListMixin & append = true. Thanks Eric Durocher (IBM, CCLA).

Changed 6 years ago by Eric Durocher

Attachment: 12936_clearItems.patch added

Clear items when loading an empty store - Eric Durocher (IBM, CCLA)

comment:22 Changed 6 years ago by cjolif

In [30618]:

refs #12936. Clear items when loading an empty store. Thanks Eric Durocher (IBM, CCLA)

comment:23 Changed 6 years ago by cjolif

Resolution: fixed
Status: assignedclosed

closing this one the continuation will be #16705.

Changed 6 years ago by Eric Durocher

Attachment: 12936_startup_destroy.patch added

Fix 2 bugs in LongListMixin?: startup fails if called twice and generateList must destroy items - Eric Durocher (IBM, CCLA)

comment:24 Changed 6 years ago by Patrick Ruzand

In [30837]:

refs #12936, startup fails if called twice and generateList must destroy items. Thanks Eric Durocher (IBM, CCLA)

Changed 6 years ago by Eric Durocher

Attachment: 12936_destroy2.patch added

Fix previous patch which does not destroy all children... - Eric Durocher (IBM, CCLA)

comment:25 Changed 6 years ago by Patrick Ruzand

In [30845]:

refs #12936, Fix previous patch which does not destroy all children. (thx edurocher)

Changed 6 years ago by Eric Durocher

Use deviceTheme in test_LongListMixin.html as in other tests - Eric Durocher (IBM, CCLA)

comment:26 Changed 6 years ago by cjolif

In [31002]:

refs #12936. Use propoer theming. Thanks Eric Durocher (IBM, CCLA).

Note: See TracTickets for help on using tickets.