#2714 Concretize function allocation #2715
Conversation
|
🚀 Build master-PR-2715-1 started! |
|
😞 Build master-PR-2715-1 failed! |
There was a problem hiding this comment.
can you create dedicated two github issue for this pull request as the two commits seems separated.
and more generally, can you add the id of the commits in the begining of the commit messages. (you might have to add
in your gitconfig to be able to add commit identifier at begining of the commit.)
and can you add junits on your fixed issues.
| @@ -1,5 +1,5 @@ | |||
| /******************************************************************************* | |||
There was a problem hiding this comment.
semantic browser view has 3 panes / 3 content providers that are inheriting from GroupedAdapterFactoryContentProvider.
Can you look at this class in which there is already a ResourceSetListenerImpl that refresh the view when the element is changed / or shall be updated. Maybe the refreshRequired shall be updated a bit to cover those new cases. we dont want to refresh to often the view, for instance when the session is opening.
There was a problem hiding this comment.
It looks like when my accelerator is invoked, there is no call to GroupedAdapterFactoryContentProvider.processRefresh()
The notifyChanged() methods seems to be working as expected, but the following call to processRefresh() is just missing.
Did I miss something when creating the allocation? Bypassed a wrapper meant to call the refresh afterward? I am a bit stumped on how to proceed from there.
There was a problem hiding this comment.
Hi @pdulth,
I had a look on the pointed code to help Etienne. I'm not sure to fully understand the implicated code. Indeed, the ResourceSetListener that you mentioned is only used by CapellaNavigatorContentProvider. The method getListener(), that instantiate the listener, is neved called for the 3 panes of the Semantic Browser view.
Do you expect that we add the listener of the ReferencingElementCP part used in Semantic Browser view, as it is done in CapellaNavigatorContentProvider ?
diff --git a/core/plugins/org.polarsys.capella.core.ui.semantic.browser.sirius/src/org/polarsys/capella/core/ui/semantic/browser/sirius/content/provider/SiriusSemanticBrowserContentProviderFactory.java b/core/plugins/org.polarsys.capella.core.ui.semantic.browser.sirius/src/org/polarsys/capella/core/ui/semantic/browser/sirius/content/provider/SiriusSemanticBrowserContentProviderFactory.java
index 8d13b25..8e765c2 100644
--- a/core/plugins/org.polarsys.capella.core.ui.semantic.browser.sirius/src/org/polarsys/capella/core/ui/semantic/browser/sirius/content/provider/SiriusSemanticBrowserContentProviderFactory.java
+++ b/core/plugins/org.polarsys.capella.core.ui.semantic.browser.sirius/src/org/polarsys/capella/core/ui/semantic/browser/sirius/content/provider/SiriusSemanticBrowserContentProviderFactory.java
@@ -17,8 +17,10 @@
import org.eclipse.emf.common.notify.Notification;
import org.eclipse.emf.ecore.EObject;
+import org.eclipse.emf.transaction.TransactionalEditingDomain;
import org.eclipse.jface.viewers.ITreeContentProvider;
import org.eclipse.sirius.viewpoint.DRepresentationDescriptor;
+import org.polarsys.capella.common.helpers.TransactionHelper;
import org.polarsys.capella.common.ui.toolkit.browser.content.provider.impl.CurrentElementCP;
import org.polarsys.capella.common.ui.toolkit.browser.content.provider.impl.ReferencedElementCP;
import org.polarsys.capella.common.ui.toolkit.browser.content.provider.impl.ReferencingElementCP;
@@ -148,6 +150,13 @@
public void notifyChanged(Notification notification) {
if (model.isListeningToPageSelectionEvents() && notification.hashCode() != lastNotification) {
lastNotification = notification.hashCode();
+ Object notifier = notification.getNotifier();
+ if (notifier instanceof EObject) {
+ TransactionalEditingDomain domain = TransactionHelper.getEditingDomain((EObject) notifier);
+ if (domain != null) {
+ domain.addResourceSetListener(getListener());
+ }
+ }
super.notifyChanged(notification);
}
}
With the above change, the behavior is OK. But I'm not sure if it is the right way to do it.
What do you think about that? Have you some additional advices?
| AbstractFunction motherFunction = (AbstractFunction) element; | ||
| Component component = resolveMotherFunctionAllocation(motherFunction); | ||
| if (component != null) { | ||
| createAlloction(component, motherFunction); |
| AbstractFunction motherFunction = (AbstractFunction) element; | ||
| Component component = resolveMotherFunctionAllocation(motherFunction); | ||
| if (component != null) { | ||
| createAlloction(component, motherFunction); |
There was a problem hiding this comment.
functionally, doesn't seems arcadia legit to allocate the mother function. only leafs can be allocated to components. to be discussed. Is there a DOREMI related ?
There was a problem hiding this comment.
Would it be acceptable if it was a config option (that was deactivated by default)?
There was a problem hiding this comment.
Hi,
It needs to be discussed.
Is there a DOREMI related describing the need to be discussed while obeo sync meeting?
There was a problem hiding this comment.
Hello,
No, this is not DOREMI related, it is a client request.
Would a PR with the functionality deactivated by default (and activable through the preferences) have better chance to be merged?
pdulth
force-pushed
the
concretize-function-allocation
branch
from
e8a2c12
to
2cbbdf6
Compare
|
🚀 Build master-PR-2715-2 started! |
|
Kudos, SonarCloud Quality Gate passed!
|
|
😟 Build master-PR-2715-2 is unstable! The product is available here. |
|
FYI build is ok if you rebase your branch from master |
… computed function allocation.
ebausson-obeo
force-pushed
the
concretize-function-allocation
branch
from
2cbbdf6
to
37ea328
Compare
|
🚀 Build master-PR-2715-4 started! |
eclipse-capella-bot
added
build-started
and removed
build-unstable
build-started
labels
|
😞 Build master-PR-2715-4 failed! |
ebausson-obeo
changed the title
ebausson-obeo
changed the title
No description provided.