ZeligsoftDev / CX4CBDDS

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

Improvements for Pathmap Conflicts #409

Closed emammoser closed 2 years ago

emammoser commented 2 years ago

Issue and tracking information

We would like the following added to the pathmap conflict dialog:

  1. One dialog for all pathmap conflicts
  2. In the dialog, a radio selection for each pathmap conflict that allows us to select which one we want to use
  3. A checkbox for each conflict that says something along the lines of "If there are no changes to the available pathmaps for this conflict, do not warn be about them again"
  4. A link in the bottom of the dialog to the Windows>Preferences>CX>Modeling>URI Mappings that users can click to open up the URI preferences section to see all active pathmaps.

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

ysroh commented 2 years ago

Is it possible to test this feature using the "papyrus_bugfix" branch?

This is the dialog for conflict pathmaps and users will be able to suppress future warnings as long as there are no new mappings introduced for the same pathmap. image

Users will be able to choose the mapping from the URI mappings preference page. image

Let me know if you have any questions or comments.

emammoser commented 2 years ago

A few things:

  1. I am all for testing tickets using alternative "bug fix" branches before pushing in larger tickets.
  2. I would like to see what happens when we see multiple pathmap conflicts. One of the things we want to see is a single dialog that lists all pathmap conflicts when they are recognized. That way the user doesn't have to keep making selections and then new dialogs keep popping up for other conflicts.
  3. In your example above, it seems like the only solution is to accept the mapping change that you are suggesting. What if in the dialog above I didn't want "test2" and I still wanted the first one? I know I can go to the URI Mappings preference, but it would be preferred to also select which pathmap I want in the warning dialog itself.
ysroh commented 2 years ago

Thanks for the input. Please see the comments in-line.

A few things:

  1. I am all for testing tickets using alternative "bug fix" branches before pushing in larger tickets. ys] ok thanks
  2. I would like to see what happens when we see multiple pathmap conflicts. One of the things we want to see is a single dialog that lists all pathmap conflicts when they are recognized. That way the user doesn't have to keep making selections and then new dialogs keep popping up for other conflicts. ys] See the answer to question 3.
  3. In your example above, it seems like the only solution is to accept the mapping change that you are suggesting. What if in the dialog above I didn't want "test2" and I still wanted the first one? I know I can go to the URI Mappings preference, but it would be preferred to also select which pathmap I want in the warning dialog itself.

ys] I will add the mapping selection combo box right in the warning dialog for the pathmap so users can select the mapping when they appear.

I will make this change and let you know when it is ready.

emammoser commented 2 years ago

One quick comment to. When I say "multiple pathmap conflicts", I mean for literally different pathmaps altogether. So, for example, we might have a model "ModelA.uml" with a pathmap "MYPATHMAP". Then we have a totally different model "ModelB.uml" with a different pathmap "DIFFERENTPATHMAP". If somehow I have conflicts for both of those, currently we get a dialog for the conflict for ModelA, and then a seperate dialog for the conflict for ModelB

ysroh commented 2 years ago

The changes are coming continuously from the Eclipse workspace change listener so we do not really know when to open the dialog for all warnings. You may get several events coming in separately or together containing multiple pathmap changes.

ysroh commented 2 years ago

We can do the one dialog IF the mapping is done once at the start up and triggered by the "remap" button any other time. This means that no automatic pathmap activation when you drag or import a new project into the workspace.

ysroh commented 2 years ago

I will experiment with something so that at least one dialog is open for all changes during the initial start-up, but might not guarantee this when multiple projects are brought into the workspace. I will let you know when I have something to show.

ysroh commented 2 years ago

I can make one dialog but the catch is that I cannot determine which pathmaps are newly added so I have to show all pathmaps that have multiple mappings in the dialog. Is this acceptable??

image

emammoser commented 2 years ago

That looks better. My next question would be what the pathmap the bottom check box is referring to? My guess is pathmap://PATHMAP222, but it might be nice if the table had a new column with checkboxes or true/false for whether or not to suppress warnings

ysroh commented 2 years ago

I am making changes so that only new conflicts that are not suppressed will show up on the dialog. There are quite a bit of code changes since we are now keeping multiple path mappings in order to support changes between multiple mappings. I will let you know when things are ready so you can test them.

ysroh commented 2 years ago

You will see the pathmaps that are not suppressed from the conflict dialog. image

You will also get appropriate warnings when pathmap mapping changes. e.g..) image

image

Please test the "papyrus_bugfix" branch, and provide me with more comments.

ysroh commented 2 years ago

Here is the pull request: https://github.com/ZeligsoftDev/CX4CBDDS/pull/418 A new column is added for the suppressed warning state.

