Closed Bug 950698 Opened 6 years ago Closed 6 years ago

Change the text for the tip for adding more search providers from Settings->Customize->Search settings

Categories

(Firefox for Android :: General, defect)

29 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 - wontfix
firefox29 + wontfix
firefox30 --- verified
fennec 29+ ---

People

(Reporter: u421692, Assigned: liuche)

References

Details

Attachments

(4 files, 6 obsolete files)

Since "Bug 768667 - Use the action bar for text selection on ICS+" changed the way the user interacts with text fields, and the user can not long tap on the text field to add the search engine, we should change the text in Settings->Customize->Search settings to:

Expected: "TIP: Add any website to your list of search providers by tapping twice to open the action bar for text selection, and tap on the magnifying icon."

Actual: "TIP: Add any website to your list of search providers by long-pressing on its search field."
Summary: Change the TIP for adding more search providers from Settings->Customize->Search settings → Change the text for the TIP for adding more search providers from Settings->Customize->Search settings
Blocks: 768667
tracking-fennec: --- → ?
Depends on: 943890
Ian - What do we want to do with this?
Flags: needinfo?(ibarlow)
Summary: Change the text for the TIP for adding more search providers from Settings->Customize->Search settings → Change the text for the tip for adding more search providers from Settings->Customize->Search settings
I believe you still have to long press on the field to get into action mode. I would phrase it as:

TIP: Add any website to your list of search providers by long pressing on its search field, and then tapping the [add search engine icon] icon.

--

Mockup: http://cl.ly/image/310z1Y3l3R3d
Flags: needinfo?(ibarlow)
Looks like this is worth tracking to see the wording correct on the first ship with this feature - will need to make sure this is run past l10n though since we are already in Aurora for FF28.
Flags: needinfo?(jbeatty)
tracking-fennec: ? → 29+
I think it's too late to make this change in 28. Some of our locales have already completed sign-offs for their work in 28 and may not catch this change to update it before the merge day.
Flags: needinfo?(jbeatty)
Given that, we'll have to tolerate the mismatch for 28 and get this in 29.
Unless we have a breakdown of our Fennec users by locale and can at least confirm the possibility of updating this for our top locales -  that might be a good in between solution so that most of our users get the correct info.  However, it's a tooltip and not a prominent text so I'm not sure we need to panic about getting this into Aurora and breaking string freeze for it.
We missed merge for Fx29, Can we land this for Fx30, please?
There's no assignee and no patch.
Assignee: nobody → liuche
Attached image Screenshot: New search string (obsolete) —
This became much more complicated than I expected!

I think we may need another image as well, because the current long-tap one doesn't really match the text anymore.

(Need to split the patches, but they are forthcoming!)
Flags: needinfo?(ibarlow)
Attached patch Update new search tip. (obsolete) — Splinter Review
This became complicated because AFACT, there isn't a way to get to the view inside of the custom layout of a Preference without overriding the onBindView method of Preference, which is necessary to replace the search icon in the text. This patch is something that could be super-classed at some point, if it needs to be reused.

This annoying hack is another point in favor of ripping out some of the current Preferences code...

This will need aurora uplift and late-l10n.

