-
Notifications
You must be signed in to change notification settings - Fork 81
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
Adding SWT + JFace when creating a plugin that "makes contribution to the UI" #1306 #1472
Conversation
Test Results 283 files - 2 283 suites - 2 46m 53s ⏱️ - 4m 34s Results for commit 582b331. ± Comparison against base commit 29ff363. This pull request skips 1 test.
♻️ This comment has been updated with latest results. |
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.
Hi @cervantes-yadira, thank you for this PR. It already looks quite good. Nice work.
I just have a comment in the code to further refine this.
If you apply the changes, please amend your commit locally and force push the amended commit to the branch of this PR so that we get a nice and clean history.
...lipse.pde.ui/src/org/eclipse/pde/internal/ui/wizards/plugin/NewProjectCreationOperation.java
Outdated
Show resolved
Hide resolved
9399021
to
be4dc0b
Compare
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.
Thanks for the update, the code looks perfect now!
Unfortunately we are currently in the quiet-period before the December release and have to await the start of the next development cycle, which should start in one to two weeks, before this can be submitted.
In the meantime could you enhance the commit message by appending a reference to the fixed issue, i.e. by appending
to the message by the line:
Fixes https://github.com/eclipse-pde/eclipse.pde/issues/1306
and then again force push the updated commit to this PR's branch (and you fetch from this repository and rebase on top of the current master).
@laeubi, @vogella or @opcoach, since you discussed in the fixed issue, are you fine with this change as well?
be4dc0b
to
a81de6e
Compare
Change looks fine to me, thanks @cervantes-yadira I think this solution is fine, additional checkboxes in the UI might IMHO not be necessary. |
Dependencies aren't added for plug-ins that generate an activator file or use a template. Fixes eclipse-pde#1306
This pull request changes some projects for the first time in this development cycle.
An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patch
Further information are available in Common Build Issues - Missing version increments. |
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.
Thanks for the update.
According to the original issue, using JFace and SWT is almost necessary for plugins that contribute to the UI. Since these dependencies can be easily removed and are already dependencies of Eclipse, we decided to add them by default.
Fixes #1306