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-18344 - revert previous change to AbstractJREComposite, use simple... #276

Merged
merged 1 commit into from Sep 22, 2014

Conversation

robstryker
Copy link
Member

...r solution of setting selected page

@@ -328,14 +328,12 @@ public IVMInstall getSelectedVM() {
public abstract List<IVMInstall> getValidJREs();

protected boolean showPreferencePage(String pageId) {
// return PreferencesUtil.createPreferenceDialogOn(getShell(), pageId, new String[] { pageId }, null).open() == Window.OK;
PreferenceManager manager = PlatformUI.getWorkbench().getPreferenceManager();
IPreferenceNode node = manager.find(pageId);
Copy link
Member

Choose a reason for hiding this comment

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

can node be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically yes but realistically no, since we only pass in 2 page id's: the one for exec-env or installed jre's. Since these are part of base eclipse, it seems very unlikely they'd ever be null. I suppose some client could subclass and pass in an invalid node, but that's also very unlikely, since this composite's entire purpose is to link to exec-env / jre pages.

@maxandersen
Copy link
Member

Does this mean https://issues.jboss.org/browse/JBIDE-18083 needs to be reopeneded since seem this is a revert of it or ?

@robstryker
Copy link
Member Author

Does this mean https://issues.jboss.org/browse/JBIDE-18083 needs to be reopeneded since seem this is a revert of it or ?

I'm not sure on our policy of when to re-open old issues. I would say the original patch by daniel fixed the original issue, but had flaws. This is a new issue, caused by the old, and the fix is targeted to the new issue, not the old. They are related, but, without a clear policy of when to re-open the old issue, I'm not sure whether we should or not.

The old issue is not present, before or after this patch. Only the new issue is present. So I'd vote against re-opening an old issue. But if you can point to a policy that I'm misunderstanding, I'd be glad to re-open it.

@maxandersen
Copy link
Member

Not sure why the policy is unclear - if the old issue is still there it needs reopening.

it is not there then it does not need to open, that answers my question. I asked since it was not clear from the commit nor jira message wether that was the case.

@robstryker robstryker merged commit 725a5e2 into jbosstools:master Sep 22, 2014
@robstryker robstryker deleted the JBIDE-18344 branch November 6, 2014 10:18
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