Closed Bug 1074732 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: julienw, Assigned: steveck)

References

Details

(Whiteboard: [p(2.1S7)=2][p(2.1S6)=1])

Attachments

(2 files, 1 obsolete file)

46 bytes, text/x-github-pull-request
julienw
: review+
julienw
: feedback+
Details | Review
46 bytes, text/x-github-pull-request
julienw
: feedback+
Details | Review
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.
In this sprint, we want to define exactly what to do here.
Whiteboard: [p(2.1S6)=1]
File a separate bug for the "selectall" part. This bug will now focus on providing the main structure for this mixin.
Whiteboard: [p(2.1S6)=1] → [p(2.1S7)=2][p(2.1S6)=1]
Assignee: nobody → schung
Target Milestone: --- → 2.1 S7 (24Oct)
Attached file 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.
Attachment #8505992 - Flags: feedback?(felash)
Attached file Link to github(mixing WIP) (obsolete) —
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.
Attachment #8506705 - Flags: feedback?(felash)
Sorry, the previous patch is wrong...
Attachment #8506705 - Attachment is obsolete: true
Attachment #8506705 - Flags: feedback?(felash)
Attachment #8506718 - Flags: feedback?(felash)
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!
Attachment #8506718 - Flags: feedback?(felash)
Attachment #8505992 - Flags: feedback?(felash) → feedback-
Attachment #8506718 - Flags: feedback+
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.
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 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.
Attachment #8505992 - Flags: feedback- → feedback?(felash)
Comment on attachment 8505992 [details] [review]
Link to github

yeah, this looks good :)
Attachment #8505992 - Flags: feedback?(felash) → feedback+
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?
Attachment #8505992 - Flags: review?(felash)
(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.
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.
(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...
Filed bug 1088754 for this.
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.
Attachment #8505992 - Flags: review?(felash)
Duplicate of this bug: 854476
Blocks: 880647
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.
Attachment #8505992 - Flags: review?(felash)
Comment on attachment 8505992 [details] [review]
Link to github

r=me
let's land it, it's good :)
Attachment #8505992 - Flags: review?(felash) → review+
Blocks: 1084298
Thanks for the review!
In master: 2f12df3ab3ff0c34bf1796906ca032796508a085
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 887198
You need to log in before you can comment on or make changes to this bug.