Bug 1226652 - Login List Multiselection and Deletion

[sleroux]

• Added multi selection functionality to login list
• Search view now retains search after navigating to detail view
• Selected logins from list can be deleted

[rnewman]

As with #1370, this isn't correct — if you're not syncing passwords, you're going to be really alarmed to be told that the logins you delete will be removed from your other devices.

(Also please steal that review request from @st3fan!)

[sleroux]

So on desktop, I can configure what I want to sync so it's not enough to know if they have an account, I also need to know if they've enabled password syncing.

Looking through the code it seems this configuration is part of the 'sync' prefs branch:

https://github.com/mozilla/firefox-ios/blob/master/Providers/Profile.swift#L685

Looks like if metaGlobal scratchpad doesn't include the 'passwords' collection, that means that password sync is not enabled (inferred from: https://github.com/mozilla/firefox-ios/blob/master/Sync/Synchronizers/Synchronizer.swift#L177) and this scratchpad lives inside the SyncStateMachine.

What would be the best way to expose this?

[rnewman]

The simplest — and most accurate — thing to do is to ask SyncablePasswords if there are any passwords that would propagate as deletions; not only does this avoid trying to extract state from the account, which is complicated and error-prone, but it also correctly muffles the warning if they haven't synced passwords yet, have none saved, or already wiped them. Storage is the source of truth!

For history, #1370 calls this hasSyncedHistory, so I'd ape that.

This basically comes down to checking if loginsM is non-empty or loginsL has anything with a sync_status that's not New. We're effectively trying to figure out if SQLiteLogins.removeAll will leave deletion sentinel rows in loginsL.

[sleroux]

@rnewman I've updated the PR to include the removeLoginsWithGUIDs method and hasSyncableLogins check to properly message the user.

@tecgirl I've added in the the custom checkmark and fox icons as well.

[rnewman]

You can drop this down to one line. Wait, that's not true.

[rnewman]

Did I already land queryReturnsResults? I don't see my commits from #1370 in this stack.

[sleroux]

Not yet - I duplicated the code from that PR.

[rnewman]

Could you do the thirty-second test of that PR and land it?

[sleroux]

Yup

[sleroux]

Updated with fixes and rebased from master to include fixes from #1370

@thebnich mind taking a quick look at 35fddce? Had to do some hackery to accommodate the custom selection icons we want to use.

[rnewman]

Nit: if you keep the old indentation throughout, it makes this all clearer.

[rnewman]

LGTM.

[thebnich]

@sleroux I found one minor bug: Click Edit, then Select All, then go back, then click Edit again. The button says Deselect All even though nothing is selected.

Other than that, LGTM besides the layout question above.

[sleroux]

Thanks for finding that bug. I've added a UITest to check for it as well as a fix.