Issue 29835555: Issue 6799 - Hebrew subscription is not installed for Hebrew locale

https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java File adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java#newcode88 adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:88: } Looks good, but I believe that if you ...

July 20, 2018, 1:48 p.m. (2018-07-20 13:48:44 UTC) #3

anton

On 2018/07/20 13:48:44, diegocarloslima wrote: > https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > File > adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (right): > > ...

July 20, 2018, 1:55 p.m. (2018-07-20 13:55:23 UTC) #4

anton

On 2018/07/20 13:55:23, anton wrote: > On 2018/07/20 13:48:44, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java ...

July 20, 2018, 2 p.m. (2018-07-20 14:00:58 UTC) #5

diegocarloslima

On 2018/07/20 14:00:58, anton wrote: > On 2018/07/20 13:55:23, anton wrote: > > On 2018/07/20 ...

July 20, 2018, 4:33 p.m. (2018-07-20 16:33:05 UTC) #6

anton

On 2018/07/20 16:33:05, diegocarloslima wrote: > On 2018/07/20 14:00:58, anton wrote: > > On 2018/07/20 ...

July 23, 2018, 7:22 a.m. (2018-07-23 07:22:27 UTC) #7

anton

On 2018/07/23 07:22:27, anton wrote: > On 2018/07/20 16:33:05, diegocarloslima wrote: > > On 2018/07/20 ...

July 23, 2018, 7:31 a.m. (2018-07-23 07:31:17 UTC) #8

On 2018/07/23 07:22:27, anton wrote:
> On 2018/07/20 16:33:05, diegocarloslima wrote:
> > On 2018/07/20 14:00:58, anton wrote:
> > > On 2018/07/20 13:55:23, anton wrote:
> > > > On 2018/07/20 13:48:44, diegocarloslima wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src...
> > > > > File
> > > > >
> > >
> adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src...
> > > > >
> > > >
> > >
> >
>
adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:88:
> > > > > }
> > > > > Looks good, but I believe that if you use
> > > Locale.getDefault().toLanguageTag(),
> > > > > it will already do the following conversions of legacy locales:
> > > > > iw -> he
> > > > > ji -> yi
> > > > > in -> id
> > > 
> > > *request to lower
> > 
> > I see... maybe extract this to a utility method (CompatUtils or inside this
> > class), which uses a custom logic for devices below API 21 and for those
over
> > API 21 uses toLanguageTag()
> 
> I don't think it will be changed in android updates so i think just for
> simplicity we can just copy-paste the code from android sources (decompiled):
> `
> public static LanguageTag parseLocale(BaseLocale var0, LocaleExtensions var1)
{
>     LanguageTag var2 = new LanguageTag();
>     String var3 = var0.getLanguage();
>     String var4 = var0.getScript();
>     String var5 = var0.getRegion();
>     String var6 = var0.getVariant();
>     boolean var7 = false;
>     String var8 = null;
>     if(isLanguage(var3)) {
>       if(var3.equals("iw")) {
>         var3 = "he";
>       } else if(var3.equals("ji")) {
>         var3 = "yi";
>       } else if(var3.equals("in")) {
>         var3 = "id";
>       }
> 
>       var2.language = var3;
>     }
> `

Since we don't have another subscriptions now i think we can just leave patch
set #1

diegocarloslima

July 23, 2018, 2:10 p.m. (2018-07-23 14:10:53 UTC) #9

On 2018/07/23 07:31:17, anton wrote:
> On 2018/07/23 07:22:27, anton wrote:
> > On 2018/07/20 16:33:05, diegocarloslima wrote:
> > > On 2018/07/20 14:00:58, anton wrote:
> > > > On 2018/07/20 13:55:23, anton wrote:
> > > > > On 2018/07/20 13:48:44, diegocarloslima wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src...
> > > > > > File
> > > > > >
> > > >
> >
adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> > > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29835555/diff/29835556/adblock-android/src...
> > > > > >
> > > > >
> > > >
> > >
> >
>
adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:88:
> > > > > > }
> > > > > > Looks good, but I believe that if you use
> > > > Locale.getDefault().toLanguageTag(),
> > > > > > it will already do the following conversions of legacy locales:
> > > > > > iw -> he
> > > > > > ji -> yi
> > > > > > in -> id
> > > > 
> > > > *request to lower
> > > 
> > > I see... maybe extract this to a utility method (CompatUtils or inside
this
> > > class), which uses a custom logic for devices below API 21 and for those
> over
> > > API 21 uses toLanguageTag()
> > 
> > I don't think it will be changed in android updates so i think just for
> > simplicity we can just copy-paste the code from android sources
(decompiled):
> > `
> > public static LanguageTag parseLocale(BaseLocale var0, LocaleExtensions
var1)
> {
> >     LanguageTag var2 = new LanguageTag();
> >     String var3 = var0.getLanguage();
> >     String var4 = var0.getScript();
> >     String var5 = var0.getRegion();
> >     String var6 = var0.getVariant();
> >     boolean var7 = false;
> >     String var8 = null;
> >     if(isLanguage(var3)) {
> >       if(var3.equals("iw")) {
> >         var3 = "he";
> >       } else if(var3.equals("ji")) {
> >         var3 = "yi";
> >       } else if(var3.equals("in")) {
> >         var3 = "id";
> >       }
> > 
> >       var2.language = var3;
> >     }
> > `
> 
> Since we don't have another subscriptions now i think we can just leave patch
> set #1

ok, for sake of simplicity LGTM

Expand All Messages | Collapse All Messages