Skip to content

Shorten identifier for two reasons #3685

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HansOlsson
Copy link
Collaborator

The reasons are:

  • It is too long
  • It causes problem for HTML it seems (needs to be verified)

Closes #3663

- It is too long
- It causes problem for HTML it seems
Closes modelica#3663
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

Looks alright, but we should wait until #3661 is merged to avoid conflicts.

@henrikt-ma
Copy link
Collaborator

  • It causes problem for HTML it seems (needs to be verified)

For the record, this was verified in #3661 and the LaTeXML issue has been reported here:

Comment on lines 2255 to 2256
{\lstinline!annotation(IncludeDirectory="modelica:/PackageName/Resources/Include")!}\annotationindex{IncludeDirectory}, used to specify a location for header files.
The preceding one is the default and need not be specified; but another location could be specified by using an URI name for the include directory, see \cref{external-resources}.
Copy link
Collaborator

@henrikt-ma henrikt-ma May 5, 2025

Choose a reason for hiding this comment

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

I realize that something important is being lost regarding the default; it needs to be clear that the default is the name of the current library's top-level package. PackageName isn't making this clear, and the explanation a few paragraphs below the item list is too hard to find.

Something like the following would preserve the meaning and still drastically shorten the \lstinline arguments:

Suggested change
{\lstinline!annotation(IncludeDirectory="modelica:/PackageName/Resources/Include")!}\annotationindex{IncludeDirectory}, used to specify a location for header files.
The preceding one is the default and need not be specified; but another location could be specified by using an URI name for the include directory, see \cref{external-resources}.
The {\lstinline!annotation(IncludeDirectory="$\mathit{uri}$")!}\annotationindex{IncludeDirectory}, used to specify a URI location (\cref{external-resources}) for header files.
The default location -- not requiring an annotation -- is \filename{modelica:/$L$/Resources/Include}, see below.

(The use of L for the top-level package name really helps getting nice line breaks in the PDF.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is an explanation for PackageName (changed from the previous name in this PR) after the list.

Obviously we can improve that, but to me that would be a separate PR, as this was just intended to have shorter names to fix two issues.

@@ -2252,11 +2252,11 @@ \subsection{Annotations for External Functions}\label{annotations-for-external-l
\end{nonnormative}
\item
The
Copy link
Collaborator

Choose a reason for hiding this comment

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

Added back on the correct line in the suggestion below:

Suggested change
The

@@ -2293,7 +2293,7 @@ \subsection{Annotations for External Functions}\label{annotations-for-external-l
The directories may use symbolic links or use a text-file as described below: e.g., a text-file \filename{vs2008} containing the text \emph{../win32/vs2005} (or \emph{vs2005}) suggesting that it is compatible with vs2005.
\end{nonnormative}

The {\lstinline!ModelicaLibraryName!} used for {\lstinline!IncludeDirectory!}, {\lstinline!LibraryDirectory!}, and {\lstinline!SourceDirectory!} indicates the top-level class where the annotation is found in the Modelica source code.
The {\lstinline!PackageName!} used for {\lstinline!IncludeDirectory!}, {\lstinline!LibraryDirectory!}, and {\lstinline!SourceDirectory!} indicates the top-level class where the annotation is found in the Modelica source code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To go with suggestion above:

Suggested change
The {\lstinline!PackageName!} used for {\lstinline!IncludeDirectory!}, {\lstinline!LibraryDirectory!}, and {\lstinline!SourceDirectory!} indicates the top-level class where the annotation is found in the Modelica source code.
In the default locations for {\lstinline!IncludeDirectory!}, {\lstinline!LibraryDirectory!}, and {\lstinline!SourceDirectory!}, the $L$ indicates the top-level class where the annotation is found in the Modelica source code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As indicated I think that's a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason why I don't think it should be a separate PR is that while this PR is currently reduces very long lines – which looks nice – it is making it harder to see that the default location is tied to the top-level package. This paragraph is too far away from where it applies to cover up for the less informative PackageName compared to the old ModelicaLibraryName.

@HansOlsson
Copy link
Collaborator Author

Looks alright, but we should wait until #3661 is merged to avoid conflicts.

I was thinking that this would completely replace #3661

@HansOlsson
Copy link
Collaborator Author

  • It causes problem for HTML it seems (needs to be verified)

For the record, this was verified in #3661 and the LaTeXML issue has been reported here:

I also needed to verify that the shorter text fixes the issue in this case, so I had to wait for the CI. (and, yes, it did)

@HansOlsson HansOlsson added the Generated HTML The generated HTML-code label May 7, 2025
@HansOlsson HansOlsson requested a review from henrikt-ma May 9, 2025 15:13
@HansOlsson
Copy link
Collaborator Author

As indicated I think this is good enough on its own to correct the HTML-bug and be more readable, and any improvement in formulations should be a separate PR.

@henrikt-ma
Copy link
Collaborator

I suggest we wrap up #3661 first, and then return to this PR as a change which is unrelated to working around the LaTeXML bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Generated HTML The generated HTML-code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious percentage in HTML-text
2 participants