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

Xnos addition #94

Merged
merged 20 commits into from
Aug 18, 2020
Merged

Xnos addition #94

merged 20 commits into from
Aug 18, 2020

Conversation

dendrondal
Copy link
Collaborator

Adds Xnos & shorcaption compatibility, updating documentation accordingly, as outlined in #87

@tompollard
Copy link
Owner

great, thanks. was the .idea folder intended to be included? from a quick google it looks like this is an IDE folder of some sort.

@dendrondal
Copy link
Collaborator Author

Yep, that's an ide artifact, didn't intend to add it. Also still need to fix the Travis yaml, I likely just need to include sudo permissions on the pip install so that it adds to PATH.

Do you know whether OSX High Sierra includes pip3 by default? I'm currently using system python, which probably isn't a great replication of a working environment.

@tompollard
Copy link
Owner

tompollard commented Jun 17, 2020

Do you know whether OSX High Sierra includes pip3 by default? I'm currently using system python, which probably isn't a great replication of a working environment.

If you are running Python 3, then most likely pip is an alias of pip3 I think. I usually work in a virtual environment, but you can see that both commands refer to the same version:

pip --version

pip 20.1.1 from .../env/lib/python3.7/site-packages/pip (python 3.7)
pip3 --version

pip 20.1.1 from .../env/lib/python3.7/site-packages/pip (python 3.7)

Not sure why the test is failing here but I'll have a look if you don't manage to work it out.

@dendrondal
Copy link
Collaborator Author

Just using pip3 fixed everything. Let me know if you're wanting to merge or make any more changes beforehand.

@tompollard
Copy link
Owner

Thanks for sorting this out, and from a quick review it looks good to me. I'll do a bit of testing over the weekend and will merge unless anything comes up!

@davidverweij
Copy link
Contributor

davidverweij commented Aug 4, 2020

@tompollard Just supporting this PR. I wanted the HTML output to also create clickable references to the pictures (the current implementations omits these likely due to the LaTeX annotation for referencing figures). Using pandoc-fignos enables this. For the make pdf I did however had to fix figure caption prefixes. It did this for the make tex but not the pdf. The logs for make tex indicated to add the following settings, which I added to the template.tex:


%% pandoc-fignos: environment to disable figure caption prefixes
\makeatletter
\newcounter{figno}
\newenvironment{fignos:no-prefix-figure-caption}{
  \caption@ifcompatibility{}{
    \let\oldthefigure\thefigure
    \let\oldtheHfigure\theHfigure
    \renewcommand{\thefigure}{figno:\thefigno}
    \renewcommand{\theHfigure}{figno:\thefigno}
    \stepcounter{figno}
    \captionsetup{labelformat=empty}
  }
}{
  \caption@ifcompatibility{}{
    \captionsetup{labelformat=default}
    \let\thefigure\oldthefigure
    \let\theHfigure\oldtheHfigure
    \addtocounter{figure}{-1}
  }
}
\makeatother

I tried to understand from your PR @dendrondal whether your commits include a similar 'fix', but am not sure. Did you encounter the LaTeX error Environment fignos:no-prefix-figure-caption undefined.?

  • edit - potentially because I am still using the template.tex?


<!--
Figures can be added with the following syntax:
![my_caption \label{my_label}](source/figures/my_image.pdf){ width=50% }
![main_text_caption](source/figures/my_image.pdf, "short_caption(optional)"){#fig:mylabel}{ width=50% }
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, there shouldn't be a comma between the image source and the short caption.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the support! I didn't realize it added links to the HTML, that's a nice bonus.

I'll fix that annotation, and see about fixing the template.tex. To be honest, I had never used a template for the body and forgot to add it back in. I'll run it with the proposed changes to make sure it works as intended.

W.r.t. the no-prefix-figure error message, that occurs if any figure doesn't have the {#number:label} notation at the end. If I remember correctly, the pdf still compiled as expected, despite giving that error? I looked up this error on the fignos page, and it appears that adding the --standalone flag may fix this, which will add some peace of mind for end users. Will try it out and get back to you.

Copy link
Owner

Choose a reason for hiding this comment

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

@dendrondal sorry for not getting around to dealing with this yet. if you are planning an update, i'll hold on for that and will then check and merge. thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@davidverweij Can you elaborate a bit more on your issue with caption prefixes? I've re-run the build both with and without the addition to the template.tex after adding the template flag to the makefile, and the output appears the same to me.

README.md Outdated
- [Pandoc](http://johnmacfarlane.net/pandoc), for converting the Markdown to the output format of your choice. You may also need to install [Pandoc cite-proc](http://pandoc.org/demo/example19/Extension-citations.html) to create the bibliography.
- Install @martisak's shortcaption module for Pandoc, with `pip install pandoc-shortcaption`
- [Pandoc](http://johnmacfarlane.net/pandoc), for converting the Markdown to the output format of your choice.
- Pandoc plugins by running ```bash install.sh``` in the main direcotry
Copy link
Owner

Choose a reason for hiding this comment

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

Did you mean this bash script? I don't see it! Should the install instructions be added to the makefile instead? (e.g. with a make install recipe)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that script didn't make it to a later release, since the Dockerfile made it redundant. I've added it back in. Having a make install recipe is an interesting idea. I'm going to put both in, as I don't want the lack of a shebang in the Makefile causing anyone headaches.

Copy link
Owner

@tompollard tompollard left a comment

Choose a reason for hiding this comment

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

Thanks @dendrondal, this looks good. I've added a couple of minor comments. The main issue is that install.sh is missing currently. Should this be incorporated into the makefile?

source/01_title_page.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

\pagenumbering{roman}
\setcounter{page}{3}
\listoffigures
Copy link
Contributor

Choose a reason for hiding this comment

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

Just my 2 cents, but I would add a strong suggestion to ensure short captions are provided so this list remains comprehensible. Alternatively two figures (one with, and one without short caption) could show how to use this.

<!-- 
The \listoffigures will use short captions first, and the whole caption if none is present. To keep this list readable, ensure each figure has a short caption, e.g.
![main_text_caption](source/figures/my_image.pdf "short caption used in alt text and \listoffigures"){#fig:mylabel}{ width=50% }
-->

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added both for the sake of clarity.

@tompollard
Copy link
Owner

Looks great, thanks @davidverweij and @dendrondal !

@tompollard tompollard merged commit 667d0cc into tompollard:master Aug 18, 2020
dendrondal added a commit to dendrondal/phd_thesis_markdown that referenced this pull request Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants