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

Tighten connection between \firstuse and document index #3043

Merged
merged 8 commits into from
Nov 30, 2021

Conversation

henrikt-ma
Copy link
Collaborator

@henrikt-ma henrikt-ma commented Nov 21, 2021

Fixes #3041.

More importantly, this instruments \firstuse with capabilities for adding introduced terminology to the index. The point is to strengthen the connection between introducing terminology in the text and making the terminology searchable in the document index. By default, the argument to \firstuse is added as-is to the index, but since the desired appearance in the index is often not exactly the same as the appearance in the text, \firstuse also takes an optional argument for specifying how the terminology should be presented in the index. Finally, on rare occasions one doesn't want an entry in the index at all (for example if a word is already present in the form of a keyword), and then one can pass an em-dash as the optional argument (for example, \firstuse[---]{partial}).

To support reviewers, I made a diff of the MLS.idx to show the effect of the commit Make \firstuse add to document index, with possibility to override:

10c10
< \indexentry {literal|hyperpage}{11}
---
> \indexentry {literal (constant)|hyperpage}{11}
110c110
< \indexentry {named elements|hyperpage}{44}
---
> \indexentry {named element|hyperpage}{44}
213a214
> \indexentry {derived from|hyperpage}{81}
263c264
< \indexentry {connection equations|hyperpage}{117}
---
> \indexentry {connection equation|hyperpage}{117}

The changes seen are intentional and the result of manually going through all occurrences of \firstuse.

Edit: The diff and the comment to the diff was updated after removing forgotten debug prints in the implementation of \firstuse.

Copy link
Member

@eshmoylova eshmoylova left a comment

Choose a reason for hiding this comment

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

I looked at a couple of changes and how they look in the build by Jenkins. It appends CUSTOM and NOTHING to the text. I am not sure if the rule is not defined correctly or something went wrong with the build. Could you check what you get in your build and if something needs to be updated? Also styleguide.md should be updated to document the new options.

chapters/annotations.tex Show resolved Hide resolved
chapters/scoping.tex Show resolved Hide resolved
Copy link
Member

@eshmoylova eshmoylova left a comment

Choose a reason for hiding this comment

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

I did not check every item (maybe 15 or so, at random that have different styles). Looks good apart from the comment about encapsulated (but may be it needs to be a separate issue?) and also an update to the styleguide.

chapters/annotations.tex Show resolved Hide resolved
chapters/scoping.tex Show resolved Hide resolved
@henrikt-ma
Copy link
Collaborator Author

Also styleguide.md should be updated to document the new options.

How about this?

styleguide.md Outdated Show resolved Hide resolved
chapters/classes.tex Outdated Show resolved Hide resolved
chapters/scoping.tex Show resolved Hide resolved
Co-authored-by: Elena Shmoylova <eshmoylova@users.noreply.github.com>
@henrikt-ma
Copy link
Collaborator Author

(I still don't know how you do it, but I can't figure out how to reply directly to the comments.)

Oh, sorry. I missed that there were three entries in the index (HTML):
image
Actually, in PDF there are only two entries:
image
I guess two of them on the same page, so they got merged?

Yes, I believe it should work that way.

Right, I first looked at the pdf and tried to follow the links there. But the pdf shows the whole page when you click on the link, and I wanted to see the exact place. So I switched to the HTML. I clicked on the first two links because in my mind there were only two, and missed the third one completely.

Is there a way to sort the entries in the index? The first item on that list is in Section 5.3.2, the second in 5,3,3, and the third in 5.3.1 is this paragraph above. It would make more sense to have 5.3.1 to be the first one.

Not that I know, unfortunately. I suppose it could be the topic of a new LaTeXML issue.

@eshmoylova
Copy link
Member

(I still don't know how you do it, but I can't figure out how to reply directly to the comments.)

I usually go to the Files tab and look through the comments there (Ctrl + f can be helpful when like in this change there are so many files). I cannot make sense sometimes of how the Conversation tab combines comments especially if they are many comments that are part of one "Review"). If I see a comment in an email that I cannot find in the Files tab I search through the Conversation.

Is there a way to sort the entries in the index? The first item on that list is in Section 5.3.2, the second in 5,3,3, and the third in 5.3.1 is this paragraph above. It would make more sense to have 5.3.1 to be the first one.

Not that I know, unfortunately. I suppose it could be the topic of a new LaTeXML issue.

Maybe it's irrelevant because the assumption is that you click on all items.

chapters/scoping.tex Show resolved Hide resolved
chapters/classes.tex Outdated Show resolved Hide resolved
chapters/connectors.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Looks good except for the specific comments.

Copy link
Collaborator

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Looks good.

@HansOlsson HansOlsson merged commit e1e088a into modelica:master Nov 30, 2021
@henrikt-ma henrikt-ma deleted the cleanup/firstuse branch November 30, 2021 21:16
@HansOlsson HansOlsson added the M36 For pull requests merged into Modelica 3.6 label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M36 For pull requests merged into Modelica 3.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"!" in "constant!variable"
3 participants