You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by Andrew Grieve <ag...@chromium.org> on 2014/11/27 04:10:26 UTC
Code Review Plz
I've taken a stab at refactoring Android's splashscreen logic into the
splashscreen plugin.
https://github.com/apache/cordova-android/pull/134
https://github.com/apache/cordova-plugin-splashscreen/pull/32
In order to be backwards-compatible, I've:
- Committed a copy of the plugin to cordova-android and registered it
where App is registered
- Made the new code in the actual plugin disabled if it detects
cordova-android < 4.0
I've also deprecated splash-screen related things within CordovaActivity,
because I don't think the Activity needs explicit logic in it for splash
screens now that its a proper plugin (plus, you can do everything with
config.xml or CordovaPreferences).
My thinking is to delete the deprecated things when I merge into the 4.0.x,
and to also remove the bundled copy of the plugin.
To test this, I've enabled the splashscreen on Android in mobilespec. For
the life of me I couldn't get the instruments test in cordova-android to
run. My Eclipse doesn't boot, Android Studio does an awful job importing
it, and the command-line instructions in the README yield:
[aapt] invalid resource directory name:
/Users/agrieve/git/cordova/cordova-android/test/bin/res/crunch
ugh! Certainly it'd be worth getting these working, but I think mobilespec
is a better test for this anyways since I could test with & without the
plugin installed.
Andrew
Re: Code Review Plz
Posted by Joe Bowser <bo...@gmail.com>.
This looks good to me. We can tweak the existing Splashscreen tests.
On Mon Dec 08 2014 at 7:42:33 AM Michal Mocny <mm...@chromium.org> wrote:
> Sorry didn't answer. I looked a week ago and liked the design and
> rationale, but don't have existing apps actually using the feature to test
> on.
>
> On Fri, Dec 5, 2014 at 4:40 PM, Joe Bowser <bo...@gmail.com> wrote:
>
> > I'll take a look on Monday. Spent this week bashing my head against my
> desk
> > trying to deal with the MozillaView js.
> >
> > On Fri, Dec 5, 2014, 1:12 PM Andrew Grieve <ag...@chromium.org> wrote:
> >
> > > Will go ahead with this next week if no one has a look. That said,
> would
> > > love a look.
> > >
> > > On Wed, Nov 26, 2014 at 10:10 PM, Andrew Grieve <ag...@chromium.org>
> > > wrote:
> > >
> > > > I've taken a stab at refactoring Android's splashscreen logic into
> the
> > > > splashscreen plugin.
> > > >
> > > > https://github.com/apache/cordova-android/pull/134
> > > > https://github.com/apache/cordova-plugin-splashscreen/pull/32
> > > >
> > > > In order to be backwards-compatible, I've:
> > > > - Committed a copy of the plugin to cordova-android and registered
> it
> > > > where App is registered
> > > > - Made the new code in the actual plugin disabled if it detects
> > > > cordova-android < 4.0
> > > >
> > > >
> > > > I've also deprecated splash-screen related things within
> > CordovaActivity,
> > > > because I don't think the Activity needs explicit logic in it for
> > splash
> > > > screens now that its a proper plugin (plus, you can do everything
> with
> > > > config.xml or CordovaPreferences).
> > > >
> > > > My thinking is to delete the deprecated things when I merge into the
> > > > 4.0.x, and to also remove the bundled copy of the plugin.
> > > >
> > > > To test this, I've enabled the splashscreen on Android in mobilespec.
> > For
> > > > the life of me I couldn't get the instruments test in cordova-android
> > to
> > > > run. My Eclipse doesn't boot, Android Studio does an awful job
> > importing
> > > > it, and the command-line instructions in the README yield:
> > > > [aapt] invalid resource directory name:
> > > > /Users/agrieve/git/cordova/cordova-android/test/bin/res/crunch
> > > >
> > > > ugh! Certainly it'd be worth getting these working, but I think
> > > mobilespec
> > > > is a better test for this anyways since I could test with & without
> the
> > > > plugin installed.
> > > >
> > > > Andrew
> > > >
> > > >
> > > >
> > >
> >
>
Re: Code Review Plz
Posted by Michal Mocny <mm...@chromium.org>.
Sorry didn't answer. I looked a week ago and liked the design and
rationale, but don't have existing apps actually using the feature to test
on.
On Fri, Dec 5, 2014 at 4:40 PM, Joe Bowser <bo...@gmail.com> wrote:
> I'll take a look on Monday. Spent this week bashing my head against my desk
> trying to deal with the MozillaView js.
>
> On Fri, Dec 5, 2014, 1:12 PM Andrew Grieve <ag...@chromium.org> wrote:
>
> > Will go ahead with this next week if no one has a look. That said, would
> > love a look.
> >
> > On Wed, Nov 26, 2014 at 10:10 PM, Andrew Grieve <ag...@chromium.org>
> > wrote:
> >
> > > I've taken a stab at refactoring Android's splashscreen logic into the
> > > splashscreen plugin.
> > >
> > > https://github.com/apache/cordova-android/pull/134
> > > https://github.com/apache/cordova-plugin-splashscreen/pull/32
> > >
> > > In order to be backwards-compatible, I've:
> > > - Committed a copy of the plugin to cordova-android and registered it
> > > where App is registered
> > > - Made the new code in the actual plugin disabled if it detects
> > > cordova-android < 4.0
> > >
> > >
> > > I've also deprecated splash-screen related things within
> CordovaActivity,
> > > because I don't think the Activity needs explicit logic in it for
> splash
> > > screens now that its a proper plugin (plus, you can do everything with
> > > config.xml or CordovaPreferences).
> > >
> > > My thinking is to delete the deprecated things when I merge into the
> > > 4.0.x, and to also remove the bundled copy of the plugin.
> > >
> > > To test this, I've enabled the splashscreen on Android in mobilespec.
> For
> > > the life of me I couldn't get the instruments test in cordova-android
> to
> > > run. My Eclipse doesn't boot, Android Studio does an awful job
> importing
> > > it, and the command-line instructions in the README yield:
> > > [aapt] invalid resource directory name:
> > > /Users/agrieve/git/cordova/cordova-android/test/bin/res/crunch
> > >
> > > ugh! Certainly it'd be worth getting these working, but I think
> > mobilespec
> > > is a better test for this anyways since I could test with & without the
> > > plugin installed.
> > >
> > > Andrew
> > >
> > >
> > >
> >
>
Re: Code Review Plz
Posted by Joe Bowser <bo...@gmail.com>.
I'll take a look on Monday. Spent this week bashing my head against my desk
trying to deal with the MozillaView js.
On Fri, Dec 5, 2014, 1:12 PM Andrew Grieve <ag...@chromium.org> wrote:
> Will go ahead with this next week if no one has a look. That said, would
> love a look.
>
> On Wed, Nov 26, 2014 at 10:10 PM, Andrew Grieve <ag...@chromium.org>
> wrote:
>
> > I've taken a stab at refactoring Android's splashscreen logic into the
> > splashscreen plugin.
> >
> > https://github.com/apache/cordova-android/pull/134
> > https://github.com/apache/cordova-plugin-splashscreen/pull/32
> >
> > In order to be backwards-compatible, I've:
> > - Committed a copy of the plugin to cordova-android and registered it
> > where App is registered
> > - Made the new code in the actual plugin disabled if it detects
> > cordova-android < 4.0
> >
> >
> > I've also deprecated splash-screen related things within CordovaActivity,
> > because I don't think the Activity needs explicit logic in it for splash
> > screens now that its a proper plugin (plus, you can do everything with
> > config.xml or CordovaPreferences).
> >
> > My thinking is to delete the deprecated things when I merge into the
> > 4.0.x, and to also remove the bundled copy of the plugin.
> >
> > To test this, I've enabled the splashscreen on Android in mobilespec. For
> > the life of me I couldn't get the instruments test in cordova-android to
> > run. My Eclipse doesn't boot, Android Studio does an awful job importing
> > it, and the command-line instructions in the README yield:
> > [aapt] invalid resource directory name:
> > /Users/agrieve/git/cordova/cordova-android/test/bin/res/crunch
> >
> > ugh! Certainly it'd be worth getting these working, but I think
> mobilespec
> > is a better test for this anyways since I could test with & without the
> > plugin installed.
> >
> > Andrew
> >
> >
> >
>
Re: Code Review Plz
Posted by Andrew Grieve <ag...@chromium.org>.
Will go ahead with this next week if no one has a look. That said, would
love a look.
On Wed, Nov 26, 2014 at 10:10 PM, Andrew Grieve <ag...@chromium.org>
wrote:
> I've taken a stab at refactoring Android's splashscreen logic into the
> splashscreen plugin.
>
> https://github.com/apache/cordova-android/pull/134
> https://github.com/apache/cordova-plugin-splashscreen/pull/32
>
> In order to be backwards-compatible, I've:
> - Committed a copy of the plugin to cordova-android and registered it
> where App is registered
> - Made the new code in the actual plugin disabled if it detects
> cordova-android < 4.0
>
>
> I've also deprecated splash-screen related things within CordovaActivity,
> because I don't think the Activity needs explicit logic in it for splash
> screens now that its a proper plugin (plus, you can do everything with
> config.xml or CordovaPreferences).
>
> My thinking is to delete the deprecated things when I merge into the
> 4.0.x, and to also remove the bundled copy of the plugin.
>
> To test this, I've enabled the splashscreen on Android in mobilespec. For
> the life of me I couldn't get the instruments test in cordova-android to
> run. My Eclipse doesn't boot, Android Studio does an awful job importing
> it, and the command-line instructions in the README yield:
> [aapt] invalid resource directory name:
> /Users/agrieve/git/cordova/cordova-android/test/bin/res/crunch
>
> ugh! Certainly it'd be worth getting these working, but I think mobilespec
> is a better test for this anyways since I could test with & without the
> plugin installed.
>
> Andrew
>
>
>