Skip to content
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

JBIDE-21460 NPE at Show In Web Browser for OpenShift server if connection is not connected #392

Closed
wants to merge 1 commit into from

Conversation

scabanovich
Copy link
Contributor

ServerExtendedProperties.getWelcomePageUrl() is mode to throw GetWelcomePageURLException
to show message in a warning dialog. Implementation may decide to log problem with stacktrace if
message in dialog is not sufficient.

public void run() {
MessageDialog.openWarning(
PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell(),
"No Welcome URL to Show", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the action is now called Show-in -> Web Browser, not welcome page. The welcome page terms are legacy and hidden. Can we change this message to be more generic? No URL found for current selection" or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

@scabanovich scabanovich force-pushed the jbide-21460 branch 2 times, most recently from 9f25b80 to 5ac3528 Compare January 27, 2016 06:05
@robstryker
Copy link
Member

This would break API and not be suitable for maintenance. The JIRA is targeted to maintenance. You'd need to prove that there are no other callers of this API in any other JBT code :(

@scabanovich
Copy link
Contributor Author

@robstryker , can it help if we make GetWelcomePageURLException extending RuntimeException instead of regular Exception. Then we do not need to declare it in 'throws' for the method so that API will not be changed. Javadoc may describe in full what and when is thrown. I know, this is kind of breaking through the back door, but it will work. Would you accept it?

@robstryker
Copy link
Member

I might, if you also do a grep on most jbt repos to check who else is using the API ;) Just so we know who else is affected BEFORE we commit it.

@scabanovich
Copy link
Contributor Author

The complete search over github (do can we trust them?)
https://github.com/search?utf8=%E2%9C%93&q=%22getWelcomePageUrl%22%2B%22jboss%22&type=Code&ref=searchresults

gives results:

jbosstools-server
org.jboss.ide.eclipse.as.core.server.internal.extendedproperties.ServerExtendedProperties
org.jboss.ide.eclipse.as.core.server.internal.extendedproperties.JBossExtendedProperties
org.jboss.ide.eclipse.as.ui.views.server.extensions.ShowInWelcomePageActionProvider
org.jboss.tools.as.itests.CreateServerCheckDefaultsTest
jbosstools-openshift
org.jboss.tools.openshift.express.internal.core.server.ExpressServerExtendedProperties
org.jboss.tools.openshift.core.server.OpenShiftServerExtendedProperties

Plus copies of these files at nickboldt/jbosstools-svn-mirror and nickboldt/temp-jbosstools-server.
That is all (if we still can trust them).

And, yes CreateServerCheckDefaultsTest now is affected and does not compile.
It can be fixed by extending RuntimeException in GetWelcomePageURLException.
Though, ServerExtendedProperties is in internal package, so that strictly speaking it is not API.
Or, exporting its package in manifest without restrictions makes it API? I am not sure about this.
So, @robstryker , please advise, if it is enough to fix CreateServerCheckDefaultsTest by catching exception, or extend new exception from RuntimeException?

@robstryker
Copy link
Member

Though, ServerExtendedProperties is in internal package, so that strictly speaking it is not API.

Unfortunately it's defacto-API as others are extending it all over :(

If you fix the test and also provide a matching patch to openshift, and make sure openshift now depends on the newest version of astools to ensure no mixed environments, it should be acceptable.

@robstryker
Copy link
Member

by 'newest' i mean the maintenance in which we expect the new exception to be present in.... not master. we don't want openshift maintenance to depend on astools master

@scabanovich
Copy link
Contributor Author

Can it be solved by setting in manifest of openshift plugins org.jboss.ide.eclipse.as.core;bundle-version="3.1.1" ?

…tion is not connected

ServerExtendedProperties.getWelcomePageUrl() is mode to throw GetWelcomePageURLException
to show message in a warning dialog. Implementation may decide to log problem with stacktrace if
message in dialog is not sufficient.
@scabanovich
Copy link
Contributor Author

@robstryker , I have updated PR, please look if I did it right.

@adietish
Copy link
Member

@robstryker can you please validate this so that we can merge the PR in openshift that depends on this?

@robstryker
Copy link
Member

pushed to master and maintenance

@robstryker robstryker closed this Feb 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants