-
Notifications
You must be signed in to change notification settings - Fork 83
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
Service: changed super class of 'ElkServicePlugin' from 'AbstractUIPlugin' to just 'Plugin' #516
Conversation
68c3528
to
e249604
Compare
Doesn't jface require swt as well? |
import org.eclipse.elk.core.data.ILayoutMetaDataProvider; | ||
import org.eclipse.elk.core.data.LayoutMetaDataService; | ||
import org.eclipse.elk.core.service.util.MonitoredOperation; | ||
import org.eclipse.elk.core.util.Pair; | ||
import org.eclipse.jface.preference.IPreferenceStore; | ||
import org.eclipse.ui.IWorkbenchPart; | ||
import org.eclipse.ui.plugin.AbstractUIPlugin; |
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.
This should not be required anymore, right?
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.
Right. Came in while adding this javadoc
elk/plugins/org.eclipse.elk.core.service/src/org/eclipse/elk/core/service/ElkServicePlugin.java
Lines 93 to 97 in e249604
/** | |
* Return the preference store associated with this plug-in. | |
* | |
* Implementation is inspired by {@link AbstractUIPlugin#getPreferenceStore()}. | |
*/ |
I'll refine that.
In its entirety yes. However, here only the preference store interface and impl from JFace are referenced, and the impl is not (and should not be) instantiated in standalone usage. |
…ugin' to just 'Plugin' ... because loading class 'AbstractUIPlugin' seems to require SWT (esp. 'SWTError') on the classpath that may not be available in standalone scenarios; * copied 'IPreferenceStore' provision from 'AbstractUIPlugin' Signed-off-by: Christian Schneider <christian.schneider@typefox.io>
e249604
to
80929ff
Compare
@uruuru refinement is done. |
I'm fine with the changes. @le-cds? |
... because loading class 'AbstractUIPlugin' seems to require SWT (esp. 'SWTError') on the classpath that may not be available in standalone scenarios;
Copied 'IPreferenceStore' provision from 'AbstractUIPlugin'
To my surprise this change is also required in the standalone context,
otherwise failures (java.lang.Error) may happen at locations like this
elk/plugins/org.eclipse.elk.core/src/org/eclipse/elk/core/data/LayoutMetaDataService.java
Lines 63 to 65 in 524db61
This also looks like such a candidate:
elk/plugins/org.eclipse.elk.core.service/src/org/eclipse/elk/core/service/DiagramLayoutEngine.java
Lines 536 to 537 in 883c156