Closed Bug 1024527 Opened 6 years ago Closed 6 years ago

Integrate Android Search Activity strings into Fennec

Categories

(Firefox for Android Graveyard :: Search Activity, defect, P1)

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

(Whiteboard: shovel-ready)

Attachments

(2 files)

Over in Bug 1021864, we're ironing out how to integrate the Search Activity into the Fennec APK.  For context, the Search Activity will be integrated in a similar manner to Android Sync: to date it has been developed as a separate (github) repository; it is essentially independent of Fennec proper; and we'd like to maintain some of that relationship for easier development and testing.
What I propose is that Search Activity's strings.xml be pre-processed (like Android Sync's).  strings.xml.in will contain search_strings.dtd(.in), just like Android Sync.  That dtd *should* live in

mobile/android/search/locales/en-US/search_strings.dtd

or similar.

*First ask*: is that okay for localizers?  Can we add new dtd roots into the tree?  (I really hope so.)

Once we have that dtd in place, I propose to do all the localization as part of mobile/android/base/locales.  That is, the Search Activity is only ever localized as part of building Fennec proper.  The alternative is to copy the mobile/android/base/locales logic to mobile/android/search/locales.  I really don't want to do that because 1) the paths are special-cased throughout the builders; 2) having the locales be handled separately makes the build system Makefiles much harder to maintain.

*Second ask*: is localizing "Fennec extras" like the Search Activity only as part of the Fennec APK acceptable?

Asking Pike for l10n perspective, glandium for build-system and l10n packaging perspective.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
rnewman: I'm going to assert that the Search Activity ignores the Fennec locale chooser for a good long time, possibly indefinitely.  That is: the Search Activity runs in your system locale, regardless of your Fennec choice.  Claiming otherwise is equivalent to signing up to do the LocaleManager extraction-into-library work :)
Flags: needinfo?(rnewman)
(In reply to Nick Alexander :nalexander from comment #2)
> Claiming otherwise is equivalent to signing up to do the
> LocaleManager extraction-into-library work :)

If it's in the same APK, can't we take the same approach that we do for Sync's activities?

