Skip to content
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

Proxy Port Improvements #2842

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

marek-piirikivi
Copy link
Contributor

I added additional check to see if there exists an item on the diagram that may own the proxy port.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bug fix
  • Feature
  • Chore (refactoring, formatting, local variables, other cleanup)
  • Documentation content changes

What is the current behavior?

Issue Number: #2612

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Additionally fixed proxy port drag icon. Automatically generated icon name is "proxy-port".

gaphor/SysML/drop.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added python Pull requests that update Python code documentation labels Nov 17, 2023
@danyeaw
Copy link
Member

danyeaw commented Nov 19, 2023

Hi @marek-piirikivi, I tried to test this by doing the following:

  1. Create a block called Car
  2. Create a 2nd block named Wheel
  3. Add a composition relationship from the Car to the Wheel
  4. Add the Wheel Part Property to the diagram
  5. Add a proxy port to the Wheel Part Property
  6. Add the Wheel Part Property to the diagram again
  7. Try to drag the proxy port on to the 2nd Wheel Part Property

However, this doesn't seem to work for me. Am I testing this correctly or was it fixing some other use case?

@marek-piirikivi
Copy link
Contributor Author

marek-piirikivi commented Nov 20, 2023

@danyeaw Could you tell me what symptoms there were? What happened? What you expected to happen?

I'll check this out myself (did not have the time yet), but the fix is about reusing already existing elements from the model browser. It does not fix the problem that we have before about the problem with placing ports when there are multiple items that can hold the port on the diagram. It is known older problem that when there are multiple items which the port can belong to, that only the first found item gets the port - is this what you experienced? I can solve this within this merge request as well, but I see this as a separate problem. This change merely "adds block's property into the list that even can own a port that's dragged from the model btowser". 🙂

@danyeaw
Copy link
Member

danyeaw commented Nov 21, 2023

@marek-piirikivi Sorry for the confusion, it sounds like I was testing out a different use case, definitely don't expand this PR to fix that other thing. I reread the issue you linked to. With this PR, if I try this:

  1. Create a block
  2. Use the corresponding part on an ibd
  3. Add a port to the part
  4. Go to the bdd, and double click the port in the model browser

The port is not adding to the part again instead of the block.

@marek-piirikivi
Copy link
Contributor Author

@danyeaw Are you sure you are looking at the correct changes? Here is a video - I tried to reproduce the issues, but maybe I am not understanding the use-case properly 🤔

gaphor-2023-11-21_06.46.29.mp4

@marek-piirikivi
Copy link
Contributor Author

definitely don't expand this PR to fix that other thing

😈 I did that. It was bugging me so much... even if I do not see the use-case for having the two items representing the same block on the same diagram.

Removing unused elements with proxy port is still broken. When I delete it from any of the items referencing it, it removes it from everywhere.

@marek-piirikivi
Copy link
Contributor Author

... I should add tests as well... 😅

@marek-piirikivi
Copy link
Contributor Author

@danyeaw @amolenaar

I made some updates:

  1. You can now drag existing ports onto all the items that may own the ports - either the owning block or a property typed by owning block. The closest item will get the block.
  2. Deleting a port item will not delete the port from the model if there are any port items presenting the port. I tested manually, seems to be working with both "Remove unused elements" enabled and disabled as expected. Now, making a mistake while dragging a duplicate port, you can remove it without having to worry about it being removed from the model.
  3. I added tests for the first one. Second one had already test coverage and it did not brake.

From my point of view, I tried to brake the functionality, but couldn't - it behaves as I expect it to behave. Please review. 🙂

Here is a small demo as well (ignore jerky mouse movement - I am not very good with mouse, shaky hands)
https://github.com/gaphor/gaphor/assets/45357878/a77847ab-1f4a-4c91-acca-0763dcd9d6bf

@danyeaw danyeaw changed the title Allow adding proxy port from the model browser to the instance of the owning block and fix proxy port drag icon Proxy Port Improvements Nov 22, 2023
Copy link
Member

@danyeaw danyeaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danyeaw danyeaw removed the python Pull requests that update Python code label Nov 22, 2023
@danyeaw danyeaw merged commit a397ad3 into gaphor:main Nov 22, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants