Closed Bug 1084298 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S11 (1may)

People

(Reporter: steveck, Assigned: steveck)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
julienw
: review+
azasypkin
: feedback+
Details | Review
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.
+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
Duplicate of this bug: 880647
Depends on: 1074732
Attached file 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.
Attachment #8518829 - Flags: feedback?(felash)
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.
Attachment #8518829 - Flags: feedback?(felash)
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.
Attachment #8518829 - Flags: feedback?(felash)
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 :)
Attachment #8518829 - Flags: feedback?(felash) → feedback?(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!
Attachment #8518829 - Flags: feedback?(azasypkin)
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!
Attachment #8518829 - Flags: feedback?(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!
Attachment #8518829 - Flags: feedback?(azasypkin) → feedback+
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!
Attachment #8518829 - Flags: review?(felash)
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 :)
Attachment #8518829 - Flags: review?(felash)
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!
Attachment #8518829 - Flags: review?(felash)
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 !
Attachment #8518829 - Flags: review?(felash)
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 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).
Attachment #8518829 - Flags: review?(felash)
Comment on attachment 8518829 [details] [review]
Link to github

Some nits and you forgot to update the unit tests :(
Attachment #8518829 - Flags: review?(felash)
Comment on attachment 8518829 [details] [review]
Link to github

Oops! Sorry for the missing in unit test part :/ Patch updated per suggestions, thanks!
Attachment #8518829 - Flags: review?(felash)
Comment on attachment 8518829 [details] [review]
Link to github

r=me with the last fix

thanks !
Attachment #8518829 - Flags: review?(felash) → review+
Thanks for the reminder!(I just checked the console log and it didn't return error in this case, my bad :/)
Keywords: checkin-needed
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.
Whiteboard: [p=1]
(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 ! :)
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=1] → [p=1][sms-sprint-2.2S11]
Whiteboard: [p=1][sms-sprint-2.2S11] → [p=1]
Target Milestone: --- → 2.2 S11 (1may)
You need to log in before you can comment on or make changes to this bug.