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

Toggle visibility of ProxyPort.type #2479

Merged
merged 4 commits into from
Jul 16, 2023
Merged

Toggle visibility of ProxyPort.type #2479

merged 4 commits into from
Jul 16, 2023

Conversation

amolenaar
Copy link
Member

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?

Type of proxy ports is not shown

Issue Number: Fixes #2424

What is the new behavior?

ProxyPort type is now shown if the "show-type" property is set.

@amolenaar amolenaar requested a review from danyeaw July 14, 2023 21:24
@github-actions github-actions bot added the python Pull requests that update Python code label Jul 14, 2023
@sz332
Copy link
Contributor

sz332 commented Jul 15, 2023

Hello, I checked out the feature and it works, but maybe we can make it even better.

Suggestions:

  • Remove the aggregate dropdown because it has no meaning for a port in my opinion @danyeaw
  • Instead of show type checkbox have a dropdown where user can select: Display name, Display type, Display name and type
  • If a Type is selected in the dropdown then the the second dropdown should automatically set it's value to Display name and type

@amolenaar
Copy link
Member Author

amolenaar commented Jul 15, 2023

Remove the aggregate dropdown because it has no meaning for a port in my opinion @danyeaw

Fair point, it's inherited from Property, the base type of ports.

Instead of show type checkbox have a dropdown where user can select: Display name, Display type, Display name and type

Not sure about this. Now the behavior is consistent with the Show ... switches for classes and blocks.
In what situations do you only want to show the type and not the name?

Another question: do we also want to show/hide the type for Properties in SysML?
At the moment I coded it as a special thingy for ProxyPorts only.

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.

This looks great! I agree that we don't want 3 options. If you don't want to name your port, then well don't name your port 😄

Just one comment, I think there should be a space before the colon when we separate names from types.

gaphor/SysML/blocks/proxyport.py Show resolved Hide resolved
@danyeaw danyeaw added feature A new feature and removed python Pull requests that update Python code labels Jul 15, 2023
@amolenaar amolenaar requested a review from danyeaw July 15, 2023 21:01
@github-actions github-actions bot added the python Pull requests that update Python code label Jul 15, 2023
Do not show Aggregation for Ports. Do not show Show Type toggle for
Properties.
@danyeaw danyeaw merged commit 1a1b38f into main Jul 16, 2023
20 checks passed
@danyeaw danyeaw deleted the port-type branch July 16, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port type in label
3 participants