-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add support for switching to recommended perspective #1529
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@@ -35,6 +35,8 @@ | |||
class="com.google.cloud.tools.eclipse.util.service.ServiceContextFactory:com.google.cloud.tools.eclipse.appengine.newproject.flex.AppEngineFlexProjectWizard" | |||
icon="platform:/plugin/com.google.cloud.tools.eclipse.appengine.ui/icons/gae-16x16.png" | |||
project="true" | |||
finalPerspective="org.eclipse.jst.j2ee.J2EEPerspective" | |||
preferredPerspectives="org.eclipse.jst.j2ee.J2EEPerspective,org.eclipse.wst.web.ui.webDevPerspective" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will adding the Java perspective at the end help users installing our plugin on non-J2EE Eclipse versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pull in WTP and the J2EE support.
I did wonder about including plain Java perspectives, especially since my preferred perspective is the Java Browsing perspective. But I figured most people would say "No" to the switch-perspective in that case.
|
||
@Override | ||
public void setInitializationData(IConfigurationElement config, String propertyName, Object data) | ||
throws CoreException { | ||
if (data == null || !(data instanceof String)) { | ||
throw new CoreException(StatusUtil.error(getClass(), "Data must be a class name")); | ||
} | ||
configElement = config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... indentation of all the lines in this method seems off.
|
||
public class MavenArchetypeProjectWizard extends Wizard implements INewWizard { | ||
public class MavenArchetypeProjectWizard extends Wizard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: if we change plugin.xml
to use com.google.cloud.tools.eclipse.util.service.ServiceContextFactory
like in the non-Maven plugin.xml
, configElement
could be injected, we won't need setInitializationData()
, and this class will not implement IExecutableExtension
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Will do.
// open most important file created by wizard in editor | ||
IFile file = runnable.getMostImportant(); | ||
BasicNewProjectResourceWizard.selectAndReveal(file, workbench.getActiveWorkbenchWindow()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use org.eclipse.ui.wizards.newresource.BasicNewResourceWizard.selectAndReveal(IResource)
instead.
Or at least change the class to BasicNewResourceWizard
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BasicNewResourceWizard.selectAndReveal(IResource)
is an instance method. Do you see any real benefit of inheriting from BasicNewResourceWizard
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's working.
* Expected to be created via the {@link ServiceContextFactory}. | ||
*/ | ||
public class MavenArchetypeProjectWizard extends Wizard | ||
implements INewWizard { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this in a single line?
With these test failures, likely because of perspective-switch dialogs, I'm wondering if this change is really a good idea :-S |
devappserver returning 503?
|
I can only think that the devappserver isn't quite ready. The test waits until it sees:
I'll change it to look for
I don't understand what this PR changes to cause this. |
Now dumps the contents of the devappserver console. Uh what's this
|
Codecov Report
@@ Coverage Diff @@
## master #1529 +/- ##
============================================
+ Coverage 69.5% 71.06% +1.56%
- Complexity 2029 2460 +431
============================================
Files 324 329 +5
Lines 12768 14579 +1811
Branches 1240 1669 +429
============================================
+ Hits 8874 10361 +1487
- Misses 3406 3661 +255
- Partials 488 557 +69
Continue to review full report at Codecov.
|
78bfa5f
to
31c721b
Compare
I output a list of files in the project directory and the deployed server module location. There are no class files… and I suspect that our project is out-of-sync with the file system:
|
So it doesn't seem like the project is being built before launch, which is bizarre. So I enabled some builder logging, which produces repeated output like:
Looking at So I suspect we have a bit of a race condition: if the auto-build job is running or scheduled as we enter And I recall now occasionally seeing the Maybe the solution is to never |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
Building directly in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway to rebase against current head? It's hard to see what's changed right now.
Will do — I curious to see if we were still having the same build errors. |
CLAs look good, thanks! |
I added some instrumentation to dump the current jobs, along with their state, rules, and I suspect the culprit is the
|
Seems to happen more consistently on Neon and Oxygen. |
Sounds like bug 486083. |
1dc13c9
to
56d4538
Compare
…ect deletion timesout
Bring in patched versions of org.eclipse.core.resources to check if https://git.eclipse.org/c/101379 fixes bug 519776 and fixes our test errors.
Makes the J2EE perspective the recommended perspective (the final perspective) for our New Project wizards, but marks the J2EE and Web perspectives as being acceptable (the preferred perspectives).
No explicit test for this, but I had to change the integration tests to switch to the J2EE perspective as the would-you-like-to-switch-perspective dialog interfered.
Fixes #1455