That is, dump a stub {Browser,}LocaleManager into the search activity repo, and call BrowserLocaleManager.getInstance() during activity creation (or later if it doesn't immediately show strings).

The right thing will happen when the non-stub class is available.

See Bug 970176 for reference.

Am I missing something?
Flags: needinfo?(rnewman)
Sadly, the file locations we can can use for Android are special, because of the clusterfucked \ escaping story in xml that we need for android, and must not have anywhere else.

That logic is only triggered for android/base/locales/en-US and embedding/android/locales/en-US.

It'd be an easy patch to change, but a whole lot of shebang to actually deploy it everywhere.

If we care about the search activity being part of the fennec l10n process really depends on what the intent is, and how big of a project it is/will become.
Flags: needinfo?(l10n)
I'm not sure what the build system question is.
Flags: needinfo?(mh+mozilla)
(In reply to Axel Hecht [:Pike] from comment #4)
> Sadly, the file locations we can can use for Android are special, because of
> the clusterfucked \ escaping story in xml that we need for android, and must
> not have anywhere else.
> 
> That logic is only triggered for android/base/locales/en-US and
> embedding/android/locales/en-US.

Ah, I see this in compare-locales: special treatment for a few directories in compare-locales/lib/Mozilla/Checks.py.

I should point out that we're including mobile/android/branding/*dtd in strings.xml, presumably without these checks, but also without issue.  I reckon that's 'cuz the branding string set is small and essentially static.

> It'd be an easy patch to change, but a whole lot of shebang to actually
> deploy it everywhere.

Roger that.
 
> If we care about the search activity being part of the fennec l10n process
> really depends on what the intent is, and how big of a project it is/will
> become.

I anticipate the Search Activity being a big deal, but also that additional Projects that aren't in mobile/android/base will require Android strings (the most likely next project being the Mozilla Stumbler).

My preference is to land a new DTD in mobile/android/search/locales/en-US/search_strings.dtd and forego the special Android checking, and retro-fit our tools as necessary; but I could be convinced otherwise.
What's the status of this bug?

I do see there is a search_strings.dtd in the tree. Does this bug mean that localizers can't actually access and localize this file?

Marking this as a P1 to make sure that the search activity can be localized.
Blocks: search
Flags: needinfo?(nalexander)
Priority: -- → P1
Yes, that file is not exposed to localizers.

Also, there's no intent to spread more directories with android badness on my side. It's just way to cumbersome to do that in any reasonable timeframe.
(See Bug 709877 for historical context for sync_strings.dtd.)
Whiteboard: shovel-ready
(In reply to Axel Hecht [:Pike] from comment #8)
> Yes, that file is not exposed to localizers.
> 
> Also, there's no intent to spread more directories with android badness on
> my side. It's just way to cumbersome to do that in any reasonable timeframe.

Pike, let me make sure I'm understanding this correctly: localizers don't currently see:

* mobile/android/search/locales/en-US/search_strings.dtd

And you don't want to expose that location to localizers.  From my perspective, this is unfortunate; we want to grow *more* locations for localizers as we modularize pieces of Fennec, not *fewer*.  But we can work around, for now, perhaps: will localizers see (without additional changes):

* mobile/android/base/locales/en-US/search_strings.dtd

If so, we'll move search_strings.dtd there, and update our import scripts.
Flags: needinfo?(nalexander) → needinfo?(l10n)
According to my notes, the file is at http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/strings/search_strings.dtd.

If it moved to mobile/android/search/locales/en-US/search_strings.dtd, it'd be possible to hook that up to l10n.ini, but that wouldn't trigger the awful android logic on there.

mobile/android/base/locales/en-US/search_strings.dtd will be fine.

I strongly believe that we do not want to spread localization files through various totally disconnected sections in the source code. The product we're localizing is fennec, just fennec. There are a few strings, just keep them together.
Flags: needinfo?(l10n)
nalexander, can you own this bug?
Flags: needinfo?(nalexander)
Yes.  Leaving the NI so it's in my queue.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Attachment #8486625 - Flags: review?(margaret.leibovic)
Flags: needinfo?(nalexander)
Pike: can you confirm that no further changes are needed in l10n.ini
for localizers to see the new search_strings.dtd?  Thanks.
Attachment #8486629 - Flags: review?(l10n)
Comment on attachment 8486629 [details] [diff] [review]
Move search_strings.dtd into m/a/base/locales/en-US. r=Pike

Technically, this is OK.

But the search settings stuff should be migrated into region.properties, http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/strings/search_strings.dtd#23 is just a big basket of localizer fail.

Also, Karen, what's the policy around that feature?
Attachment #8486629 - Flags: review?(l10n) → review-
Flags: needinfo?(krudnitski)
(In reply to Axel Hecht [:Pike] from comment #16)
> Comment on attachment 8486629 [details] [diff] [review]
> Move search_strings.dtd into m/a/base/locales/en-US. r=Pike
> 
> Technically, this is OK.

Thanks for the quick turn-around.
 
> But the search settings stuff should be migrated into region.properties,
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/strings/
> search_strings.dtd#23 is just a big basket of localizer fail.

Migrating these settings into region.properties is almost certainly out of scope for Fx 35 due to technical divisions between Fennec proper and the Search Activity.  I propose we land this or something very close to it so that we can localize in 35; and then figure out what a region.properties system looks like.

Can you elaborate on what you mean by "localizer fail" here?  Do you mean that localizers have no particular knowledge of searchplugins, so exposing something like this to localizers at all is a bad idea?
Flags: needinfo?(l10n)
The thing is, you're not going to get this properly localized by some entry in a DTD.

They're going to do all kinds of things to the "yahoo" string, ignoring the comment you put on top. And even if we landed the right string once, they're still going to regress it merrily. Sad, but true.

region.properties on the other hand is not exposed to regular translation tools, but only changes when technically savvy people edit it. Which is why I asked kar for context on how and why to do what.

Can you bundle region.properties into the search activity in addition to what search_strings.dtd does? It's going to require different code to get the localized value, but it'll be the stable way forward, given our experience with these kinds of settings.
Flags: needinfo?(l10n)
Adding some more thoughts on the subject.

* region.properties
That's our 'safe' place for productization settings. Localizers are instructed to not touch those files, and changes are easier to spot when reviewing sign-offs (localizers sign-off a changeset for shipping, l10n-drivers review it for errors before accepting it).

Also, we have external localization tools that explicitly ignore that file, so localizers won't even see it.

Take a look at this page
http://l10n.mozilla-community.org/~flod/p12n/errors/

"Firefox Mobile" is now in a clean state (ga-IE has been fixed, we have a bug on file for zh-CN), as Firefox desktop. They weren't a year ago before I started cleaning up.

* Policy
As Pike said, there's also a "policy" question: are localized supposed to keep that value, or can they change it? That's probably a question for BD.

* default_engine_identifier
I'm not sure about the choice of using the filename as value. For a similar setting, we use the 'name' attribute of the searchplugin, not the filename.
http://hg.mozilla.org/mozilla-central/file/152ef25e89ae/mobile/locales/en-US/chrome/region.properties#l6

What happens if localizers screw up and specify a non existing searchplugin? Do we have a fallback mechanism?
(In reply to Francesco Lodolo [:flod] from comment #19)
> Adding some more thoughts on the subject.
> 
> * region.properties
> That's our 'safe' place for productization settings. Localizers are
> instructed to not touch those files, and changes are easier to spot when
> reviewing sign-offs (localizers sign-off a changeset for shipping,
> l10n-drivers review it for errors before accepting it).
> 
> Also, we have external localization tools that explicitly ignore that file,
> so localizers won't even see it.
>
> Take a look at this page
> http://l10n.mozilla-community.org/~flod/p12n/errors/
> 
> "Firefox Mobile" is now in a clean state (ga-IE has been fixed, we have a
> bug on file for zh-CN), as Firefox desktop. They weren't a year ago before I
> started cleaning up.
> 
> * Policy
> As Pike said, there's also a "policy" question: are localized supposed to
> keep that value, or can they change it? That's probably a question for BD.
> 
> * default_engine_identifier
> I'm not sure about the choice of using the filename as value. For a similar
> setting, we use the 'name' attribute of the searchplugin, not the filename.
> http://hg.mozilla.org/mozilla-central/file/152ef25e89ae/mobile/locales/en-US/
> chrome/region.properties#l6

I decided to use the file name (the 'identifier' attribute for the plugin) because that seemed simpler than the 'name' attirbute in the plugin. Do you happen to know the history behind that decision for the regular search engines?

> What happens if localizers screw up and specify a non existing searchplugin?
> Do we have a fallback mechanism?

This is bug 1065123, which I am going to fix :)

nalexander, is there a way for us to get at region.properties in the search activity without needing to interact with gecko? It would be nice to be able to manage all our search engine defaults in one place.
Comment on attachment 8486625 [details] [review]
Link to Github pull-request: https://github.com/mozilla/fennec-search/pull/86

This looks good to me, but we do need to sort out the default_engine_identifier issue. However, I think that should happen in a separate patch, either here or in a follow-up bug.
Attachment #8486625 - Flags: review?(margaret.leibovic) → review+
If this lands, I would be much happier with an hard-coded "Yahoo" somewhere out of string files while we figure out the story behind it (especially the policy part).
(In reply to :Margaret Leibovic from comment #20)
> I decided to use the file name (the 'identifier' attribute for the plugin)
> because that seemed simpler than the 'name' attirbute in the plugin. Do you
> happen to know the history behind that decision for the regular search
> engines?

It's too far away in the past for me, maybe gavin or mconnor (or pike) could have some ideas.
Depends on: 1065306
Update: In bug 1065123 I'm going to write a patch to remove this string from search_strings.dtd, so that we won't be blocked on that problem anymore.

I filed bug 1065306 to come up with a real solution for this localized-default engine issue.
Depends on: 1065123
Regarding policy, I presume this refers to things like the list itself (number of providers, order, etc). My expectation is that it mirrors that of our current search provider options. I would want the default that someone sets in the settings menu to also reflect that in the search activity page. Same goes for the list - if DE has a set of providers today, that same list should be reflected in the search activity. This also goes for any 'rules' for default that we have (ie we decide to change Bing across a set of locales, this also must be reflected in the search activity page).

Is that what we're referring to here? And does the above make sense?
Flags: needinfo?(krudnitski)
Policy = default search engine, does it need to be Yahoo (it's Google for the standard search)? 
Can it be changed by locales? For example I'd need to check, but we don't have Yahoo in all locales we ship on Fennec.
I'm going to suggest that we mimic the list of the locale's current list of search providers. We can look at possibly modifying the list, but I would think that would really confuse our users (two separate provider lists).

That does mean if a user changes their default engine to, say, DDG, then that would also be reflected in the activity. I say this again because I worry users wouldn't understand the difference - for them, a search is a search is a search under the Firefox umbrella.

Right now, Google is the default provider globally. When we look at moving Bing into that position, we have been discussing making that change on a subset of locales (at first). So we need that flexibility. I would also maintain that after any stated defaults and order that may be required, then the locales can choose the engines that best serve their community as per current guidelines. Again, however, to mimic the current list of providers reflected when typing in the URL bar.
So, reading Kar's statements, we actually don't want and need a different configuration for search activity settings? Also, that the current setting in en-US as yahoo is a bug?
the original intent of this project was to investigate and be bolder in terms of how we launch searches, how we present queries and how we present results. More freedom was required in designing this as to see what could work well. Also Yahoo provided more creative freedom than Google as well around how results were presented.

the 'first launch' of this project, however, is more conservative in nature. Therefore I assert that we need to preserve the flexibility to have different settings, but upon initial implementation should mimic the current provider lists for each locale.

we're phasing these bits and I expect us to be more creative in nightly while more conservative in our beta & release channels to ensure we get the 'product' right (avoid user confusion, ensure provider buy-in, get the right UX on all fronts, etc)
(In reply to Karen Rudnitski [:kar] from comment #27)
> I'm going to suggest that we mimic the list of the locale's current list of
> search providers. We can look at possibly modifying the list, but I would
> think that would really confuse our users (two separate provider lists).

The way it's currently implemented, the list is the same, since we're pulling the search plugins out of the same directory.

> That does mean if a user changes their default engine to, say, DDG, then
> that would also be reflected in the activity. I say this again because I
> worry users wouldn't understand the difference - for them, a search is a
> search is a search under the Firefox umbrella.

This is different right now because search settings are managed by gecko, and the search activity doesn't depend on gecko at all (and we don't want it to, for performance reasons).

(In reply to Karen Rudnitski [:kar] from comment #29)
> the original intent of this project was to investigate and be bolder in
> terms of how we launch searches, how we present queries and how we present
> results. More freedom was required in designing this as to see what could
> work well. Also Yahoo provided more creative freedom than Google as well
> around how results were presented.
> 
> the 'first launch' of this project, however, is more conservative in nature.
> Therefore I assert that we need to preserve the flexibility to have
> different settings, but upon initial implementation should mimic the current
> provider lists for each locale.

To be clear, we don't have anything in place to support a different list of search engines for the search activity, but we *do* have a separate default engine pref for it.

If it's not a blocker to shipping, I would really prefer to keep the search activity search engine pref separate from the Fennec search engine pref (if the user changes them), since making them the same would require a non-trivial amount of work.
(In reply to :Margaret Leibovic from comment #30)

> To be clear, we don't have anything in place to support a different list of
> search engines for the search activity, but we *do* have a separate default
> engine pref for it.
> 
> If it's not a blocker to shipping, I would really prefer to keep the search
> activity search engine pref separate from the Fennec search engine pref (if
> the user changes them), since making them the same would require a
> non-trivial amount of work.

I think the separate default engine pref makes sense since Search and Fennec are two different use cases. I want to see how it plays out: Do people tend to want to keep Search and Fennec defaults the same or not?
It makes sense provided a user understands these are separated, which I'm not sure they would (I certainly wouldn't). I'm also thinking that our providers would feel that this devalues default and may request a certain behaviour if we're included in the agreements [just keeping this in the back of our minds]. 

However, I would suggest the following:
- ensure the default set on shipping is the same default as the current list
- upon first release, we can see how many people change the default(s) and use the search activity vs in-browser search (and whether they notice a difference in default). 
- ensure we *can* link the two for possibly commercial reasons. We'll know whether this is required or not in the coming month or two, I reckon. 

TBH, I'm still concerned about user expectations. My hypothesis is that users have no idea that these are separate things, and I'm not sure in this case that they should be treated as different thing. But I'm willing to let it test out a bit.
(In reply to Karen Rudnitski [:kar] from comment #32)
 
> TBH, I'm still concerned about user expectations. My hypothesis is that
> users have no idea that these are separate things, and I'm not sure in this
> case that they should be treated as different thing. But I'm willing to let
> it test out a bit.

I take the opposite stance. They are two different applications. Why would people assume the defaults are the same? I wouldn't.
I would :)

So let's check out the hypotheses and how it pans out.
(In reply to Francesco Lodolo [:flod] from comment #22)
> If this lands, I would be much happier with an hard-coded "Yahoo" somewhere
> out of string files while we figure out the story behind it (especially the
> policy part).

OK, this has landed: Bug 1065123.  So the immediate l10n blocker is out of the way.

I am going to interpret:

1) this change, which removes Pike's (and flod's) immediate concern;
2) follow-up Bug 1065123, which will address Pike's longer concern; and
3) Pike's "technically OK" in https://bugzilla.mozilla.org/show_bug.cgi?id=1024527#c16

as r=margaret,Pike.  Let's get these strings out to localizers (well, the few who localize Nightly) and move on to Bug 1065123.
(In reply to Nick Alexander :nalexander from comment #35)
> (In reply to Francesco Lodolo [:flod] from comment #22)
> > If this lands, I would be much happier with an hard-coded "Yahoo" somewhere
> > out of string files while we figure out the story behind it (especially the
> > policy part).
> 
> OK, this has landed: Bug 1065123.  So the immediate l10n blocker is out of
> the way.
> 
> I am going to interpret:
> 
> 1) this change, which removes Pike's (and flod's) immediate concern;
> 2) follow-up Bug 1065123, which will address Pike's longer concern; and

That should be Bug 1065306.

> 3) Pike's "technically OK" in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1024527#c16
> 
> as r=margaret,Pike.  Let's get these strings out to localizers (well, the
> few who localize Nightly) and move on to Bug 1065123.

Also, that should be Bug 1065306.
FYI, I filed bug 1065891 about keeping the default engine prefs in sync.
https://hg.mozilla.org/mozilla-central/rev/10c93ff9783c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.