manifest.xml.in doesn't currently provide RTL support

RESOLVED FIXED

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: Niv Yahel, Assigned: Niv Yahel)

Tracking

(Blocks: 1 bug)

Trunk
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

I'm working on bug 702845. The manifest.xml.in file must be changed to targetSdkVersion 17, and the <application> tag needs to have android:supportsRtl="true" added to it.
(Assignee)

Comment 1

4 years ago
Created attachment 814402 [details] [diff] [review]
android_manifest_rtl.patch
Attachment #814402 - Flags: review?
(Assignee)

Updated

4 years ago
Blocks: 702845

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → Android
Hardware: x86 → All

Updated

4 years ago
Attachment #814402 - Flags: review? → review?(mark.finkle)
(Assignee)

Updated

4 years ago
Blocks: 924700
(Assignee)

Updated

4 years ago
Blocks: 924703
Comment on attachment 814402 [details] [diff] [review]
android_manifest_rtl.patch

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

::: mobile/android/base/AndroidManifest.xml.in
@@ +72,5 @@
>      <!-- App requires OpenGL ES 2.0 -->
>      <uses-feature android:glEsVersion="0x00020000" android:required="true" />
>  
>      <application android:label="@MOZ_APP_DISPLAYNAME@"
> +     android:supportsRtl="true"    

Drive-by review - we're going to want to align this with the other attributes below, and you should trim the trailing whitespace.
Comment on attachment 814402 [details] [diff] [review]
android_manifest_rtl.patch

This looks good to me. I want Lucas to take a look too and consider what affect moving to SDK 17 will have. I don't expect moving SDK versions will be a major dealbreaker, but I want someone besides me to be involved.

We may have other bugs on moving to SDK 17 too.

Also, as Mike suggested, please fix up your indenting.
Attachment #814402 - Flags: review?(mark.finkle)
Attachment #814402 - Flags: review?(lucasr.at.mozilla)
Attachment #814402 - Flags: feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 815049 [details] [diff] [review]
android_manifest_rtl.patch

I fixed up the indenting. For some reason, my text editor rendered the spacing wrong, so I just fixed it up in vim.
Attachment #814402 - Attachment is obsolete: true
Attachment #814402 - Flags: review?(lucasr.at.mozilla)
Attachment #815049 - Flags: review?
(Assignee)

Comment 5

4 years ago
Created attachment 815053 [details] [diff] [review]
android_manifest_rtl.patch

I realized what the problem was. I was copying and pasting the indenting, which turned tabs into spaces. I remember reading in the coding style that I should be using spaces and not indents, but I didn't want to make the changes, so I simply re-created the indenting manually.
Attachment #815049 - Attachment is obsolete: true
Attachment #815049 - Flags: review?
Attachment #815053 - Flags: review?

Updated

4 years ago
Blocks: 925108
Attachment #815053 - Flags: review? → review?(lucasr.at.mozilla)
Comment on attachment 815053 [details] [diff] [review]
android_manifest_rtl.patch

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

I can't think of any problem from targeting 17 instead of 16 (if we want to use the RTL stuff from Android).

::: mobile/android/base/AndroidManifest.xml.in
@@ +77,2 @@
>  		 android:icon="@drawable/icon"
> +		 android:name="org.mozilla.gecko.GeckoApplication"

What exactly has changed here? It usually a good idea to avoid adding whitespace changes to the patch.
Attachment #815053 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 7

4 years ago
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 815053 [details] [diff] [review]
> android_manifest_rtl.patch
> 
> Review of attachment 815053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't think of any problem from targeting 17 instead of 16 (if we want to
> use the RTL stuff from Android).
> 
> ::: mobile/android/base/AndroidManifest.xml.in
> @@ +77,2 @@
> >  		 android:icon="@drawable/icon"
> > +		 android:name="org.mozilla.gecko.GeckoApplication"
> 
> What exactly has changed here? It usually a good idea to avoid adding
> whitespace changes to the patch.

