Apache Cordova
  1. Apache Cordova
  2. CB-572

Refactor the Splashscreen API so that the code is outside of DroidGap

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.7.0, 1.8.0, 1.8.1, 1.9.0
    • Fix Version/s: 2.0.0
    • Component/s: Android
    • Labels:
      None

      Description

      It would make more sense if we had the code in the plugin somehow, and added the necessary methods to CordovaInterface. This way we can keep things more consistent to those who have the unfortunate task of combining DroidGap with a MapActivity.

        Activity

        Simon MacDonald made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Won't Fix [ 2 ]
        Hide
        Simon MacDonald added a comment -

        We've decided not to move the code out of DroidGap as it would not make sense. If someone is embedding the CordovaWebView in their own application then can provide thier own splash screen functionality.

        Show
        Simon MacDonald added a comment - We've decided not to move the code out of DroidGap as it would not make sense. If someone is embedding the CordovaWebView in their own application then can provide thier own splash screen functionality.
        Hide
        Aurelien MERCIER added a comment -

        That's fine for me. I was more concerned about this task since I had an issue with pushnotification in our application. The push starts a new activity in launching our application, unfortunately in 1.7.0/1.8.0 we were unable to hide the splashscreen. However with 1.9.0 this issue has been fixed.
        Moreover, it seems Simon had several issues with his prototype which let me think that's probably not a good idea to spend more time on it just to refactor the code.

        Show
        Aurelien MERCIER added a comment - That's fine for me. I was more concerned about this task since I had an issue with pushnotification in our application. The push starts a new activity in launching our application, unfortunately in 1.7.0/1.8.0 we were unable to hide the splashscreen. However with 1.9.0 this issue has been fixed. Moreover, it seems Simon had several issues with his prototype which let me think that's probably not a good idea to spend more time on it just to refactor the code.
        Hide
        Joe Bowser added a comment -

        @Aurelien: I agree with Simon that this doesn't make sense for CordovaWebView. Can you explain why this would make sense? Otherwise I think we should close this one.

        Show
        Joe Bowser added a comment - @Aurelien: I agree with Simon that this doesn't make sense for CordovaWebView. Can you explain why this would make sense? Otherwise I think we should close this one.
        Filip Maj made changes -
        Fix Version/s 2.0.0 [ 12318875 ]
        Fix Version/s 1.9.0 [ 12319551 ]
        Affects Version/s 1.8.1 [ 12321750 ]
        Affects Version/s 1.8.0 [ 12319550 ]
        Affects Version/s 1.9.0 [ 12319551 ]
        Hide
        Filip Maj added a comment -

        Moving to 2.0.

        Show
        Filip Maj added a comment - Moving to 2.0.
        Hide
        Simon MacDonald added a comment -

        Not really, when I was doing my prototype of moving everything into the SplashScreen class I had set the onload to be true. However, the problem is one of timing. First your activity that extends DroidGap gets loaded which then creates a CordovaWebView which in turn loads the PluginManager. The PM is responsible for reading plugins.xml and loading the ones that have onload set to true right away.

        All of that is enough of a delay for the user to notice that the SplashScreen is not display as soon as the Activity is created. Also, the DroidGap class was sending a message to the plugin to show the SplashScreen before the class was instantiated so the message was lost. So, it looks like we'd need to queue up that message until someone is able to act upon it which is a non-trivial change.

        Plus, does it even make sense to move all this out to a Plugin considering that someone who'd be embedding a CordovaWebView in their own app probably has their own plans for a SplashScreen.

        Anyway, I will circle back to this once the great Camera cleanup is done.

        Show
        Simon MacDonald added a comment - Not really, when I was doing my prototype of moving everything into the SplashScreen class I had set the onload to be true. However, the problem is one of timing. First your activity that extends DroidGap gets loaded which then creates a CordovaWebView which in turn loads the PluginManager. The PM is responsible for reading plugins.xml and loading the ones that have onload set to true right away. All of that is enough of a delay for the user to notice that the SplashScreen is not display as soon as the Activity is created. Also, the DroidGap class was sending a message to the plugin to show the SplashScreen before the class was instantiated so the message was lost. So, it looks like we'd need to queue up that message until someone is able to act upon it which is a non-trivial change. Plus, does it even make sense to move all this out to a Plugin considering that someone who'd be embedding a CordovaWebView in their own app probably has their own plans for a SplashScreen. Anyway, I will circle back to this once the great Camera cleanup is done.
        Hide
        Filip Maj added a comment -

        @Simon what about using onload="true" attribute in plugins.xml for it? Does that help?

        Show
        Filip Maj added a comment - @Simon what about using onload="true" attribute in plugins.xml for it? Does that help?
        Hide
        Simon MacDonald added a comment -

        @Joe: I will not be able to get to it for 1.9.0. I actually have concerns pushing this out to a plugin due to timing issues. When the splash screen needs to be shown it's plugin is not loaded yet so you will have a blank screen when you are expecting a splash screen. I'll look at it again in 2.0 but it is not as easy as moving all the code into the SplashScreen class.

        Show
        Simon MacDonald added a comment - @Joe: I will not be able to get to it for 1.9.0. I actually have concerns pushing this out to a plugin due to timing issues. When the splash screen needs to be shown it's plugin is not loaded yet so you will have a blank screen when you are expecting a splash screen. I'll look at it again in 2.0 but it is not as easy as moving all the code into the SplashScreen class.
        Hide
        Aurelien MERCIER added a comment -

        In iphone it's already a plugin: CDVSplashScreen. In Android, it's called a plugin but doesn't behave as one. This has caused some issues in our application to get rid of the splashscreen with the hide method because it was too tight with DroidGap and sometimes the hide method was not applied on the right view...
        Plus in terms of maintenance, that's definitely better for you to refactor the code outside DroidGap...

        Show
        Aurelien MERCIER added a comment - In iphone it's already a plugin: CDVSplashScreen. In Android, it's called a plugin but doesn't behave as one. This has caused some issues in our application to get rid of the splashscreen with the hide method because it was too tight with DroidGap and sometimes the hide method was not applied on the right view... Plus in terms of maintenance, that's definitely better for you to refactor the code outside DroidGap...
        Hide
        Joe Bowser added a comment -

        @Aurelien: How would this bring better harmony with the iOS version?

        @Simon: Can we get the fix in time for 1.9, or does bumping it up to 2.0 make sense?

        Show
        Joe Bowser added a comment - @Aurelien: How would this bring better harmony with the iOS version? @Simon: Can we get the fix in time for 1.9, or does bumping it up to 2.0 make sense?
        Hide
        Aurelien MERCIER added a comment -

        Are you kidding???, I think a lot of people care about this issue and I think that's an excellent idea to refactor it outside DroidGap. Plus it will bring a better harmony with the iOS version, essential when you develop for both devices.

        Show
        Aurelien MERCIER added a comment - Are you kidding???, I think a lot of people care about this issue and I think that's an excellent idea to refactor it outside DroidGap. Plus it will bring a better harmony with the iOS version, essential when you develop for both devices.
        Hide
        Simon MacDonald added a comment -

        I was hoping someone else would comment on this but it looks like no one cares Might as well close it for now.

        Show
        Simon MacDonald added a comment - I was hoping someone else would comment on this but it looks like no one cares Might as well close it for now.
        Hide
        Joe Bowser added a comment -

        Can we close this as Won't Fix?

        Show
        Joe Bowser added a comment - Can we close this as Won't Fix?
        Hide
        Joe Bowser added a comment -

        It does mean that the Splashscreen plugin MUST be used with DroidGap, which is a pretty nasty dependency. Are we OK with this?

        Show
        Joe Bowser added a comment - It does mean that the Splashscreen plugin MUST be used with DroidGap, which is a pretty nasty dependency. Are we OK with this?
        Hide
        Simon MacDonald added a comment -

        As I prototyped this today I came to the conclusion that we may not want to do this. If we move all the code out of DroidGap and into the SplashScreen plugin we have a timing issue as the SplashScreen plugin may not be loaded when the main activity wants to show the splash screen even if the plugin has the onload attribute set to true.

        Additionally, in order to do this correctly as a plugin we will need to add more methods to CordovaInterface which is something Joe is already finding onerous when you want to implement that interface. For folks that want to embed a CordovaWebView in their own activity they are probably taking care of the splash screen themselves. So, keeping this in DroidGap for folks who don't want to worry about embedding the web view kinda makes sense.

        Show
        Simon MacDonald added a comment - As I prototyped this today I came to the conclusion that we may not want to do this. If we move all the code out of DroidGap and into the SplashScreen plugin we have a timing issue as the SplashScreen plugin may not be loaded when the main activity wants to show the splash screen even if the plugin has the onload attribute set to true. Additionally, in order to do this correctly as a plugin we will need to add more methods to CordovaInterface which is something Joe is already finding onerous when you want to implement that interface. For folks that want to embed a CordovaWebView in their own activity they are probably taking care of the splash screen themselves. So, keeping this in DroidGap for folks who don't want to worry about embedding the web view kinda makes sense.
        Joe Bowser made changes -
        Parent CB-190 [ 12539025 ]
        Issue Type Sub-task [ 7 ] Improvement [ 4 ]
        Joe Bowser made changes -
        Summary Refactor the Screenshot API so that the code is outside of DroidGap Refactor the Splashscreen API so that the code is outside of DroidGap
        Hide
        Joe Bowser added a comment -

        This should be splashscreen, not screenshot

        Show
        Joe Bowser added a comment - This should be splashscreen, not screenshot
        Joe Bowser made changes -
        Fix Version/s 1.9.0 [ 12319551 ]
        Fix Version/s 1.8.0 [ 12319550 ]
        Hide
        Joe Bowser added a comment -

        You have until next month, since we agreed to move CB-190 to the next release.

        Show
        Joe Bowser added a comment - You have until next month, since we agreed to move CB-190 to the next release.
        Filip Maj made changes -
        Field Original Value New Value
        Component/s Android [ 12316401 ]
        Hide
        Simon MacDonald added a comment -

        kk, I should be able to do this for 1.8 without any issues.

        Show
        Simon MacDonald added a comment - kk, I should be able to do this for 1.8 without any issues.
        Joe Bowser created issue -

          People

          • Assignee:
            Simon MacDonald
            Reporter:
            Joe Bowser
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development