Closed
Bug 1153904
Opened 9 years ago
Closed 9 years ago
Add item in Settings for Voice input
Categories
(Firefox for Android Graveyard :: Settings and Preferences, defect)
Tracking
(firefox41 fixed, firefox42 verified)
RESOLVED
FIXED
Firefox 41
People
(Reporter: antlam, Assigned: karim)
References
Details
Attachments
(3 files)
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•9 years ago
|
Flags: needinfo?(jhugman)
Reporter | ||
Comment 1•9 years ago
|
||
Title: Search dictation Subtitle 1: Disabled Subtitle 2: Enabled in the title bar
Comment 2•9 years ago
|
||
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•9 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•9 years ago
|
Flags: needinfo?(kbenhmida)
Assignee | ||
Comment 4•9 years ago
|
||
Bug 1153904 - Add item in Settings for Voice input. r=liuche
Attachment #8611580 -
Flags: review?(liuche)
Comment 5•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8611580 -
Flags: review?(liuche) → feedback+
Assignee | ||
Comment 6•9 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)
Updated•9 years ago
|
Attachment #8611580 -
Flags: review?(liuche) → review?(rnewman)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Component: General → Settings and Preferences
Hardware: x86 → All
Updated•9 years ago
|
Attachment #8611580 -
Flags: review?(rnewman)
Comment 7•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
(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•9 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•9 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•9 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•9 years ago
|
Attachment #8611580 -
Flags: review?(rnewman)
Assignee | ||
Comment 13•9 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•9 years ago
|
Attachment #8611580 -
Flags: review?(rnewman)
Comment 14•9 years ago
|
||
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+
Reporter | ||
Comment 16•9 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)
Comment 17•9 years ago
|
||
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 18•9 years ago
|
||
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•9 years ago
|
||
Voice input Allow voice dictation in the title bar
Assignee | ||
Comment 20•9 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•9 years ago
|
Attachment #8611580 -
Flags: review?(liuche)
Attachment #8611580 -
Flags: review+
Assignee | ||
Comment 21•9 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 22•9 years ago
|
||
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•9 years ago
|
||
Bug 1153904 - Voice input setting item UI test. r=liuche
Attachment #8624552 -
Flags: review?(liuche)
Comment 24•9 years ago
|
||
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•9 years ago
|
||
Bug 602818 - Integrate QR code scanner into Fennec. r=liuche
Attachment #8626474 -
Flags: review?(liuche)
Assignee | ||
Comment 27•9 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•9 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•9 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•9 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 31•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8624552 -
Flags: review?(liuche) → review+
Comment 32•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
Attachment #8624552 -
Flags: review+ → review?(liuche)
Assignee | ||
Comment 35•9 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•9 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 37•9 years ago
|
||
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+
Comment 38•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b3f1aedcf9ba https://hg.mozilla.org/integration/fx-team/rev/d6999339a68e
Comment 39•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3f1aedcf9ba https://hg.mozilla.org/mozilla-central/rev/d6999339a68e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment 40•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8624552 -
Flags: review?(liuche)
Comment 41•9 years ago
|
||
Verified as fixed in build 42.0a1 2015-07-02; Device: Asus Transformer Pad (Android 4.2.1).
status-firefox42:
--- → verified
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•