I did add a space but it was to align all of the attributes. If you like, I can submit another patch with that extra whitespace removed.
(In reply to Niv Yahel from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > Comment on attachment 815053 [details] [diff] [review]
> > android_manifest_rtl.patch
> > 
> > Review of attachment 815053 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I can't think of any problem from targeting 17 instead of 16 (if we want to
> > use the RTL stuff from Android).
> > 
> > ::: mobile/android/base/AndroidManifest.xml.in
> > @@ +77,2 @@
> > >  		 android:icon="@drawable/icon"
> > > +		 android:name="org.mozilla.gecko.GeckoApplication"
> > 
> > What exactly has changed here? It usually a good idea to avoid adding
> > whitespace changes to the patch.
> 
> I did add a space but it was to align all of the attributes. If you like, I
> can submit another patch with that extra whitespace removed.

No need to send another patch. Just mentioned it as a tip for future patches :-)

What's the plan for landing the RTL changes? Are we going to push patch as they are approved or wait until we have most of the dependent bugs fixed?

FWIW, I'd prefer to only change the manifest once we have some actual RTL support patches ready to land.
Flags: needinfo?(mark.finkle)
(Assignee)

Comment 9

4 years ago
(In reply to Lucas Rocha (:lucasr) from comment #8)
> (In reply to Niv Yahel from comment #7)
> > (In reply to Lucas Rocha (:lucasr) from comment #6)
> > > Comment on attachment 815053 [details] [diff] [review]
> > > android_manifest_rtl.patch
> > > 
> > > Review of attachment 815053 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I can't think of any problem from targeting 17 instead of 16 (if we want to
> > > use the RTL stuff from Android).
> > > 
> > > ::: mobile/android/base/AndroidManifest.xml.in
> > > @@ +77,2 @@
> > > >  		 android:icon="@drawable/icon"
> > > > +		 android:name="org.mozilla.gecko.GeckoApplication"
> > > 
> > > What exactly has changed here? It usually a good idea to avoid adding
> > > whitespace changes to the patch.
> > 
> > I did add a space but it was to align all of the attributes. If you like, I
> > can submit another patch with that extra whitespace removed.
> 
> No need to send another patch. Just mentioned it as a tip for future patches
> :-)
> 
> What's the plan for landing the RTL changes? Are we going to push patch as
> they are approved or wait until we have most of the dependent bugs fixed?
> 
> FWIW, I'd prefer to only change the manifest once we have some actual RTL
> support patches ready to land.

Thanks for the tip! I agree that this patch should only be landed once we've made more progress with adding RTL support. As of now, this patch alone breaks some of the layouts when the locale is RTL.
Agreed. Let's wait to land this patch until we have other RTL patches ready and we don't break the app when in an RTL locale.

We also need to consider what parts of RelEng need to be pinged for a target SDK bump. Do we need to change our mozconfigs?
Flags: needinfo?(mark.finkle)
Please f? me on build system changes throughout mobile/android.  Ta!

Updated

4 years ago
Blocks: 928663

Updated

4 years ago
Blocks: 928688

Updated

4 years ago
Blocks: 935388

Updated

4 years ago
Blocks: 935392

Updated

4 years ago
Blocks: 943751
Assignee: nobody → nivivon
Status: NEW → ASSIGNED
Created attachment 8507591 [details] [diff] [review]
Updated patch. v1

Updated. We handled the target SDK change elsewhere. This might break if you use very old tools, but I doubt it.

Not landing yet; needs testing and other bugs.
Attachment #815053 - Attachment is obsolete: true
Attachment #8507591 - Flags: review+
Attachment #8507591 - Flags: feedback?(nalexander)
Version: Firefox 27 → Trunk
Clearing blocks to make the dependency tree more useful.
No longer blocks: 924700, 924703, 925108, 928663, 928688, 935388, 935392, 943751
Comment on attachment 8507591 [details] [diff] [review]
Updated patch. v1

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

Sure, rubber stamp from me.  Should we be landing behind a build flag?
Attachment #8507591 - Flags: feedback?(nalexander) → feedback+
Priority: -- → P3

Updated

6 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.