901079 - [Contacts] Contacts list should offer a select mode

Status: RESOLVED FIXED

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

Description

[Francisco Jordano [:arcturus] [:francisco]]

We will need to support a select mode for the export functionality. This mode could be configurable, perhaps we could have bulk remove in the future.

After evaluating some of the ideas, create a new view mode, like in the search view, and after taking a look to the first UX designs (soon to be added), we need to either copy the functionality and look and feel of the contact list or add this 'select mode' to the list.

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

I finally opted for adding the 'select mode' to the contact list, avoiding to be in the middle of the critical path while creating the list.

At some point we need to take into account enter in this search mode while we are still generating the list.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #1)
> At some point we need to take into account enter in this search mode while
> we are still generating the list.

Did you mean "select mode" here instead of "search mode"?  Search mode should work now while we are loading.

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Patch 11323

Hi,

this is just the code needed to setup the list in select mode or go back to normal mode.

Didn't add any extra code to enter in search mode and test it since that will be part of the export functionality.

If you are interested in seeing it in action, you can attach the following code to any action:

|contacts.List.enterSelectMode('Export', function exportfn(ids) {
    alert(ids);
    contacts.List.exitSelectMode();
  },function onExportMode() {
    //Contacts.goBack();
    //List in select mode
  });|

@Ben, I've tried as much as possible not to penalise the performance for the common case of rendering the normal list, but sure you can point me to being even more efficient :)

Thanks folks!

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> (In reply to Francisco Jordano [:arcturus] from comment #1)
> > At some point we need to take into account enter in this search mode while
> > we are still generating the list.
> 
> Did you mean "select mode" here instead of "search mode"?  Search mode
> should work now while we are loading.

Right ;), I mean 'select mode'

Thanks!

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I ran into a couple errors while testing this on my phone:

  1) Enter select mode, press the Home button, enter contacts.  The toolbar menu buttons are back.
  2) Minor visual style issue.  The checkboxes partially show to the left of the fixed header when they scroll off the top of the screen.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 785169 [details]
Patch 11323

I think this is a really good start and the overall app looks nice in the selection mode.

That being said, I did find some issues that probably need some attention:

  - Two issues mentioned in comment 5 above.
  - Multiple checkbox labels are getting added to the DOM for contacts as you scroll through.  See Github comments.
  - I did notice about 1 to 2 seconds of jank when entering select mode with 1000 contacts.  This may be due a perf regression in bug 901657, but also could be due to a lot of reflows from updating the 1000 rows.  I think we need to investigate more.

Based on these issues I think I'd like to review again after the code has been updated.  Since we don't do multiple attachments for pull requests I'll just leave my r? open for now.  (Or should I r-?  Not sure how this is handled with pull requests.)

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I also wanted to add a separate comment outside the context of the review.

It seems that overall we might want to consider some strategies for managing the complexity of the contacts_list.js file.  It was previously 1162 lines and this pull request would take it to 1394 lines.  This seems to be getting fairly large and is only likely to get larger.

If there is any way to extract some of the selection logic out into a separate, testable module, I think it would be helpful.  This could be done with smaller pieces or for the whole module using something like the search.js with its adapter "source" object approach.

That being said, I do like that this approach exposes a very simple API to the export functionality with the enterSearchMode() function that then passes on the IDs via a callback.  Its nice that the export won't have to know anything about the list DOM structure.

On the other hand, one negative of embedding the selection logic directly in contacts_list.js is that the list code now has another layer of statefulness.  This would seem to introduce potentially a new collection of bugs with keeping the state consistent, etc.  (For example, what happens if a user clicks "select all", they switch to mail, add a contact from mail, and switch back to contacts.  Is the new contact selected?)

Anyway, I don't really have a recommended course of action at this point other than to suggest we talk about it more.  I just wanted to put down some general design thoughts that have been running through my head.

Thanks again Francisco for doing this work!

Comment 8

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Patch 11398

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 786866 [details]
Patch 11398

Updated the github PR with comments.  My main current concerns are:

  1) I'd like to minimize the API exposed by the list if we can.  The exact name
     is less of an issue vs. requiring the separate exitSelectMode() as it
     forces other code to keep track of the list's state.

  2) There are some performance issues with entering select mode with a large
     list.  Part of this can be fixed by pausing the visibility monitor and
     possibly optimizing the CSS.  I think this will require more work, though.

  3) The interaction between the list and search in select mode seems like it
     could be cleaner.

  4) I still see the title bar with the selection buttons getting messed up
     when you press Home and re-open contacts while in select mode.

I know there is a desire to start work on the export code itself soon. I think if we can settle on (1) locking down the API for the export code, then items (2), (3), and (4) could be addressed as separate bugs.

I'd be happy to look at (2) as a separate bug.

Thoughts?

Comment 10

[Jose Manuel Cantera]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Comment on attachment 786866 [details]
> Patch 11398
> 
> Updated the github PR with comments.  My main current concerns are:
> 
> I know there is a desire to start work on the export code itself soon. I
> think if we can settle on (1) locking down the API for the export code, then
> items (2), (3), and (4) could be addressed as separate bugs.
> 
> I'd be happy to look at (2) as a separate bug.
> 
> Thoughts?

I agree on addressing 2, 3, and 4 as separate bugs.

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

Hi folks,

(1) has already been addressed following Ben's suggestions.

The rest I'm quite happy to resolve in a different bug :)

Thanks!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 786866 [details]
Patch 11398

Thanks Francisco.  If you could go ahead and write bugs for the performance and title bar issues, I think I'm comfortable giving r+.  The comment you added for the search integration is probably adequate for now until we can look more closely at refactoring.  You addressed all my other concerns and I think we can continue to change the code as you work the export functionality.

Thanks again.  Excellent work!

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Thanks Ben,

Will open the bugs to follow up with the performance improvements we have been talking about.

Comment 14

[Jose Manuel Cantera]

Comment on attachment 786866 [details]
Patch 11398

good work, although a few follow-ups are needed to polish

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

Thanks folks for the reviews and suggestions.

This landed on master:

https://github.com/mozilla-b2g/gaia/commit/44b8bbead5ffeac11af1b44bd89f2a611451c72e

Comment 16

[Fabien Cazenave [:kaze]]

FTR, this patch has introduced user-facing strings without creating the related l10n entries (see selectAll, deselectAll): a follow-up would be neat. Thanks!

Comment 17

[Fabien Cazenave [:kaze]]

Follow-up: bug 911687 (should that be a “depends on…” or a “blocks…”?).