New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refuse to run startup actions if earlier errors #2799

Merged
merged 10 commits into from Jan 3, 2017

Conversation

Projects
None yet
2 participants
@rhwood
Contributor

rhwood commented Jan 1, 2017

apps.StartupActionsManager no longer runs the startup actions if there were (some) prior errors while starting a JMRI application. In this PR this means that if there is an error initializing a connection during application start, the startup actions will not run afterwards.

startup-refusal

This PR:

  • Adds API to the jmri.spi.PreferencesManager to allow one PreferencesManager to be aware if a PreferencesManager it depends on fails to initialize correctly (previously a PreferencesManager was always assumed to have initialized correctly). Currently only the ConnectionConfigManager and StartupActionsManager completely utilize this API.
  • Retains exceptions thrown by apps.startup.StartupModel objects, so they can be displayed later. Currently these are only shown in the dialog when there is an error, and some actions still don't retain all exceptions.

rhwood added some commits Dec 31, 2016

Minor cleanup
Spelling, Overrides, and unneeded method call
API for managers initializing with errors
Add API for PreferencesManagers to initialize with errors, so they are
in a state that can be checked so follow on managers can refuse to
initialize as well.
Use new PreferencesManager API to display errors
Also take steps to ensure a manager is not initialized more than once
if initialized with errors.
Errors are displayed in the order they occur.
Use new API to display errors with connections.
The real error is *always* shown in a separate dialog because the
XmlAdapters use a different mechanism for displaying those errors and
will not propagate them up the call stack.
Also provide constructor to create an InitializationException purely
from a cause.
Allow the StartupManager to not perform actions when loading preferences
The StartupManager now does a two step process for loading its
preferences. It loads all preferences into memory (creating the list of
actions available in the preferences window) and then, if there were
not errors blocking it, performs those actions.

To make this work, the triggers for startup actions were moved from the
XmlAdapters that load the actions to the StartupModels themselves. Some
actions still need to throw nicer error messages, but this generally
works now.

@rhwood rhwood requested a review from bobjacobsen Jan 1, 2017

@rhwood

This comment has been minimized.

Show comment
Hide comment
@rhwood

rhwood Jan 1, 2017

Contributor

@bobjacobsen Can you please sanity check this?

Contributor

rhwood commented Jan 1, 2017

@bobjacobsen Can you please sanity check this?

Only attempt to perform valid actions
If a startup action is not valid, do not perform it.

@rhwood rhwood added the Enhancement label Jan 2, 2017

@rhwood rhwood added this to the 4.7.1 milestone Jan 2, 2017

@bobjacobsen

This comment has been minimized.

Show comment
Hide comment
@bobjacobsen

bobjacobsen Jan 3, 2017

Member

Looks good to me.

Some of the things I've tried:

  • Failed connections to LocoNet PR3, NCE, C/MRI before loading panels
  • Loading a panel that refers to a connection that hadn't been configured
  • Various errors in panels: Not validate, not valid names

When this showed the errors, everything looked as expected. Some of the errors aren't being handled well, hence didn't show up, but that's not a fault with this PR.

Was there something in particular you were concerned about? Happy to try anything that I've missed, just let me know.

Member

bobjacobsen commented Jan 3, 2017

Looks good to me.

Some of the things I've tried:

  • Failed connections to LocoNet PR3, NCE, C/MRI before loading panels
  • Loading a panel that refers to a connection that hadn't been configured
  • Various errors in panels: Not validate, not valid names

When this showed the errors, everything looked as expected. Some of the errors aren't being handled well, hence didn't show up, but that's not a fault with this PR.

Was there something in particular you were concerned about? Happy to try anything that I've missed, just let me know.

@bobjacobsen bobjacobsen removed their request for review Jan 3, 2017

@bobjacobsen

This comment has been minimized.

Show comment
Hide comment
@bobjacobsen

bobjacobsen Jan 3, 2017

Member

(Not sure what to do with Request to Review once done. I removed my name from the review block, upper right - is that the right thing?)

Member

bobjacobsen commented Jan 3, 2017

(Not sure what to do with Request to Review once done. I removed my name from the review block, upper right - is that the right thing?)

@rhwood rhwood requested review from bobjacobsen and removed request for bobjacobsen Jan 3, 2017

@rhwood

This comment has been minimized.

Show comment
Hide comment
@rhwood

rhwood Jan 3, 2017

Contributor

Looks good to me.

Thank you.

Was there something in particular you were concerned about? Happy to try anything that I've missed, just let me know.

I just wanted a second set of eyes on this to make sure I hadn't missed something critical or important.

(Not sure what to do with Request to Review once done. I removed my name from the review block, upper right - is that the right thing?)

I think that in the area where it allows merging and shows the CI status, it allows you to approve or request changes to the PR (it may not if you've removed yourself).

Contributor

rhwood commented Jan 3, 2017

Looks good to me.

Thank you.

Was there something in particular you were concerned about? Happy to try anything that I've missed, just let me know.

I just wanted a second set of eyes on this to make sure I hadn't missed something critical or important.

(Not sure what to do with Request to Review once done. I removed my name from the review block, upper right - is that the right thing?)

I think that in the area where it allows merging and shows the CI status, it allows you to approve or request changes to the PR (it may not if you've removed yourself).

@rhwood rhwood merged commit ce3b02c into JMRI:master Jan 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhwood added a commit to JMRI/website that referenced this pull request Jan 3, 2017

@rhwood rhwood deleted the rhwood:startup-with-errors branch Oct 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment