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

Remove obsolete drawings shown in diagram layer #3056

Closed
christiankral opened this issue Jul 17, 2019 · 20 comments
Closed

Remove obsolete drawings shown in diagram layer #3056

christiankral opened this issue Jul 17, 2019 · 20 comments
Assignees
Labels
documentation Issue addresses the documentation enhancement New feature or enhancement
Milestone

Comments

@christiankral
Copy link
Contributor

christiankral commented Jul 17, 2019

In the MSL there exist many Modelica classes which show a drawing the diagram layer. These drawings are obsolete according to the guidelines of the User's Guide Modelica.UsersGuide.Conventions.Icons:

image

There are still plenty of Modelica classes in the MSL which do obey this rule. Diagram drawings to be used in the HTML documentation shall somehow be store somewhere as discussed in #2603 and #2695.

Note. In order to store some of my scripts and images until we come up with a decision I created https://gitlab.com/christiankral/ModelicaImages

@christiankral christiankral added enhancement New feature or enhancement documentation Issue addresses the documentation labels Jul 17, 2019
@christiankral christiankral added this to the MSL4.0.0 milestone Jul 17, 2019
@beutlich
Copy link
Member

What is your plan for MSL v4.0.0 actually? Do you want to check all classes manually and compare if the diagram layer duplicates the icon layer?

@christiankral
Copy link
Contributor Author

Is there a better way to do that? For sure there is a better way, but this is some programming effort, I suppose: Detect which diagram annotations contain line elements.

I guess we shall create PRs for each sub-package to make small step improvements.

@HansOlsson
Copy link
Contributor

I have two problems with understanding this guideline that was added 2017-04-18.
What does "Icons drawn in the Diagram layer" mean? To me "Icon" means icon of a component, but I assume it means something else here, and that should be made clearer.
Why was it added?

Is the intention to avoid something like Modelica.Blocks.Discrete.Sampler (etc) where both diagram and icon layer graphically represent the sampler? In that case I don't see the need to move that to the documentation layer.

Note that augmenting the diagram with drawing makes sense to me, e.g. Modelica.Blocks.Examples.PID_Controller has some illustration that works.
I assume the problem is when the diagram layer only contains drawings.

@HansOlsson
Copy link
Contributor

HansOlsson commented Aug 12, 2019

If the above represents the intent I would propose to rephrase it, perhaps as:

The diagram layer is intended to contain the graphical components, and if there are no graphical components it can be left empty. In particular do not make the diagram layer a copy of the icon layer. Graphical illustrations shall not be added in the diagram layer, but can be added in the HTML documentation.

Note: Assuming this is the intent the check would be for the following combination:

  • Non-empty Diagram-layer annotation.
  • Non-connector class
  • No graphical non-connector component

Note that in most cases the Diagram layer doesn't really duplicate the Icon-layer; it just looks similar and graphical illustrations are fine if augmenting the components as in PID_Controller.

@henrikt-ma
Copy link
Contributor

I was going to say that mentioning preferredView would also make sense in case of empty diagram layers, but to my great surprise, "icon" isn't a valid setting according to the preferred-view-annotation rule in 18.2 Annotations for Documentation

@HansOlsson
Copy link
Contributor

I was going to say that mentioning preferredView would also make sense in case of empty diagram layers, but to my great surprise, "icon" isn't a valid setting according to the preferred-view-annotation rule in 18.2 Annotations for Documentation…

I agree that lack is a bit odd.

However, I don't think that preferredView="icon" would be good for these cases; I think the text- or documentation-layer is more appropriate to understand how the model behaves. Showing the icon doesn't give any new information, and it is too subtle in my opinion.

@henrikt-ma
Copy link
Contributor

However, I don't think that preferredView="icon" would be good for these cases; I think the text- or documentation-layer is more appropriate to understand how the model behaves. Showing the icon doesn't give any new information, and it is too subtle in my opinion.

In my workflow, I'm almost always thrown off by documentation opening up by default, but I agree that opening the text view would be even better than the icon.

For classes that only serve to provide an icon, however, it would still be useful to set preferredView="icon".

@christiankral
Copy link
Contributor Author

If the above represents the intent I would propose to rephrase it, perhaps as:

The diagram layer is intended to contain the graphical components, and if there are no graphical components it can be left empty. In particular do not make the diagram layer a copy of the icon layer. Graphical illustrations shall not be added in the diagram layer, but can be added in the HTML documentation.

OK, it makes sense to me to to clarify the wording of the MSL User's Guide. Saying this, we posiibly leave the MSL models as they are, except there is a clear reason to remove some obsolete graphics from the diagram layer.

