Resolve circular dependency between org.eclipse.mylyn.monitor.* and org.eclipse.mylyn.context.* (#16) #17
Conversation
org.eclipse.mylyn.context.* #16
|
|
||
| private static CommonContextPlugin INSTANCE; | ||
|
|
||
| private ContextCallBack contextCallBack; |
There was a problem hiding this comment.
Please do not introduce this mutability, we will have a lot of troubles with it:
- who and then should set it
- how to make sure that it is already set before we need it
- and so on
org.eclipse.mylyn.commons.context/src/org/eclipse/mylyn/common/context/CommonContextPlugin.java
Outdated
Show resolved
Hide resolved
...clipse.mylyn.monitor.ui/src/org/eclipse/mylyn/monitor/ui/AbstractUserInteractionMonitor.java
Outdated
Show resolved
Hide resolved
...clipse.mylyn.monitor.ui/src/org/eclipse/mylyn/monitor/ui/AbstractUserInteractionMonitor.java
Outdated
Show resolved
Hide resolved
| } | ||
| MonitorUiPlugin.getDefault().notifyInteractionObserved(navigationEvent); | ||
| } | ||
| throw new UnsupportedOperationException("should be handeled in AbstractContextInteractionMonitor"); |
There was a problem hiding this comment.
This could be very surprising implementation change
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <?eclipse version="3.4"?> | ||
| <plugin> | ||
| <extension-point id="ContextCallBack" name="Context CallBack" schema="schema/ContextCallBack.exsd"/> |
There was a problem hiding this comment.
Let's reduce the extension point id to a single word, without camel case
There was a problem hiding this comment.
Should we change this to communicator?
There was a problem hiding this comment.
Whatever describes it better from you PoV.
I also would think twice if we can have many of them, in that case "communicators"
| </complexType> | ||
| </element> | ||
|
|
||
| <element name="ContextCallBack"> |
There was a problem hiding this comment.
Let's reduce the element name to a single word, without camel case
There was a problem hiding this comment.
Should we change this to communicator?
| Bundle-Version: 3.26.0.qualifier | ||
| Bundle-Activator: org.eclipse.mylyn.common.context.CommonContextPlugin | ||
| Bundle-Vendor: %Bundle-Vendor | ||
| Export-Package: org.eclipse.mylyn.common.context |
There was a problem hiding this comment.
I would mark this package "internal", until we have stable API
| } | ||
|
|
||
| } | ||
| return contextCallBack; |
There was a problem hiding this comment.
still can be null at this point, I suggest to provide a stub implimintation instead of null for such a case
| if (extensions.length > 0) { | ||
| IConfigurationElement[] elements = extensions[0].getConfigurationElements(); | ||
| for (IConfigurationElement element : elements) { | ||
| if (element.getName().compareTo(CONTEXT_CALL_BACK) == 0) { |
There was a problem hiding this comment.
Please consider Objects.equals we don't need deep and precise compareTo here
There was a problem hiding this comment.
will be in the next commit for the branch
org.eclipse.mylyn.commons.context/src/org/eclipse/mylyn/common/context/CommonContextPlugin.java
Outdated
Show resolved
Hide resolved
org.eclipse.mylyn.commons.context/src/org/eclipse/mylyn/common/context/CommonContextPlugin.java
Show resolved
Hide resolved
for lazy initializion
|
Guys, sorry for the slow response. I am giving training all day every day this week. No minute left. |
|
Why the request tell me that I have 1 change requested? |
|
We continue with this work in #31 |
No description provided.