Uploaded image for project: 'Pivot'
  1. Pivot
  2. PIVOT-916

Replace DesktopApplicationContext.displayException calls with ApplicationContext.handleUncaughtException

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.4, 2.1, 2.0.5
    • Component/s: None
    • Labels:
      None

      Description

      There should be consistent way to handle uncaught exceptions in Pivot applications.

      However uncaught exceptions thrown in DesktopApplicationContext class (for example in application.startup ) are handled using private static method displayException, which displays dialog and its logic cannot be overriden.

      May be ApplicationContext.handleUncaughtException could be made protected and calls to DesktopApplicationContext.displayException could be replaced by ApplicationContext.handleUncaughtException. And possibly Application.Adapter could implement UncaughtExceptionHandler - so current DesktopApplicationContext.displayException logic could be moved to new Application.Adapter.uncaughtExceptionThrown method.

      This should enable to override uncaught exception handling globally in pivot applications.

      Motivation:

      We deploy Pivot app using Java Web Start. Users have by default disabled Java Console (and they are not familiar with it). We want to display custom dialog to handle uncaught exceptions displaying full stack trace and with possiblity to report exception to help desk.
      I think, in current implementation it is not possible to override handling of uncaught exceptions thrown during application init and other specific situations.

      1. 916.diff
        2 kB
        Roger Whitcomb
      2. app_exception.patch
        4 kB
        Roger Whitcomb
      3. ScriptApplication.java.patch
        1 kB
        Sandro Martini
      4. trunk_wtk.patch
        6 kB
        Karel Hübl
      5. wtk_test.zip
        7 kB
        Karel Hübl

        Activity

        Hide
        kaja78 Karel Hübl added a comment -

        Proposed patch.

        Show
        kaja78 Karel Hübl added a comment - Proposed patch.
        Hide
        smartini Sandro Martini added a comment -

        Hi Karel, thanks for the patch, but sorry I need some info:
        did you generated it from 2.0.x maintenance branch, or from trunk ?
        Could you post here a minimal sample to test the new behavior ?

        Last, changing API should be done for our 2.1 release, and not in the 2.0.x maintenance ... is it good the same for you, or do you need it for 2.0.4 (we need a vote in this case) ?

        Thanks a lot.

        Show
        smartini Sandro Martini added a comment - Hi Karel, thanks for the patch, but sorry I need some info: did you generated it from 2.0.x maintenance branch, or from trunk ? Could you post here a minimal sample to test the new behavior ? Last, changing API should be done for our 2.1 release, and not in the 2.0.x maintenance ... is it good the same for you, or do you need it for 2.0.4 (we need a vote in this case) ? Thanks a lot.
        Hide
        kaja78 Karel Hübl added a comment - - edited

        Hi Sandro,

        The patch is for trunk revision 1513401. I am attaching archived eclipse project with simple unit test and testing application.

        The unit test is only testing exception handling for applicatin startup. I was not able implement unit test for application shutdown (there is System.exit(0) call in DesktopApplicationContext.HostFrame.processWindowEvent()). It also seems to me, that suspend() and resume() methods are not invoked during minimizing and activating the Pivot desktop application. This is possible another bug...

        You can test the behaviour by executing the TestingApplication main method:
        1) Execute the app
        2) Click the Go button
        3) Close the app

        You should see these lines in system output with patched wtk trunk on classpath and no dialog showing after app startup:
        uncaughtExceptionThrown: Exception starting up app.
        uncaughtExceptionThrown: Exception performing action.
        uncaughtExceptionThrown: Exception shutting down app.

        You can also check, that minimizing and activating the testing app is not invoking suspend() and resume() methods of the testing app.

        Regarding the changing API. I would appreciate the fix in 2.0.4.

        Show
        kaja78 Karel Hübl added a comment - - edited Hi Sandro, The patch is for trunk revision 1513401. I am attaching archived eclipse project with simple unit test and testing application. The unit test is only testing exception handling for applicatin startup. I was not able implement unit test for application shutdown (there is System.exit(0) call in DesktopApplicationContext.HostFrame.processWindowEvent()). It also seems to me, that suspend() and resume() methods are not invoked during minimizing and activating the Pivot desktop application. This is possible another bug... You can test the behaviour by executing the TestingApplication main method: 1) Execute the app 2) Click the Go button 3) Close the app You should see these lines in system output with patched wtk trunk on classpath and no dialog showing after app startup: uncaughtExceptionThrown: Exception starting up app. uncaughtExceptionThrown: Exception performing action. uncaughtExceptionThrown: Exception shutting down app. You can also check, that minimizing and activating the testing app is not invoking suspend() and resume() methods of the testing app. Regarding the changing API. I would appreciate the fix in 2.0.4.
        Hide
        smartini Sandro Martini added a comment -

        Hi Karel, thank you very much for all the stuff, I'll try to look in next days, and integrate even your test classes. Let's update soon.

        On the API change I'll do it unless someone has objections (in next days) ... please tell me/write here.

        Show
        smartini Sandro Martini added a comment - Hi Karel, thank you very much for all the stuff, I'll try to look in next days, and integrate even your test classes. Let's update soon. On the API change I'll do it unless someone has objections (in next days) ... please tell me/write here.
        Hide
        smartini Sandro Martini added a comment -

        updated test application ApplicationHandlerTest, but note that to have display with a value, all applications extending Application.Adapter must add the following line at the beginning of their
        overridden startup method:
        super.startup(display, properties);

        or display will be null and so no Exception message could be displayed in case of errors.
        Could be a thing to write in related Javadoc comment ...

        This should be good the same, but tell me for objections in a few days, otherwise I'll mark this as resolved, and merge changes to trunk.

        Karel, at the moment I think it would be better to not add your tests (here in attach) to our tests subprojects, but really thanks for adding here, we'll keep them for the future.

        Show
        smartini Sandro Martini added a comment - updated test application ApplicationHandlerTest, but note that to have display with a value, all applications extending Application.Adapter must add the following line at the beginning of their overridden startup method: super.startup(display, properties); or display will be null and so no Exception message could be displayed in case of errors. Could be a thing to write in related Javadoc comment ... This should be good the same, but tell me for objections in a few days, otherwise I'll mark this as resolved, and merge changes to trunk. Karel, at the moment I think it would be better to not add your tests (here in attach) to our tests subprojects, but really thanks for adding here, we'll keep them for the future.
        Hide
        kaja78 Karel Hübl added a comment -

        Sandro, thanks for the fix.

        To prevent the problem with not intitalizing the disp variable, you can make the startUp method final and create new non final method for overriding. Also the disp variable could be protected. However JavaDoc should be sufficient...

        public static class Adapter implements Application, UncaughtExceptionHandler {
        protected Display disp;

        @Override
        public final void startup(Display display, Map<String, String> properties) throws Exception

        { this.disp = display; startupInternal(display, properties); }

        public void startupInternal(Display display, Map<String, String> properties) throws Exception {}
        ...

        Show
        kaja78 Karel Hübl added a comment - Sandro, thanks for the fix. To prevent the problem with not intitalizing the disp variable, you can make the startUp method final and create new non final method for overriding. Also the disp variable could be protected. However JavaDoc should be sufficient... public static class Adapter implements Application, UncaughtExceptionHandler { protected Display disp; @Override public final void startup(Display display, Map<String, String> properties) throws Exception { this.disp = display; startupInternal(display, properties); } public void startupInternal(Display display, Map<String, String> properties) throws Exception {} ...
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Hi Karel,
        Won't that break all existing applications that already override "startup" and subclass Application.Adapter?

        Show
        rwhitcomb Roger Whitcomb added a comment - Hi Karel, Won't that break all existing applications that already override "startup" and subclass Application.Adapter?
        Hide
        smartini Sandro Martini added a comment - - edited

        Hi all,
        Roger I think too that the final part of the change would break existing applications ...

        So I'm thinking to resolve this issue now only writing/clarifying JavaDoc with info on how to handle this in 2.0.x (and merge in trunk).
        Then add a related issue for trunk with breaking changes and add only there ...

        Work note: verify if update even ScriptApplication to extend Application.Adapter ... and do some test with it, like in the ApplicationHandlerTest . Just put a patch here but still not committed.

        What do you think ?

        Show
        smartini Sandro Martini added a comment - - edited Hi all, Roger I think too that the final part of the change would break existing applications ... So I'm thinking to resolve this issue now only writing/clarifying JavaDoc with info on how to handle this in 2.0.x (and merge in trunk). Then add a related issue for trunk with breaking changes and add only there ... Work note: verify if update even ScriptApplication to extend Application.Adapter ... and do some test with it, like in the ApplicationHandlerTest . Just put a patch here but still not committed. What do you think ?
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        I think we should set the display variable in the Application BEFORE calling the startup method instead of making "startup" do it, and requiring all implementations of Application.Adapter change their constructors to do "super(...)". There are only two places that actually call the "startup" method, which are in BrowserApplicationContext.java and DesktopApplicationContext.java. It would be a simple matter in these places, before calling "startup" to set the display in the application object (maybe need a new "setDisplay" method?), and then the Application.Adapter doesn't need to do it, nor do any user applications need to change.

        I will post a patch for this shortly....

        Show
        rwhitcomb Roger Whitcomb added a comment - I think we should set the display variable in the Application BEFORE calling the startup method instead of making "startup" do it, and requiring all implementations of Application.Adapter change their constructors to do "super(...)". There are only two places that actually call the "startup" method, which are in BrowserApplicationContext.java and DesktopApplicationContext.java. It would be a simple matter in these places, before calling "startup" to set the display in the application object (maybe need a new "setDisplay" method?), and then the Application.Adapter doesn't need to do it, nor do any user applications need to change. I will post a patch for this shortly....
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        But, of course, this would still require a change to user applications (to add the new "setDisplay" method). Hmmm. Let me think about this some more. I still hope there can be a better way than to change all user applications in order to make this work.

        Show
        rwhitcomb Roger Whitcomb added a comment - But, of course, this would still require a change to user applications (to add the new "setDisplay" method). Hmmm. Let me think about this some more. I still hope there can be a better way than to change all user applications in order to make this work.
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Another thought that (I don't think) would require any user application change is to add logic to "handleUncaughtException" in ApplicationContext to put up an Alert in addition to "exception.printStackTrace()" if there are no handlers registered. It could check if there are any active Display objects (using "displays.getLength() != 0") and then use the primary display to show an Alert. Basically this would mean moving the code that is currently in Application.Adapter in "displayException" down into ApplicationContext.handleUncaughtException.

        The other problem I see is that DesktopApplicationContext puts the application into the "applications" list BEFORE calling "startup", while BrowserApplicationContext puts the application into the list AFTER successfully calling "startup". Maybe this is the source of the problem to begin with (i.e., that startup exceptions never make it to the Application's "UncaughtExceptionHandler" because the application isn't in the list.... (or at least part of the problem).

        So, doing both these changes I think this would still allow applications to override the default handling of uncaught exceptions (which would be to both print the stack trace to the console, and show an Alert if there was any Display active), and have startup exceptions caught and handled by whatever mechanism is in place.

        What do you think?

        Show
        rwhitcomb Roger Whitcomb added a comment - Another thought that (I don't think) would require any user application change is to add logic to "handleUncaughtException" in ApplicationContext to put up an Alert in addition to "exception.printStackTrace()" if there are no handlers registered. It could check if there are any active Display objects (using "displays.getLength() != 0") and then use the primary display to show an Alert. Basically this would mean moving the code that is currently in Application.Adapter in "displayException" down into ApplicationContext.handleUncaughtException. The other problem I see is that DesktopApplicationContext puts the application into the "applications" list BEFORE calling "startup", while BrowserApplicationContext puts the application into the list AFTER successfully calling "startup". Maybe this is the source of the problem to begin with (i.e., that startup exceptions never make it to the Application's "UncaughtExceptionHandler" because the application isn't in the list.... (or at least part of the problem). So, doing both these changes I think this would still allow applications to override the default handling of uncaught exceptions (which would be to both print the stack trace to the console, and show an Alert if there was any Display active), and have startup exceptions caught and handled by whatever mechanism is in place. What do you think?
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        This "app_exception.patch" builds on what is in 2.0.4 branch currently, to move the default Alert display into ApplicationContext and remove the "displayException" from Application.Adapter.

        I think this is better, and resolves Karel's issue without needing to change any user code. It also allows all startup exceptions to be handled by Pivot, either in the default handler, or in a user's application.

        Show
        rwhitcomb Roger Whitcomb added a comment - This "app_exception.patch" builds on what is in 2.0.4 branch currently, to move the default Alert display into ApplicationContext and remove the "displayException" from Application.Adapter. I think this is better, and resolves Karel's issue without needing to change any user code. It also allows all startup exceptions to be handled by Pivot, either in the default handler, or in a user's application.
        Hide
        kaja78 Karel Hübl added a comment -

        Thanks Roger, I agree this is better solution.

        Show
        kaja78 Karel Hübl added a comment - Thanks Roger, I agree this is better solution.
        Hide
        smartini Sandro Martini added a comment -

        Just committed the patch from Roger.
        If no objections, in a few days I'll mark this as resolved ... thanks to all for the help.

        Now I'll check if ScriptApplication need some changes.
        Finally I'll do the merge into trunk.

        Show
        smartini Sandro Martini added a comment - Just committed the patch from Roger. If no objections, in a few days I'll mark this as resolved ... thanks to all for the help. Now I'll check if ScriptApplication need some changes. Finally I'll do the merge into trunk.
        Hide
        smartini Sandro Martini added a comment -

        Resolved. Just merged in trunk.

        Show
        smartini Sandro Martini added a comment - Resolved. Just merged in trunk.
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        I'd like to make another small enhancement here: namely move the default code that is now inside "uncaughtExceptionThrown" to its own static method so that any Application that implements the new interface could itself default to calling the default, default method if desired.

        Show
        rwhitcomb Roger Whitcomb added a comment - I'd like to make another small enhancement here: namely move the default code that is now inside "uncaughtExceptionThrown" to its own static method so that any Application that implements the new interface could itself default to calling the default, default method if desired.
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Proposed change: just move the code from inside "handleUncaughtException" to its own method "defaultUncaughtExceptionHandler" so it can be called from user application if desired.

        My implementation looks like this:
        @Override
        public void uncaughtExceptionThrown(Exception exception) {
        try

        { MessageDialog.showException(mainWindow, exception); }

        catch (Exception ourProblem)

        { // Ignore this exception (essentially spurious problem in our code) // and display the original one via the default handler ApplicationContext.defaultUncaughtExceptionHandler(exception); }

        }

        Show
        rwhitcomb Roger Whitcomb added a comment - Proposed change: just move the code from inside "handleUncaughtException" to its own method "defaultUncaughtExceptionHandler" so it can be called from user application if desired. My implementation looks like this: @Override public void uncaughtExceptionThrown(Exception exception) { try { MessageDialog.showException(mainWindow, exception); } catch (Exception ourProblem) { // Ignore this exception (essentially spurious problem in our code) // and display the original one via the default handler ApplicationContext.defaultUncaughtExceptionHandler(exception); } }
        Hide
        smartini Sandro Martini added a comment -

        to me looks good

        Show
        smartini Sandro Martini added a comment - to me looks good
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Checked in the change to "trunk":
        Sending wtk\src\org\apache\pivot\wtk\ApplicationContext.java
        Transmitting file data .
        Committed revision 1624381.

        Merged into "branches/2.0.x":
        Sending .
        Sending wtk\src\org\apache\pivot\wtk\ApplicationContext.java
        Transmitting file data .
        Committed revision 1624382.

        Show
        rwhitcomb Roger Whitcomb added a comment - Checked in the change to "trunk": Sending wtk\src\org\apache\pivot\wtk\ApplicationContext.java Transmitting file data . Committed revision 1624381. Merged into "branches/2.0.x": Sending . Sending wtk\src\org\apache\pivot\wtk\ApplicationContext.java Transmitting file data . Committed revision 1624382.
        Hide
        smartini Sandro Martini added a comment -

        Hi Roger, is it safe to mark as Resolved now ?

        Show
        smartini Sandro Martini added a comment - Hi Roger, is it safe to mark as Resolved now ?
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Implemented and tested in our application. All is good.

        Show
        rwhitcomb Roger Whitcomb added a comment - Implemented and tested in our application. All is good.

          People

          • Assignee:
            rwhitcomb Roger Whitcomb
            Reporter:
            kaja78 Karel Hübl
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development