1153904 - Add item in Settings for Voice input

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

We should add a line in Settings to turn this off if the user doesn't want it.

I'm thinking inside Settings > Display 

Since theres a clue to the "title bar" under there.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Title: Search dictation 
Subtitle 1: Disabled
Subtitle 2: Enabled in the title bar

Comment 2

[Chenxia Liu [:liuche]]

Karim, I'm going to assign this to you too - this code is adding a preference to Settings > Display. The xml for the display preferences are here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/xml/preferences_display.xml

So basically, you'll need to add an item to the preferences xml, and have that handle whether the mic icon shows up or not.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Went over the copy with Karim, taking into account localization and the subtitle..

Title: Dictation
Subtitle: Allow voice input in the title bar

Comment 4

[Karim Benhmida [:karim]]

Attachment: MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Bug 1153904 - Add item in Settings for Voice input. r=liuche

Comment 5

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/9523/#review8557

::: mobile/android/base/resources/xml/preferences_display.xml:36
(Diff revision 1)
> +    <CheckBoxPreference android:key="android.not_a_preference.voice_search"

Let's make this key a little more specific so that it gives context to what the boolean means. Perhaps voice_search_enabled?

::: mobile/android/base/toolbar/ToolbarEditText.java:484
(Diff revision 1)
> +        if (voiceIsEnabled) {

Can we combine these into a simpler boolean check, such as voiceIsEnabled && NIGHTLY && supportsVoice?

I think the existing code actually fails in the situation where you install the voice support, start Nightly, and then remove voice support (because we never clear the touch listener).

::: mobile/android/base/preferences/GeckoPreferences.java:784
(Diff revision 1)
> +                } else if (AppConstants.RELEASE_BUILD &&

Actually, we want this on Nightly only (since Voice integration is on nightly only right now). You should check that !AppConstants.NIGHTLY_BUILD (or something) instead. Be sure to update the comment too.

::: mobile/android/base/preferences/GeckoPreferences.java:785
(Diff revision 1)
> +                        PREFS_VOICE_SEARCH.equals(key)) {

We also need to add in a check for supportsVoiceRecognizer, the way we do in the ToolbarEditText - otherwise, this pref implies that Firefox will support voice integration by changing this pref, when in reality, we might not be able to support voice integration for some reason.

::: mobile/android/base/toolbar/ToolbarEditText.java:481
(Diff revision 1)
> +        final SharedPreferences sharedPrefs = GeckoSharedPrefs.forApp(getContext());

You can use mContext here, and this should be forProfile(), instead of forApp() - this preference should be something that is specific to each user's profile, not controlled globally.

Comment 6

[Karim Benhmida [:karim]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Bug 1153904 - Add item in Settings for Voice input. r=liuche

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

https://reviewboard.mozilla.org/r/9525/#review8621

Overall I suggest you add a pref to AppConstants (and perhaps confvars.sh to drive it) that's `VOICE_SEARCH_ENABLED`, just as we did with tab queue. That can initially be `= NIGHTLY_BUILD`. This will enable us to turn off the feature later if we need to, and it also allows you to make this code more descriptive and efficient.

::: mobile/android/base/toolbar/ToolbarEditText.java:110
(Diff revisions 1 - 2)
> +        mVoiceOnTouchListener = new VoiceSearchOnTouchListener();

It seems wrong to be doing this if the feature is disabled, no?

At the very least, if HardwareUtils says we can't support it, or if we're not in NIGHTLY_BUILD, we ought to not bother.

::: mobile/android/base/preferences/GeckoPreferences.java:785
(Diff revisions 1 - 2)
> -                        PREFS_VOICE_SEARCH.equals(key)) {
> +                        PREFS_VOICE_SEARCH_ENABLED.equals(key) && HardwareUtils.supportsVoiceRecognizer(getApplicationContext(), getResources())) {

This conditional seems wrong.

The feature should be unconditionally off if we don't support voice recognition or we're not in Nightly, but we *should* show the preference regardless of the current value of the pref!

So I think this should be:

```
} else if (!AppConstants.NIGHTLY_BUILD ||
           !HardwareUtils.supportsVoiceRecognizer(getApplicationContext(), getResources())) {
   …
}
```

::: mobile/android/base/toolbar/ToolbarEditText.java:486
(Diff revisions 1 - 2)
> +        final boolean voiceIsSupported = HardwareUtils.supportsVoiceRecognizer(getContext(), getResources());

Flip these two conditions around and don't bother reading the pref or grabbing SharedPreferences if !voiceIsSupported.

Also extract constants and be consistent: mContext versus getContext().

Indeed, if you pull this out into a method or two, you can make this all neat:

```
  private boolean voiceIsSupported() {
      return HardwareUtils.supportsVoiceRecognizer(getContext(), getResources());
  }

  private boolean voiceIsEnabled() {
      if (!voiceIsSupported()) {
          return false;
      }
      return GeckoSharedPrefs.forApp(getContext())
                             .getBoolean(PREF_VOICE_SEARCH_ENABLED, false);
  }
```

::: mobile/android/base/util/HardwareUtils.java:11
(Diff revision 2)
> +import org.mozilla.gecko.R;

All of this indicates that this shouldn't be in HardwareUtils -- it's nothing to do with hardware, it's about the intent handlers available on the device.

Make a `VoiceRecognizerUtils` class in util/ instead.

Don't forget to add it to mobile/android/base/moz.build.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chenxia Liu [:liuche] from comment #5)

> ::: mobile/android/base/toolbar/ToolbarEditText.java:481
> (Diff revision 1)
> > +        final SharedPreferences sharedPrefs = GeckoSharedPrefs.forApp(getContext());
> 
> You can use mContext here, and this should be forProfile(), instead of
> forApp() - this preference should be something that is specific to each
> user's profile, not controlled globally.

I don't think forProfile() works in Settings. See bug 1162509. I think we need to use forApp() until bug 1162509 is fixed. We really need to fix bug 1162509, it's bad news.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #7)
> Comment on attachment 8611580 [details]
> MozReview Request: Bug 1153904 - Add item in Settings for Voice input.
> r=liuche
> 
> https://reviewboard.mozilla.org/r/9525/#review8621
> 
> Overall I suggest you add a pref to AppConstants (and perhaps confvars.sh to
> drive it) that's `VOICE_SEARCH_ENABLED`, just as we did with tab queue. That
> can initially be `= NIGHTLY_BUILD`. This will enable us to turn off the
> feature later if we need to, and it also allows you to make this code more
> descriptive and efficient.

Do we think this feature is large enough to warrant it's own build flag? Or should we just use NIGHTLY_BUILD until we want it to ride the trains? I worry about the proliferation of build flags which become obsolete quickly.

Comment 10

[Karim Benhmida [:karim]]

https://reviewboard.mozilla.org/r/9525/#review8731

> This conditional seems wrong.
> 
> The feature should be unconditionally off if we don't support voice recognition or we're not in Nightly, but we *should* show the preference regardless of the current value of the pref!
> 
> So I think this should be:
> 
> ```
> } else if (!AppConstants.NIGHTLY_BUILD ||
>            !HardwareUtils.supportsVoiceRecognizer(getApplicationContext(), getResources())) {
>    …
> }
> ```

The conditional was wrong, but we still need to check for the pref value. What happens is we loop through all the display preferences, so we first have to find the voice search preference, then remove it if voice is not supported or we're not in Nightly, if we omit that it'll remove all the preferences.

Comment 11

[Karim Benhmida [:karim]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Bug 1153904 - Add item in Settings for Voice input. r=liuche

Comment 12

[Karim Benhmida [:karim]]

https://reviewboard.mozilla.org/r/9523/#review8733

> You can use mContext here, and this should be forProfile(), instead of forApp() - this preference should be something that is specific to each user's profile, not controlled globally.

forProfile() not working. See Mark's comment.

Comment 13

[Karim Benhmida [:karim]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Bug 1153904 - Add item in Settings for Voice input. r=liuche

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

https://reviewboard.mozilla.org/r/9525/#review8865

Ship It!

::: mobile/android/base/locales/en-US/android_strings.dtd:248
(Diff revision 4)
> +<!ENTITY pref_voice_search_summary "Allow voice input in the title bar">

Should this be "title bar" or "location bar"?

::: mobile/android/base/locales/en-US/android_strings.dtd:247
(Diff revision 4)
> +<!ENTITY pref_voice_search "Dictation">

"Speech"?

Anthony and I are discussing this, and we'll comment in the bug.

Comment 15

[Richard Newman [:rnewman]]

Anthony: final strings?

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Richard Newman [:rnewman] from comment #14)
> Should this be "title bar" or "location bar"?

We call it title bar in other parts of our Settings, so I just stuck to that.

> "Speech"?

How about: 

Voice input
Allow dictation option in the title bar.

Comment 17

[Chenxia Liu [:liuche]]

I like "Voice input", but dictation seems too advanced, and option might be too vague.

How about:
"Allow text input by voice in title bar" ? Or maybe "text by voice" ?

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

https://reviewboard.mozilla.org/r/9525/#review8879

Just a few comments.

Also, technically, it isn't voice_search but rather voice_input, but up to you if you want to change it everywhere (incl comments).

::: mobile/android/base/toolbar/ToolbarEditText.java:502
(Diff revision 4)
> -        final Intent intent = new Intent(RecognizerIntent.ACTION_RECOGNIZE_SPEECH);
> +        if (!voiceIsSupported()) {

I don't think we need a separate private method for voiceIsSupported - the name is too similar to voiceIsEnabled as to be confusing, and it just wraps a single call anyways. Either set it as a local final variable, or call it directly.

::: mobile/android/base/toolbar/ToolbarEditText.java:483
(Diff revision 4)
> -            // Remove the mic button if we can't support the voice recognizer.
> +            final Drawable micImg = getContext().getResources().getDrawable(R.drawable.ab_mic);

I'm feeling hesitant about making this getDrawable() call every onFocus - can we make this a member variable as well and set it if it's null?

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Voice input
Allow voice dictation in the title bar

Comment 20

[Karim Benhmida [:karim]]

https://reviewboard.mozilla.org/r/9525/#review9799

> I don't think we need a separate private method for voiceIsSupported - the name is too similar to voiceIsEnabled as to be confusing, and it just wraps a single call anyways. Either set it as a local final variable, or call it directly.

This code will be replaced anyways in a future commit, because I switched to ImageButton instead of drawables since I needed to display 2 icons (voice & qr code).

Comment 21

[Karim Benhmida [:karim]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Bug 1153904 - Add item in Settings for Voice input. r=liuche

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

https://reviewboard.mozilla.org/r/9525/#review9801

One last thing - can you add string tests for the Voice dictation strings in testSettingsMenuItems? Either as a second patch, or in a follow-up bug. Follow the StringHelper methods that fetch from the strings in code instead of the hard-coded parts - there's a bug to fix those hard-coded strings too (bug 1174878 if you're curious).

Comment 23

[Karim Benhmida [:karim]]

Attachment: MozReview Request: Bug 1153904 - Voice input setting item UI test. r=liuche

Bug 1153904 - Voice input setting item UI test. r=liuche

Comment 24

[Chenxia Liu [:liuche]]

Comment on attachment 8624552 [details]
MozReview Request: Bug 1153904 - Voice input setting item UI test. r=liuche

https://reviewboard.mozilla.org/r/11715/#review10211

Great! This looks great to me :) Just make a try push on a recent pull from fx-team, and if that goes green, you can flag this for checkin.

Comment 25

[Karim Benhmida [:karim]]

Attachment: MozReview Request: Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Comment 26

[Chenxia Liu [:liuche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d15686077c8

Comment 27

[Karim Benhmida [:karim]]

Comment on attachment 8626474 [details]
MozReview Request: Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Comment 28

[Karim Benhmida [:karim]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Bug 1153904 - Add item in Settings for Voice input. r=liuche

Comment 29

[Karim Benhmida [:karim]]

Comment on attachment 8624552 [details]
MozReview Request: Bug 1153904 - Voice input setting item UI test. r=liuche

Bug 1153904 - Voice input setting item UI test. r=liuche

Comment 30

[Karim Benhmida [:karim]]

Comment on attachment 8626474 [details]
MozReview Request: Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Comment 31

[Chenxia Liu [:liuche]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

https://reviewboard.mozilla.org/r/9525/#review10597

Just a few comments + nits.

::: mobile/android/base/preferences/GeckoPreferences.java:793
(Diff revision 6)
> +                        (!AppConstants.NIGHTLY_BUILD || !VoiceRecognizerUtils.supportsVoiceRecognizer(getApplicationContext(), getResources().getString(R.string.voicesearch_prompt)))) {

Nit: Can you update this alignment to match the alignment of the other else-ifs? So second line should be one space past the first open paren.

::: mobile/android/base/toolbar/ToolbarEditText.java:482
(Diff revision 6)
> -        if (!AppConstants.NIGHTLY_BUILD || !supportsVoiceRecognizer()) {
> +        if (voiceIsEnabled() && AppConstants.NIGHTLY_BUILD) {

Nit: I'd swap these two, because NIGHTLY_BUILD is a cheaper check than voiceIsEnabled, so the short circuit will be slightly more performant.

::: mobile/android/base/toolbar/ToolbarEditText.java:503
(Diff revision 6)
> -    private Intent createVoiceRecognizerIntent() {
> +                .getBoolean("android.not_a_preference.voice_search_enabled", true);

Let's pull this default value from a DEFAULT_SEARCH_ENABLED constant that we set in AppConstants.

Comment 32

[Chenxia Liu [:liuche]]

Comment on attachment 8624552 [details]
MozReview Request: Bug 1153904 - Voice input setting item UI test. r=liuche

https://reviewboard.mozilla.org/r/11715/#review10611

Comment 33

[Chenxia Liu [:liuche]]

Comment on attachment 8626474 [details]
MozReview Request: Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

https://reviewboard.mozilla.org/r/12097/#review10551

::: mobile/android/base/BrowserApp.java:1087
(Diff revision 3)
> +        if (hasFocus) {

Add a comment here about why you're doing this.

::: mobile/android/base/preferences/GeckoPreferences.java:799
(Diff revision 3)
> +                        (!AppConstants.NIGHTLY_BUILD || !InputOptionsUtils.supportsQrCodeReader(getApplicationContext()))) {

Nit: One more space, so this openparen lines up with the first argument of this paren.

::: mobile/android/base/resources/layout/toolbar_edit_layout.xml:32
(Diff revision 3)
> +                 android:background="#00ffffff"/>

The first two values of this are the opacity, so we can omit this. I think we could actually just use @android:color/white.

::: mobile/android/base/resources/layout/toolbar_edit_layout.xml:38
(Diff revision 3)
> +                 android:background="#00ffffff"/>

Same here, for color.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:78
(Diff revision 3)
> +                    if (hasFocus) {

Let's add a comment here explaining why we're doing this on focus changes.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:81
(Diff revision 3)
> +                        }

Nit: else should be on the same line as the closing } bracket.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:88
(Diff revision 3)
> +                        }

Same, bracket/else positioning.

::: mobile/android/base/util/InputOptionsUtils.java:35
(Diff revision 3)
> +    public static Intent createQrCodeReaderIntent() {

Nit: Can you capitalize QR in this method name? Also add a comment about how Bug 602818 enables QR code input when you have this particular Android app installed.

::: mobile/android/tests/browser/robocop/testSettingsMenuItems.java:233
(Diff revision 3)
> -        if (AppConstants.NIGHTLY_BUILD && VoiceRecognizerUtils.supportsVoiceRecognizer(this.getActivity().getApplicationContext(), this.getActivity().getResources())) {
> +        if (AppConstants.NIGHTLY_BUILD && InputOptionsUtils.supportsVoiceRecognizer(this.getActivity().getApplicationContext(), this.getActivity().getResources().getString(R.string.voicesearch_prompt))) {

One last thing, maybe we should check for the category as well - but this is optional because it adds some complexity, because we need to make sure we're not checking if both voice and QR are not supported (and the category is removed).

::: mobile/android/base/preferences/GeckoPreferences.java:798
(Diff revision 3)
> +                else if (PREFS_QRCODE_ENABLED.equals(key) &&

One last thing that you should check is to make sure that you remove the "Input options" category if neither voice nor QR code is enabled. You can take a look in the Telemetry/Health Report section - although I think that is gated on compile-time static state, rather than dynamic state, but I think it will still work.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:230
(Diff revision 3)
> +                .getBoolean("android.not_a_preference.voice_input_enabled", true);
> +    }

Same as QR code - set this default value in AppConstants.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:312
(Diff revision 3)
> +                .getBoolean("android.not_a_preference.qrcode_enabled", true);

Same as before: set this default value in AppConstants.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:315
(Diff revision 3)
> +    private void launchQrCodeReader() {

Nit: QR in method name instead of Qr.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:323
(Diff revision 3)
> +                    // We can get the SCAN_RESULT_FORMAT, SCAN_RESULT_BYTES,

Let's not put this under RESULT_OK, but after this block - "The other possible results are:..."

::: mobile/android/base/toolbar/ToolbarEditLayout.java:67
(Diff revision 3)
> +        needsKeyboardDisplay = false;

Can set this in the constructor.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:129
(Diff revision 3)
> +    public void onParentFocus() {

Definitely add a comment here about why we're doing this on focus.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:145
(Diff revision 3)
> +        }

Nit: else on the same line as first closing }.

::: mobile/android/base/toolbar/ToolbarEditLayout.java:331
(Diff revision 3)
> +                        needsKeyboardDisplay = true;

Comment here, that we want to queue up a keyboard show action (which will get lost if we try to summon the keyboard now).

::: mobile/android/base/toolbar/ToolbarEditLayout.java:54
(Diff revision 3)
> +    private boolean needsKeyboardDisplay;

Add a comment to this variable explaining what it's for. You can set it here, instead of creating it in the constructor, actually.

Also, possibly rename this to showKeyboardOnFocus. I think that makes it a little more clear.

Comment 34

[Karim Benhmida [:karim]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Bug 1153904 - Add item in Settings for Voice input. r=liuche

Comment 35

[Karim Benhmida [:karim]]

Comment on attachment 8624552 [details]
MozReview Request: Bug 1153904 - Voice input setting item UI test. r=liuche

Bug 1153904 - Voice input setting item UI test. r=liuche

Comment 36

[Karim Benhmida [:karim]]

Comment on attachment 8626474 [details]
MozReview Request: Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

Comment 37

[Chenxia Liu [:liuche]]

Comment on attachment 8626474 [details]
MozReview Request: Bug 602818 - Integrate QR code scanner into Fennec. r=liuche

https://reviewboard.mozilla.org/r/12097/#review10639

Comment 38

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/b3f1aedcf9ba
https://hg.mozilla.org/integration/fx-team/rev/d6999339a68e

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3f1aedcf9ba
https://hg.mozilla.org/mozilla-central/rev/d6999339a68e

Comment 40

[Chenxia Liu [:liuche]]

Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Flipped these on reviewboard :/

Comment 41

[Flaviu Cos]

Verified as fixed in build 42.0a1 2015-07-02;
Device: Asus Transformer Pad (Android 4.2.1).