Add item in Settings for Voice input

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: antlam, Assigned: karim)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 41
All
Android
Points:
---

Firefox Tracking Flags

(firefox41 fixed, firefox42 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Flags: needinfo?(jhugman)
(Reporter)

Comment 1

3 years ago
Title: Search dictation 
Subtitle 1: Disabled
Subtitle 2: Enabled in the title bar
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.
Assignee: nobody → kbenhmida
Flags: needinfo?(jhugman)
(Reporter)

Comment 3

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

Title: Dictation
Subtitle: Allow voice input in the title bar
Flags: needinfo?(kbenhmida)
(Assignee)

Updated

3 years ago
Flags: needinfo?(kbenhmida)
(Assignee)

Comment 4

3 years ago
Created 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
Attachment #8611580 - Flags: review?(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.
(Assignee)

Comment 6

3 years ago
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
Attachment #8611580 - Flags: feedback+ → review?(liuche)
Attachment #8611580 - Flags: review?(liuche) → review?(rnewman)
Status: NEW → ASSIGNED
Component: General → Settings and Preferences
Hardware: x86 → All
Attachment #8611580 - Flags: review?(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.
(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.
(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.
(Assignee)

Comment 10

3 years ago
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.
(Assignee)

Comment 11

3 years ago
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
Attachment #8611580 - Flags: review?(liuche)
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8611580 - Flags: review?(rnewman)
(Assignee)

Comment 13

3 years ago
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
Attachment #8611580 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Attachment #8611580 - Flags: review?(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.
Attachment #8611580 - Flags: review?(rnewman) → review+
Anthony: final strings?
Flags: needinfo?(alam)
(Reporter)

Comment 16

3 years ago
(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.
Flags: needinfo?(alam) → needinfo?(rnewman)
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" ?
Flags: needinfo?(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/#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?
Attachment #8611580 - Flags: review?(liuche) → review+
(Reporter)

Comment 19

3 years ago
Voice input
Allow voice dictation in the title bar
(Assignee)

Comment 20

3 years ago
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).
(Assignee)

Updated

3 years ago
Attachment #8611580 - Flags: review?(liuche)
Attachment #8611580 - Flags: review+
(Assignee)

Comment 21

3 years ago
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 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).
Attachment #8611580 - Flags: review?(liuche) → review+
(Assignee)

Comment 23

3 years ago
Created 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
Attachment #8624552 - Flags: review?(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.
Attachment #8624552 - Flags: review?(liuche) → review+
(Assignee)

Comment 25

3 years ago
Created 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
Attachment #8626474 - Flags: review?(liuche)
(Assignee)

Comment 27

3 years ago
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
(Assignee)

Comment 28

3 years ago
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
Attachment #8611580 - Flags: review+ → review?(liuche)
(Assignee)

Comment 29

3 years ago
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
Attachment #8624552 - Flags: review+ → review?(liuche)
(Assignee)

Comment 30

3 years ago
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 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.
Attachment #8611580 - Flags: review?(liuche) → review+
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.
Attachment #8626474 - Flags: review?(liuche)
(Assignee)

Comment 34

3 years ago
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
Attachment #8611580 - Flags: review+ → review?(liuche)
(Assignee)

Updated

3 years ago
Attachment #8624552 - Flags: review+ → review?(liuche)
(Assignee)

Comment 35

3 years ago
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
(Assignee)

Comment 36

3 years ago
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
Attachment #8626474 - Flags: review?(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
Attachment #8626474 - Flags: review?(liuche) → review+
https://hg.mozilla.org/mozilla-central/rev/b3f1aedcf9ba
https://hg.mozilla.org/mozilla-central/rev/d6999339a68e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment on attachment 8611580 [details]
MozReview Request: Bug 1153904 - Add item in Settings for Voice input. r=liuche

Flipped these on reviewboard :/
Attachment #8611580 - Flags: review?(liuche) → review+

Comment 41

3 years ago
Verified as fixed in build 42.0a1 2015-07-02;
Device: Asus Transformer Pad (Android 4.2.1).
status-firefox42: --- → verified
You need to log in before you can comment on or make changes to this bug.