emammoser commented 2 years ago

This is looking really good. I did hit 1 issues with my early testing. If I had 3 projects pulled in. Each with a model, each with a pathmap. Then I copied those projects outside of eclipse 3 times each and imported all 9 projects. The dialog would only pop up with conflicts for the first model. For example, the following folder structure where each project "ProjectA" is the same and "ProjectB" is the same and 'ProjectC" is the same other than different paths and project names.

ysroh commented 2 years ago

Thanks. I will take a look at the issue.

ysroh commented 2 years ago

I did not reproduce the same issue you described. I think it might depend on how those projects are imported. I imported all of them together via File->Import-> Import existing project and selected the root folder that contains all sub folders.

image

Can you please describe steps to import those copied projects into workspace?

emammoser commented 2 years ago

Import the Original non-numbered projects first. So ProjectA, ProjectB, and ProjectC. Then, import the other 9 after that.

ysroh commented 2 years ago

Ok. I was able to reproduce it.

ysroh commented 2 years ago

@j26151 Can you please check to see if the issue is fixed?

emammoser commented 2 years ago

Everything else looks good to me. One last thing however. When the current mapped pathmap is removed, it looks like CX just picks what dynamic pathmap it will use instead. Can we instead show the same dialog with CXs suggestion as the current value? This way the user could select something else. If I have ProjectA, test1/ProjectA-1, and test2/ProjectA-2 and I delete the original ProjectA which was also the model containing the chosen pathmap. CX choosen either test1/ProjectA-1 or test2/ProjectA-2. Instead, I would like it to show the dialog with one of those options chosen so that users can decide or just click OK if they are fine with CX's suggestion

ysroh commented 2 years ago

Absolutely, I will work on it and let you know.

ysroh commented 2 years ago

Done. I will merge the pull request after code review if no other issues are found.

ysroh commented 2 years ago

Fix merged onto "papyrus" branch. I will close this issue once verified.

emammoser commented 2 years ago

When I had ProjectB and its 3 other version as well as ProjectC and its 3 other version imported. Then I removed both projects that had the dynamic pathmap set I got the following 4 dialogs:

image

Ideally we would want one dialog that looks like one of the dialogs on the left of the image that has both removed pathmaps and their new suggestion. What are your thoughts here? I know this seems like a fairly silly case, but it is often that we might have git projects with 5 or so ICMs that are pathmapped. If we remove that single project, we could see up to 10 of these dialogs which would definitely not be ideal.

emammoser commented 2 years ago

I do understand the need for the right dialog now after actually reading it. Although we would want those consolidated as well. So maybe the above can become 2 dialogs instead of 4?

ysroh commented 2 years ago

Yes. I didn't think of that case. I will look into it and let you know.

ysroh commented 2 years ago

I've started the pull request for this fix. https://github.com/ZeligsoftDev/CX4CBDDS/pull/421 Can you verify that the fix works on your side?

emammoser commented 2 years ago

I was seeing all kinds of different behavior. The first time I tried to import a few more models with conflicting pathmaps, my eclipse hung for a few minutes. The second time, when I tried to remove some projects that were the current pathmaps, I still got all of the disparate dialogs . Also, I got this NPE:

image

ysroh commented 2 years ago

I've made some changes to make it more robust. You will notice some delays when you import or remove dynamic models to consolidate all possible event sequences. Can you please verify this?

ysroh commented 2 years ago

I think things are working much better now :)

emammoser commented 2 years ago

When I delete 2+ projects each containing a pathmapped model in one delete operation, I still receive multiple dialogs for the pathmaps: image

ysroh commented 2 years ago

The only way to consolidate dialogs is to keep waiting for the next event as long as a new event occurs within a preset delay period. What is the optimal delay to wait for the next event is unknown. If you wait longer then you are guaranteed to get one dialog but the Eclipse will freeze for a longer period. If you set a shorter delay then you might get multiple dialogs if an event process time takes longer than the delay period.

I added a new field in the URI mappings preference page to set the delay time to wait for a new event. The consolidated dialog will open once there are no more events detected within the delay period. Can you try different values to see what works best for you? I can set that delay value to be the default.

There will be no perfect value for this so please let me know the value that works best for the average use cases rather than the edge case.

emammoser commented 2 years ago

This looks great to me. I think we should set the value to 1 second for now. 1 second worked for me. If our users have issues with 1 second we can revisit it later. The last fix I would ask for is that the image "Open URI Mappings preference page for all pathmap list" changed to "Open URI Mappings preference page for full pathmap list"

ysroh commented 2 years ago

All done and merged.

emammoser commented 2 years ago

I have verified this on our end. Thank you

ysroh commented 2 years ago

Thanks