I'll be on PTO next week, so if someone can shepherd this bug through landing and uplift, it would be much appreciated!
Attachment #8372974 - Flags: review?(bnicholson)
(In reply to Chenxia Liu [PTO until 2/18] [:liuche] from comment #9)
> Created attachment 8372723 [details]
> Screenshot: New search string
> 
> This became much more complicated than I expected!
> 
> I think we may need another image as well, because the current long-tap one
> doesn't really match the text anymore.
> 
> (Need to split the patches, but they are forthcoming!)

I think the image is fine, to be honest. The icon should be about 20% bigger though.
Flags: needinfo?(ibarlow)
Comment on attachment 8372974 [details] [diff] [review]
Update new search tip.

Review of attachment 8372974 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +80,5 @@
>       advisory message on the customise search providers settings page explaining how to add new
> +     search providers.
> +     The "%I" in the string will be replaced by a small image of the icon described, and can be moved to wherever
> +     it is applicable. Please keep the spacing around the "%I" string. -->
> +<!ENTITY pref_search_hint "TIP: Add any website to your list of search providers by long-pressing on its search field and then tapping the &#37;I icon.">

&#37;I should be factored out into a 'formatI' constant here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/strings.xml.in#12

The reason is that localizers are used to variables being formatted as "&X;", so they may accidentally remove any trailing characters (the I in this case). There's no validation that checks that they used the variable correctly, and we find only after we start seeing crashes (full history in bug 775142).

::: mobile/android/base/preferences/ModifiableHintPreference.java
@@ +19,5 @@
> +import android.widget.TextView;
> +
> +class ModifiableHintPreference extends Preference {
> +    private static final String LOGTAG = "ModifiableHintPref";
> +    private Context mContext;

Nit: final

@@ +36,5 @@
> +        mContext = context;
> +    }
> +
> +    @Override
> +    protected void onBindView(View view) {

IIRC, onBindView is called many times when prefs are created. I'd override onCreateView instead.
Attachment #8372974 - Flags: review?(bnicholson) → review+
Attachment #8378729 - Flags: review?(bnicholson)
Attached patch Part 1: Update search tip (obsolete) — Splinter Review
Called in onCreateView now.
Attachment #8372974 - Attachment is obsolete: true
Attachment #8378733 - Flags: review?(bnicholson)
Attachment #8372723 - Attachment is obsolete: true
Attachment #8378729 - Flags: review?(bnicholson) → review+
Attachment #8378729 - Attachment is obsolete: true
Attachment #8378736 - Flags: review?(bnicholson)
Attachment #8378736 - Flags: review?(bnicholson) → review+
Comment on attachment 8378733 [details] [diff] [review]
Part 1: Update search tip

Review of attachment 8378733 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/preferences/ModifiableHintPreference.java
@@ +45,5 @@
> +        return thisView;
> +    }
> +
> +    private void configurePreferenceView(View view) {
> +        Log.e("AOEU", "configurePreferenceView called");

Drop logging

@@ +50,5 @@
> +        TextView textView = (TextView) view.findViewById(RESID_TEXT_VIEW);
> +        String searchHint = textView.getText().toString();
> +
> +        // Use an ImageSpan to include the "add search" icon in the Tip.
> +        int imageSpanIndex = searchHint.indexOf("%I");

Might want to factor "%I" into a String constant...

@@ +61,5 @@
> +            ImageSpan searchIcon = new ImageSpan(drawable);
> +            final SpannableStringBuilder hintBuilder = new SpannableStringBuilder(searchHint);
> +
> +            // Insert the image.
> +            hintBuilder.setSpan(searchIcon, imageSpanIndex, imageSpanIndex + 2, Spanned.SPAN_INCLUSIVE_INCLUSIVE);

...and if you do, use length() here instead of a hardcoded 2.
Attachment #8378733 - Flags: review?(bnicholson) → review+
Lift "%I" into a constant. Carrying over r+.
Attachment #8378736 - Attachment is obsolete: true
Attachment #8379224 - Flags: review+
(qref)
Attachment #8379224 - Attachment is obsolete: true
Attachment #8379226 - Flags: review+
Addressed review comments, carrying over r+.
Attachment #8378733 - Attachment is obsolete: true
Attachment #8379227 - Flags: review+
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 768667 changed the way to add search engines
User impact if declined: Hint to add search engine won't match actual pathway 
Testing completed (on m-c, etc.): Local testing
Risk to taking this patch (and alternatives if risky): very low, string changes
String or IDL/UUID changes made by this patch: One string change, late-l10n
Attachment #8379231 - Flags: approval-mozilla-aurora?
Target Milestone: --- → Firefox 30
I will wait for this patch to land in m-c before uplifting it
Attachment #8379231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on Nightly 30.0a1(2014-02-23).
Is there anything I need to do for this as late-l10n?
Flags: needinfo?(jbeatty)
(In reply to Chenxia Liu [:liuche] from comment #27)
> Is there anything I need to do for this as late-l10n?

What's the thinking for getting this into 29 instead of 30, as Erin suggested in comment 7? If this were within the first two weeks of the cycle, I wouldn't be so reluctant, but seeing as we're now halfway through, I'm more reluctant.
Flags: needinfo?(jbeatty)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #26)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/1ed9ed99b763

Can we back this out? I'd like to know the approval process here, cause I don't think any of the l10n drivers were asked about the impact for landing this at this point of the cycle.
Backed out of aurora, and clearing the late-l10n keyword. Sorry for the confusion here, I thought late-l10n meant this was good to uplift, my mistake.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2180eeda96b
Keywords: late-l10n
Going out on a limb that means this is wontfix for 28/29.
You need to log in before you can comment on or make changes to this bug.