ZeligsoftDev / CX4CBDDS

CX4CBDDS component modeling and generation tool
Apache License 2.0
8 stars 5 forks source link

Selected elements in compare view are covered by a white box #443

Closed eposse closed 3 months ago

eposse commented 1 year ago

Issue and tracking information

When comparing models or merging them if the user clicks on a diff on either the left- or right-hand panes of the Content Merge Viewer, the element in the diff gets covered by a white rectangle hiding the element's name and icon.

Developer's time Estimated effort to fix (hours):

Developer's Actual time spent on fix (hours)

Issue reporter to provide a detailed description of the issue in the space below

Steps to reproduce

  1. Add or clone the the git repo in the modelmergereproducer.zip from the cx-reproducers/bug/415 repository
  2. Import the project
  3. Open the "Git History" view
  4. Checkout the "conflicting_branch"
  5. Right-click on the "feature_branch" and select "Merge" A dialog appears saying that there is a conflict.
  6. Close the dialog and go to the "Git Staging" view
  7. Right-clock on the BasicPubSub.uml file under "Unstaged changes" and select "Merge Tool" After a little bit, model differences are computed and shown. The Content Viewer should show the following:

Screenshot from 2023-03-02 18-58-47

  1. Click on the icon next to "<Package> defn" to unfold

Screenshot from 2023-03-02 18-59-15

  1. Click on the diff for "<CXEnum> Color", you see the following:

Screenshot from 2023-03-02 19-03-39

Expected result

The selected element's name and icon should be visible.

Actual result

The selected element is covered by a white rectangle hiding its name and icon.

Environment

Ubuntu 20.04 CentOS 7

emammoser commented 1 year ago

NGC has approved this issue to be investigated

eposse commented 5 months ago

An update on this issue.

The problem is in EMF Compare. I have a working solution, but I'm having some trouble integrating it. I'm trying to integrating through an extension point, but EMF Compare is not picking up my extension, and this lead me to discover that it is also not picking up a critical extension from Papyrus Compare. So it looks like we are loosing some Papyrus Compare functionality. I'll see if I can resolve this easily or if we need a separate issue. If I can't resolve it easily, I can still do a patch, but in general it's preferable to override behaviours via extensions rather than patches, as doing patches incurs in technical debt that we would have to pay if we need to upgrade EMF Compare.

emammoser commented 5 months ago

Thank you for the update. I definitely agree that where possible, we should stick to extensions if possible. It is frustrating that EMF Compare is having this issue. Does it have the issue in the version of papyrus we are using in the v2 stream, or the v3 stream, or both?

eposse commented 5 months ago

Sorry, I forgot to answer. The issue happens on both streams. That's probably because they both use almost the same version of EMF Compare, where the error comes from. The old Papyrus comes with EMF Compare 3.3.11 and the newer one with 3.3.24, so I'm guessing they haven't gotten around to fix this.

As an update, as I mentioned earlier, I'm trying to override the EMF Compare code with an extension, and I mentioned that the Papyrus-Compare extension which I want to override wasn't being picked up. Well, I have more info on this. It turns out that it's not picked up when launching the comparison from the Git History view, but it is picking it up when launching the comparison from the Synchronize view (which only allows you to compare workspace items with some commit, and not comparisons between two commits). I'm still not sure how to override this extension yet, but at least now I know that there are two different scenarios, and it looks like the extension selection depends on the type of elements selected (two commits from the Git History View vs. a Papyrus model file in the workspace and a commit).

In hindsight it's not entirely surprising, as I remember that Philip, the author of Papyrus Compare, told me that support for EGit was limited.

So the matter seems to be about how extensions are registered and how so-called "ViewerDescriptors" (the thing that describes the Viewer to be overriden) are selected depending on the type of elements selected.

eposse commented 5 months ago

Depends on: Issue #501

eposse commented 5 months ago

I've marked this issue as depending on #501 because it turns out that issue is concerns how compare/merge viewers (and their extensions) are picked up to be shown when doing a comparison from the Git History view. Since the solution to this issue requires overriding a viewer (so that selected elements are displayed correctly, and Papyrus Compare functionality is active), we need to solve that one first.

I'll add more details on that issue.

eposse commented 4 months ago

I'm a bit overdue with an update on this issue.

As I mentioned earlier, in order to fix this one, I need to override some behaviour from EMF Compare. This lead me to discover Issue #501, at the heart of which is how certain extensions are picked by Eclipse Compare and EGit. In particular, in order to override the EMF Compare behaviour, I needed to implement an extension which was not being picked up by EGit and Eclipse Compare. This led me to a deep dive into the early stages of a compare operation, and in particular to try to understand the differences between a comparison launched from EGit and one from the Synchronize view. Both turn out to be different. Papyrus Compare works with the Synchronize view but not with EGit, so when using EGit, you get a plain EMF Compare operation. That might be enough for many purposes, in particular when comparing .uml files, but it will skip the Papyrus Compare features. But the bigger problem with respect to this bug, is that the extensions picked up in the two scenarios are different, and neither picks the extensions that would provide the fix. After a long time working on that, I think I have exhausted all alternatives to try to make Eclipse Compare, EGit and EMF compare, pick up the right extensions. It seems like the code that I need to override, cannot be overridden through an extension point. So at this point the only alternative left is to do a patch. That's what I will try to do now.

emammoser commented 4 months ago

Thank you for the update, we appreciate your perseverance on this issue. Do we think there will be any chance that such a patch (if you are able to create one) would be accepted by the community eventually?

eposse commented 4 months ago

I'm not sure. The EMF Compare team seems rather inactive. Their last commit was 3 months ago, and all the commits from last year were simple releng work. No new features or bug fixes. There only seem to be three developers left. The activity in the forum is also very low. I asked a question last June and it's still the third newest. So the project seems to be, if not dead, in a kind of comma. Also, my experience with the Xtext team left me quite discouraged. When I proposed them to provide the patches I made replacing log4j1 with log4j2, their response was lukewarm at best. They asked for more details, which I provided extensively, and then they went quiet. Because of that, I desisted from trying to give patches to other projects, because if one of the projects rejects it, then log4j2 will still pop-up in Papyrus. It seems like the open-source community around Eclipse is dwindling, or is less enthusiastic. I know that a couple of projects (including Papyrus) have left the Eclipse Simultaneous Release train, due to its new high frequency release timeline (releases every 3 months). This causes a lot of problems for many projects which have only a very small number of active developers. Aside from the technical challenges of keeping up with the release cadence, I think there is a bit of an attrition factor. Just recently, the lead (and only developer) of the OCL project decided to quit. So whether the community accepts patches might depend on a few things. If the patch is simple and straightforward and doesn't make substantial changes, meaning it requires very little effort from the relevant project's core team, then there is a chance. But more substantial changes may not be so easily accepted. For this particular bug, I may be able to make it simple enough. But the patches required to fix Issue #501 go deeper, so I wouldn't count on those being accepted.

eposse commented 3 months ago

An update.

The fix works on streams/v2 after introducing a feature patch for org.eclipse.emf.compare.rcp.ui. However, on streams/v3 it seems to have opened a dependencies can of worms in the build. It doesn't seem like an easy solution is possible. I'm investigating possible solutions.

eposse commented 3 months ago

Ok, I fixed the issues. It works now on both v2 and v3.

The links for the builds are these:

Let me know if you want to test them or if I can merge them.

emammoser commented 3 months ago

I think you can merge them. We won't have the ability to test them anytime soon.

eposse commented 3 months ago

Closed as agreed.