@tobolar
Copy link
Contributor

tobolar commented Nov 21, 2019

I also agree to #3056 (comment) from @HansOlsson .
What in that case would be with e.g. Modelica.Blocks.Math.MatrixGain? It seems to me to break the "do not make the diagram layer a copy of the icon layer" rule.

@dietmarw
Copy link
Member

Just noticed that the majority of math blocks use duplicate diagram layers.

@christiankral
Copy link
Contributor Author

I also agree to #3056 (comment) from @HansOlsson .
What in that case would be with e.g. Modelica.Blocks.Math.MatrixGain? It seems to me to break the "do not make the diagram layer a copy of the icon layer" rule.

Yes, correct, but it is basically just an alternative icon and it is indeed not required to better understand the model.

@christiankral
Copy link
Contributor Author

I wonder if we have a compromise on how we proceed

  1. Keep augmented drawings of the Diagram layer
  2. Remove duplicates of the the Icon layer
  3. Add preferredView="text" for regular models and preferredView="icon" for icons

@HansOlsson
Copy link
Contributor

I wonder if we have a compromise on how we proceed

Keep augmented drawings of the Diagram layer
Remove duplicates of the the Icon layer

Those two make sense.

Add preferredView="text" for regular models and preferredView="icon" for icons

I would prefer if we don't use preferredView unless really needed, and especially not have preferredView="icon". If you want to understand a model the text, diagram or documentation layers are the most useful, and switching to a different layer often feels a bit odd.

And you often (at least in many tools) click on the icon to get to the model - so you already know what the icon looks like (e.g. for the "Add"-block - which adds signals) - and wanted to understand it more; so just showing the icon or another image looking like the icon doesn't really benefit the users.

christiankral added a commit to christiankral/ModelicaStandardLibrary that referenced this issue Jan 11, 2020
christiankral added a commit to christiankral/ModelicaStandardLibrary that referenced this issue Jan 11, 2020
christiankral added a commit to christiankral/ModelicaStandardLibrary that referenced this issue Jan 11, 2020
@christiankral
Copy link
Contributor Author

Add preferredView="text" for regular models and preferredView="icon" for icons

I would prefer if we don't use preferredView unless really needed, and especially not have preferredView="icon". If you want to understand a model the text, diagram or documentation layers are the most useful, and switching to a different layer often feels a bit odd.

And you often (at least in many tools) click on the icon to get to the model - so you already know what the icon looks like (e.g. for the "Add"-block - which adds signals) - and wanted to understand it more; so just showing the icon or another image looking like the icon doesn't really benefit the users.

No preferredView was added in #3033 as there was no need for it.

@christiankral
Copy link
Contributor Author

christiankral commented Jan 12, 2020

I removed redundant diagram layers and partially moved some images to the info layer. Many packages I am familiar with are covered by #3303.

However, the following packages I am definitely not familiar with:

  • Modelica.Clocked
  • Modelica.StateGraph
  • Modelica.Mechanics.MultiBody
  • Modelica.Mechanics.Rotational
  • Modelica.Mechanics.Translational
  • Modelica.Fluid
  • Modelica.Media

I kindly invite the library officers of these libraries to go through possibly affected models and remove redundant diagram drawings and/or place a copy of the drawing in the info layer.

@casella
Copy link
Contributor

casella commented Jan 13, 2020

I quickly scanned Modelica.Fluid, if I understood what this issue is (which I'm not 100% sure) I only found two examples, namely Fittings.SharpEdgedOrifice and Fittings.AbruptAdaptor. I'm not aware of any such cases in Modelica.Media, since there are no components to be used with drag 'n drop there, only replaceable base classes and records.

I don't think this is very high priority, I'll see what I can do, but I guess I have other much more relevant issues to take care of before the deadline.

@tobolar
Copy link
Contributor

tobolar commented Jan 14, 2020

I will proceed with Mechanics when PR #3312 is merged in order to omit conflicts.

@beutlich
Copy link
Member

I will proceed with Mechanics when PR #3312 is merged in order to omit conflicts.

#3312 was merged.

@tobolar
Copy link
Contributor

tobolar commented Jan 22, 2020

  • Modelica.Mechanics.Rotational
  • Modelica.Mechanics.Translational

Both done with 2f6ae65

@christiankral
Copy link
Contributor Author

I guess this ticket is resolved for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue addresses the documentation enhancement New feature or enhancement
Projects
None yet
Development

No branches or pull requests