1074732 - [Messages] Create a mixin to handle the "select" UI model

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

In this bug, we want to create a "Select View Dialog" that we can then mixin into ThreadUI and ThreadListUI to handle the select threads/messages dialogs in a more consistent and efficient way.

One goal is to stop using querySelectAll to know which checkbox are checked. We need to uncouple this so that we can stop adding all threads/messages in the DOM.

The "select all" button needs to have a special case here.

Comment 1

[Julien Wajsberg [:julienw]]

In this sprint, we want to define exactly what to do here.

Comment 2

[Julien Wajsberg [:julienw]]

File a separate bug for the "selectall" part. This bug will now focus on providing the main structure for this mixin.

Comment 3

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, it's the first WIP that I mentioned offline, which only handles the select logic and data, but UI control is still kept in thread/thread list. Will add more comments on github, thanks.

Comment 4

[Steve Chung [:steveck]]

Attachment: Link to github(mixing WIP)

Not sure if this one is more close to you mixin idea. Now the Selection only contains the method related to selection and these methods will mixin into ThreadUI /ThreadListUI instead of creating an instance to handle it.

Comment 5

[Steve Chung [:steveck]]

Attachment: Link to github(mixing WIP)

Sorry, the previous patch is wrong...

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8506718 [details] [review]
Link to github(mixing WIP)

yeah, this is more what I have in mind. I think we can even push the mixin concept further, like Oleg did in EventDispatcher.

I left some comments on github.

Thanks!

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8506718 [details] [review]
Link to github(mixing WIP)

I applied the mixin mechanism you mentioned on github, and leaving the UI related method in views. These UI related methods will encapsulate in mixin and handler will only control the selected data list. I also simplify the selected list to array with id only, since id could manage all the related jobs currently.

Comment 8

[Julien Wajsberg [:julienw]]

Left some more comments to github.

I think it's fine to leave everything that is related to view and dom in ThreadUI/ThreadListUI for now, but eventually we'll need to move this too.

Comment 9

[Steve Chung [:steveck]]

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

Revisit the selection module structure, I think it still need some refinement. Basically I add more elements and methods for selection module and expose less selection related logic(and even UI update) on view panel.

Comment 10

[Julien Wajsberg [:julienw]]

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

yeah, this looks good :)

Comment 11

[Steve Chung [:steveck]]

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

Select/unselect method exposed for the case you mentioned, and with some unit tests for selection handler. I also thought that moving module to lazyloader at first, but the problem is module might not be loaded if user tries to edit threads at the very beginning(although most of people don't do this). So maybe we could simply check if the module is loaded since it's trivial that user could not enter edit mode at launch time, or you prefer other way for edit mode entry?

Comment 12

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #11)
> Comment on attachment 8505992 [details] [review]
> Link to github
> 
> Select/unselect method exposed for the case you mentioned, and with some
> unit tests for selection handler. I also thought that moving module to
> lazyloader at first, but the problem is module might not be loaded if user
> tries to edit threads at the very beginning(although most of people don't do
> this). So maybe we could simply check if the module is loaded since it's
> trivial that user could not enter edit mode at launch time, or you prefer
> other way for edit mode entry?

Yeah, we can add a simple check if you want. I think it's impossible to enter the selection mode before we start lazy loading, but maybe if the load takes more time, the user might enable it before the script is inserted.

Another option is to lazy load the script in the "startEdit" function before instanciating the object and entering edit mode? I'm fine with doing this in a separate bug.

Comment 13

[Julien Wajsberg [:julienw]]

I just noticed we don't display the overlay when deleting threads now? Was it modified in bug 1053952? I think we should have an overlay if it takes more than 1sec... and anyway all buttons should be disabled while we delete :/

I'll file a separate bug for this.

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> I just noticed we don't display the overlay when deleting threads now? Was
> it modified in bug 1053952? I think we should have an overlay if it takes
> more than 1sec... and anyway all buttons should be disabled while we delete
> :/
> 
> I'll file a separate bug for this.

It's not bug 1053952...

Comment 15

[Julien Wajsberg [:julienw]]

Filed bug 1088754 for this.

Comment 16

[Julien Wajsberg [:julienw]]

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

I think we need better unit tests. Otherwise, some minor comments + if you can do lazy load now I'd be glad, but we can also move to another bug.

Another thing I see, but it happens on master too: when I press "Select all" and "Deselect all", there is a small moment before the button changes; however, it's really quick when (for example in a thread with 1 message only) you select the message. Do oyu know where this comes from? This can be in another bug too, since it's in master.

Comment 18

[Steve Chung [:steveck]]

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

Here is the refined patch for the test part(I forgot to attach the selection unit test in last patch :-/) and lazyload selection module. I will take some time for the select all latency issue.

Comment 19

[Julien Wajsberg [:julienw]]

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

r=me
let's land it, it's good :)

Comment 20

[Steve Chung [:steveck]]

Thanks for the review!
In master: 2f12df3ab3ff0c34bf1796906ca032796508a085