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

Treat 'connect' like the keyword it is in listings #2785

Merged
merged 7 commits into from Apr 6, 2021

Conversation

henrikt-ma
Copy link
Collaborator

No description provided.

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.

Ok, makes sense.

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.

I'm not sure this is ideal. To me connect (and der) are more "functions" than normal keywords and thus I think I prefer the current highlighting of both of them.

@HansOlsson HansOlsson self-requested a review December 31, 2020 17:41
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.

As indicated I would need more time to consider this.

@henrikt-ma
Copy link
Collaborator Author

Right, this shouldn't be fixed for just connect:

Having more than one color for different categories of keywords sounds like a good idea, if we can only come up with enough colors that also work well in inline listings. One of the challenges I see here is the coloring of initial vs initial equation and initial algorithm – I find it hard to think of a categorization that wouldn't put these in different categories, and it remains to be seen whether listings.sty can handle two word combinations…

The real problem I'd like to address with this PR is that there are colors that are used by both keywords and non-keywords. I find it impossible to argue that readers understand that coloring has nothing to do with keyword status.

@HansOlsson HansOlsson added this to the Phone2021-1 milestone Jan 11, 2021
@HansOlsson HansOlsson modified the milestones: Phone2021-1, Phone2021-2 Mar 9, 2021
@HansOlsson
Copy link
Collaborator

Proposal: connect and der same color as non-keywords as assert, but bold-face.

@henrikt-ma
Copy link
Collaborator Author

Small update regarding work in progress: The fonts currently used for the pdfLaTeX build doesn't distinguish between medium weight and boldface, which is a bit of a regression from the 3.4 document, and meaning that the discussion in this issue so far has been partly hypothetical (there is a different in font weight in the LaTeXML build). Because of this, I'm currently trying to find a better source code font for the pdfLaTeX build. Once I've found something, I plan to switch the font as part of this PR, so that the choice of font can be discussed together with how different font weights are used for syntax highlighting.

This way, it becomes possible to both modify the default size of \ttfamily itself, and then possibly override this with \smallifpdf.

For the current teletype font we don't need this, but it has turned out to be useful in the evaluation of some other candidates.
Changes include:
- Make sure only actual Modelica keywords are highlighted as such.
- Highlight fixed strings in same way as Modelica keywords.
- Make BNF syntactical constructs stand out by having upright shape (using italics for production rule names).

That production rule names are set in italics will also make it easier to tell the difference between a grammar listing and a Modelica listing when reading the document.
@henrikt-ma
Copy link
Collaborator Author

I am a aware of the CI failure, and it should be fixed within the next few days. That aside, I consider this ready for another round of feedback.

@HansOlsson HansOlsson self-requested a review March 30, 2021 14:00
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.

Ok, I think it is ok now.

@HansOlsson HansOlsson merged commit dd31c99 into modelica:master Apr 6, 2021
@henrikt-ma henrikt-ma deleted the bugfix/connect-is-keyword branch April 9, 2021 21:21
@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.

None yet

2 participants