1232968 - Choosing to import only one option from onboarding screen will import both bookmarks and history

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. With a clean profile open Firefox
2. From the onboarding screen choose "Transfer to Firefox"
3. Uncheck "History" option and leave only "Bookmarks" checked
4. Click "Transfer" -> Next -> Start Browsing

Expected results:
- History panel is empty. Bookmarks panel is  populated with bookmarks from default browser

Actual results:
- Both History and Bookmarks panels are populated from the default browser

note:
- reproducible on 45.0a2 Aurora (2015-12-15), 46.0a1 Nightly (2015-12-15)
- not reproducible on 43.0, 43b10 and 44.0a2 (2015-12-08)

Comment 1

[Chenxia Liu [:liuche]]

Okay, it looks like there's a regression from bug 1209539 - setting another listener seems to have disabled the onMultiClickListener. I'll look into this.

Comment 2

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1232968 - Choosing to import only one option from onboarding screen will import both bookmarks and history. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/28435/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28435/

Comment 3

[Chenxia Liu [:liuche]]

The OnMultiChoiceClickListener we originally set is being replaced by the listener we're later setting on the ListView [1]. I didn't check to see if this roundabout listener-setting was replacing one of our existing listeners (which it is).

Also cleared up some of the checkbox state.

[1] http://androidxref.com/5.1.1_r6/xref/frameworks/base/core/java/com/android/internal/app/AlertController.java#1078

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8699762 [details]
MozReview Request: Bug 1232968 - Choosing to import only one option from onboarding screen will import both bookmarks and history. r=sebastian

https://reviewboard.mozilla.org/r/28435/#review25369

::: mobile/android/base/java/org/mozilla/gecko/firstrun/ImportPanel.java:53
(Diff revision 1)
> -                final List<Integer> checked = new ArrayList<>(Arrays.asList(0, 1));
> +                final Boolean[] checked = new Boolean[2];
> +                Arrays.fill(checked, true);

You don't need Boolean objects here, I think? (Capitalized 'b'). This will avoid Autoboxing/Unboxing[1] overhead.

A compact version would be:
boolean[] checked = {true, true};

[1] https://docs.oracle.com/javase/tutorial/java/data/autoboxing.html

::: mobile/android/base/java/org/mozilla/gecko/firstrun/ImportPanel.java:95
(Diff revision 1)
> +                        final boolean isChecked = ((CheckedTextView) view).isChecked();
> +                        if (isChecked) {
> +                            checked[i] = true;
> +                        } else {
> +                            checked[i] = false;
> +                        }

This looks like it could be simplified:

checked[i] = ((CheckedTextView) view).isChecked();

Comment 5

[Chenxia Liu [:liuche]]

...lol, what embarrassing things. Incremental replacement should always be followed by a sanity check! Thanks :)

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/36aa441232b7

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36aa441232b7

Comment 8

[Teodora Vermesan (:TeoVermesan)]

Choosing to import:
-only "History" and leave "Bookmarks" unchecked => History panel is populated. Bookmarks panel has only the default ones.
-only "Bookmarks" and leave "History" unchecked => Bookmarks panel is populated. History panel is empty.

Verified as fixed using:
Device: Samsung S6 Edge (Android 5.1.1)
Build: Firefox for Android 46.0a1 (2015-12-22)