#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)
Change History (35)
comment:1 Changed 9 years ago by
Priority: | high → blocker |
---|
comment:3 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
- 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 9 years ago by
Attachment: | 12936.2.patch added |
---|
comment:6 Changed 9 years ago by
New patch, mixin name changed to OnDemandScrollList (can still be discussed...) and Christophe's feedback taken into account - thanks.
comment:7 Changed 9 years ago by
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 9 years ago by
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).
Changed 9 years ago by
Attachment: | 12936.patch added |
---|
Renamed to LazyScrollList?, fixed dojo style, added doc comments - Eric Durocher (IBM, CCLA)
comment:9 Changed 9 years ago by
Milestone: | 1.8 → 1.9 |
---|
comment:10 Changed 9 years ago by
Milestone: | 1.9 → 2.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 9 years ago by
Keywords: | 1.9 added |
---|
comment:12 Changed 9 years ago by
Owner: | changed from ykami to Eric Durocher |
---|---|
Status: | new → assigned |
comment:14 Changed 8 years ago by
This new implementation loads/unloads items by pages, to reduce layout calculations. It is also simpler. Also changed the name to LongListMixin.
Changed 8 years ago by
Attachment: | 12936.3.patch added |
---|
New implementation that loads items by pages, more efficient and fluid - Eric Durocher (IBM, CCLA)
Changed 8 years ago by
Attachment: | 12936.4.patch added |
---|
Final patch including DOH test - Eric Durocher (IBM, CCLA)
comment:16 Changed 8 years ago by
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:18 Changed 8 years ago by
will be closed once put in the release notes and documented in the reference guide.
Changed 8 years ago by
Attachment: | 12936_StoreList.patch added |
---|
Fixed typo in class name + bugs when used with a StoreList? - Eric Durocher (IBM, CCLA)
comment:19 Changed 8 years ago by
See also #16705 which is a continuation to this, using paging at the store level.
Changed 8 years ago by
Attachment: | 12936_clearItems.patch added |
---|
Clear items when loading an empty store - Eric Durocher (IBM, CCLA)
comment:23 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
closing this one the continuation will be #16705.
Changed 8 years ago by
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)
Changed 8 years ago by
Attachment: | 12936_destroy2.patch added |
---|
Fix previous patch which does not destroy all children... - Eric Durocher (IBM, CCLA)
Changed 8 years ago by
Attachment: | test_LongListMixin_deviceTheme.patch added |
---|
Use deviceTheme in test_LongListMixin.html as in other tests - Eric Durocher (IBM, CCLA)
Bulk update of open ticket priorities.