1084298 - [Messages] Decoupling the all inputs query logic from DOM tree structure

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Steve Chung [:steveck]]

Currently we simply use querySelectorAll DOM API to represent the all items of threads/messages, but in bug 903763 we will only render a chunk of data and keep rest of them in memory instead of rendering all at first. This means we should not depend on DOM API for all data anymore.

The structure for keeping the data might be a map with id as key and message/thread object as value. Since there will be another selected map for keeping the selected data, it's also possible to migrate the selected map into one all data map.

Comment 1

[Gabriele Svelto [:gsvelto]]

+1 for this bug and moving to a more MVC implementation. This would also solve the many UI problems that have been plaguing users with very long threads in edit mode where "Select All" would usually:

a) take a lot of time

b) only pick up the messages/threads that had been inserted in the DOM so effectively not *all* if we weren't done yet

Comment 3

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, it's still a WIP that caching the data that need to be displayed in the view. In this way we don't have to render all the data to the view immediately. But hope this idea could cache all the data correctly.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8518829 [details] [review]
Link to github

gave some comments on github :)

I think we should move to have a single model object instead of having the model scattered in the different views.

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8518829 [details] [review]
Link to github

Hi Julien, the patch has been modified that using Threads for all the data need to be cached, but not sure if it's a good way for handling the drafts.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8518829 [details] [review]
Link to github

Moving the feedback request to Oleg, as it's clear I couldn't find time to look at this important refactoring.

Oleg, please see also bug 958105.

I'm sure that you both together will come up with the best way :)

Comment 7

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8518829 [details] [review]
Link to github

Looks good to me, I've left some question\suggestions at GitHub. Removing f? flag for now as can't fully test it on device/mulet due to some exceptions thrown (mentioned that at GitHub too).

Thanks!

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8518829 [details] [review]
Link to github

Hi Oleg, I've applied the suggestion you gave. Will create a follow up for the thread deletion refinement, thanks!

Comment 9

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8518829 [details] [review]
Link to github

Looks good! Just left few ideas at Github. Haven't tried on device yet, but it's for review :)

Thanks!

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8518829 [details] [review]
Link to github

Hi Julien, I remembered you mentioned new data model for thread data weeks ago, and I think this patch might relate to your model, since the the Threads data will changes a little bit and your data model might be built based on Threads. So it will be great it you have any other option about these changes before you move on, thanks!

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8518829 [details] [review]
Link to github

I just made a couple suggestions, the biggest is to use Threads.registerMessage wherever you can, and create a Threads.unregisterMessage to... unregister a message :)

Otherwise this works perfectly :)

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8518829 [details] [review]
Link to github

Hi Julien, the patch is updated based on your comments and I also fixed the draft thread issue, thanks for the suggestions!

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8518829 [details] [review]
Link to github

This looks good.

Just take the message id for unregisterMessage and i think it's good to go !

Comment 14

[Steve Chung [:steveck]]

I think it's good question the how can we only provide id for unregisterMessage(replyed on github). Since it's only used in thread, I wish it could be easiler for message deletion in thread. But I'm not sure if it's good way to imply unregisterMessage will only unregister the message in active thread.

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8518829 [details] [review]
Link to github

Add the map for message id and thread id mapping. Maybe you would prefer the map for id and message object, but I don't find any necessity for this while regist/unregist message(unless you have more thoughts for utilizing this map).

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8518829 [details] [review]
Link to github

Some nits and you forgot to update the unit tests :(

Comment 17

[Steve Chung [:steveck]]

Comment on attachment 8518829 [details] [review]
Link to github

Oops! Sorry for the missing in unit test part :/ Patch updated per suggestions, thanks!

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8518829 [details] [review]
Link to github

r=me with the last fix

thanks !

Comment 19

[Steve Chung [:steveck]]

Thanks for the reminder!(I just checked the console log and it didn't return error in this case, my bad :/)

Comment 20

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#wt-KalZoQSCP20IdUaXsyQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 21

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #19)
> Thanks for the reminder!(I just checked the console log and it didn't return
> error in this case, my bad :/)

Yeah because an error would happen if we call the function without any argument... which we never do ! :)

Comment 22

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/e8ed62fc16a23fb8b68b720562af5be5b45ed7dd