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

Enabled 'z' to force-justify the last line of a score. #459

Merged
merged 5 commits into from
May 27, 2015

Conversation

henryso
Copy link
Contributor

@henryso henryso commented May 26, 2015

Fixes #43.

In implementing this, I found a bug in the way elements are "cut", which I think I've fixed. The tests pass, except for one which ended with a 'Z', and whose result is acceptable.

This is ready for review.

@henryso henryso changed the title Enabled 'z' to force-justify the last line of a score. Fixes #43. Enabled 'z' to force-justify the last line of a score. May 26, 2015
@eroux
Copy link
Contributor

eroux commented May 26, 2015

Just to be sure; did you check that the (vertical) space after the score is the same with this change? I don't think it changed, but it's something that is rarely tested and not obvious (sometimes the change in spacing is very small).

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

There is a difference in the vertical space, about the height of one character. I'll see if I can fix it.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

I'm stumped. I don't know what is generating that extra space. Any hints?

@eroux
Copy link
Contributor

eroux commented May 26, 2015

There's a code in gregoriotex.lua that removes empty lines, there is maybe something it doesn't manage to remove here...

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

The line that both adds the extra space and enables the justification is \grepenalty{-10001} which evaluates (in this case) to \penalty-10001.

I took a look at the Lua code, and for the case without the z, the process function doesn't remove any lines. In the case with the z, it does remove a single line.

I don't claim to understand the underlying TeX structure, so I don't feel confident messing with this function.

@eroux
Copy link
Contributor

eroux commented May 26, 2015

Debugging this is not easy, but I think you'll learn a lot... What I'd probably do is inspect the number of lines in the post_linebreak lua hook, it's relatively easy, and you'll be able to see what the lines contain, and if there is something that should be removed...

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

That's the thing... The number of lines is correct (after the empty line is removed) and the last non-empty line contains, apparently, the content of the last line. Are you saying I should look to remove something from within the last line?

@eroux
Copy link
Contributor

eroux commented May 26, 2015

Would you have a minimal example showing the problem?

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

fix-43.gabc
name: test for force justify of last line via 'z';
%%
(c3)
g(g) g(g) g(g) g(g) g(g)
g(g) g(g) g(g) g(g) g(g)
g(g) g(g) g(g) g(g) g(g)
g(g) g(g) g(g) g(g) g(g)
g(g) g(g) g(g) g(g) g(g)
g(g) g(g) (::f+z)
fix-43.tex
\documentclass[11pt]{article}
\usepackage[debug=all]{gregoriotex}
\usepackage{libertine}
\begin{document}
\includescore[f]{fix-43}
*****
\end{document}

@eroux
Copy link
Contributor

eroux commented May 26, 2015

A clue:

diff --git a/tex/gregoriotex.lua b/tex/gregoriotex.lua
index e61ee1b..3020140 100644
--- a/tex/gregoriotex.lua
+++ b/tex/gregoriotex.lua
@@ -160,6 +160,9 @@ local function process (h, groupcode, glyphes)
       currentshift=0
     end
   end
+  for n in node.traverse(h) do
+    texio.write_nl('found a '..node.type(n.id))
+  end
   -- due to special cases, we don't return h here (see comments in bug #20974)
   return true
 end 

Another one:

\setlength{\baselineskip}{0pt plus 0pt minus 0pt}

@eroux
Copy link
Contributor

eroux commented May 26, 2015

BTW, setting baselineskip to 0 (or a user defined value) for the time of a score is probably a good idea...

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

So it looks like there's an extra glue at the end. I'll look into this more closely tonight.

@eroux
Copy link
Contributor

eroux commented May 26, 2015

I think TeX adds \baselineskip after each line, and when we remove one we forget to remove the \beaselineskip afterwards. So the bug certainly appears in other circumstances.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

The glue subtype appears to be 1 (n.subtype, using the code above, unless I'm doing this wrong), which, from the documentation, should be \lineskip, unless I'm reading that wrong, too.

@eroux
Copy link
Contributor

eroux commented May 26, 2015

baselineskip might be a LaTeX wrapper around lineskip yes

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

The extra glue is actually before the empty line that we remove. I tried removing this glue when the empty line is removed, but this only shrinks the vertical size very slightly, and certainly not a character's height.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

The line prior to the glue before the empty line looks like the last line produced with no z, except that there is no parfillskip before the rightskip at the end. There's some other magic going on here.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

Setting baselineskip to 0 made no difference. However, setting lineskip to 0 did tighten things up. Not only did the space below the score shrink, but the space between lines within the score also shrunk. I don't know if this is the right thing to do, though.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

That said, even with lineskip set to 0, it is still tighter when you leave out the z. I'm out of ideas, at least for now. Maybe I'll think of something as time goes by.

@eroux
Copy link
Contributor

eroux commented May 26, 2015

Strange, in my tests setting baselineskip to 0 did work... I'm trying with

\documentclass[11pt]{article}
\usepackage{gregoriotex}
\usepackage{libertine}
\begin{document}

\setlength{\baselineskip}{0pt plus 0pt minus 0pt}

\includescore[a]{fix-43}
*****

\includescore[a]{fix-43-noz}
****
\end{document}

What's your test?

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

I actually set baselineskip within begingregorioscore and set it back in endgregorioscore Perhaps the key is there somewhere.

@eroux
Copy link
Contributor

eroux commented May 26, 2015

if you reset it in endgregorioscore, you must do it after \par, otherwise it won't work

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

I'm resetting it right before \relax at the end. If I don't reset it, then it does appear to work.

@rpspringuel
Copy link
Contributor

Be sure to have a bunch of text (multiple lines) after the score to make sure that it appears right. You can use \lipsum from the lipsum package to create such a block of text really quick.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

Yes, I am testing with Lorem Ipsum text (though not \lipsum). That said, I don't have a good solution yet.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

An interesting observation: If the last line is incidentally justified by TeX (when the notes just happen to line up that way, even without the z at the end), then I see the extra space. In this case, also, the callback is removing an empty line.

@henryso
Copy link
Contributor Author

henryso commented May 26, 2015

I think I've gotten to the bottom of the cause, but I don't know how to fix it correctly.

In both the z case and the incidentally justified case, there is a negative penalty before the \rightskip at the end of the last line. In the case of z, this value is fixed at -10001, and comes from the \grenewlinecommon macro. In the case of the incidentally justified case, it's just some negative value.

(below has been edited)

If I remove the penalty in \grenewlinecommon, the space below the score magically goes away and so does the justification. Is there a proper way to deal with this?

Thoughts on a solution?

@henryso
Copy link
Contributor Author

henryso commented May 27, 2015

After thinking about this for a while, I'm thinking the best thing to do would be to put a \grepenalty{-10001} in \endgregorioscore. Then you always get the space and everything remains consistent regardless of how the score ends. However, this may just be my frustration talking. Anyone have any other ideas?

@eroux
Copy link
Contributor

eroux commented May 27, 2015

That would make things consistent indeed... I'll try to inspect that later today.

@eroux
Copy link
Contributor

eroux commented May 27, 2015

Here is another approach: use

\parfillskip 0pt plus 0pt minus 0pt

before your score, and use #461

@eroux
Copy link
Contributor

eroux commented May 27, 2015

That might remove the need for the final z, with a TeX-only solution... It might be better, what do you think?

@eroux
Copy link
Contributor

eroux commented May 27, 2015

I don't know what's best to document it or make it more accessible to users, maybe setting parfillskip to a user defined value in begingregorioscore according to some \greAlignLastLineRight or something?

@henryso
Copy link
Contributor Author

henryso commented May 27, 2015

I certainly do like the TeX approach better, but as this was proposed by @rpspringuel in #43, I'd like his opinion as well.

So if I understand the complete fix, I should merge in #461, get rid of the C code here that allows the z on the final bar, and create some sort of macro that instructs begingregorioscore to set parfillskip to zero. If this is right, then it sounds good ti me.

I don't think it makes much sense to use a user-defined value here. I think you either want ragged or justified. However, I can be easily convinced otherwise. Is there a reason for a user-defined value for parfillskip?

How about \grejustifiedlastline and \greraggedlastline (I don't think we have agreed to camel-case here, at least not yet) to switch between the behaviors? I suggest "justified" because this doesn't do right-alignment in the traditional sense.

@eroux
Copy link
Contributor

eroux commented May 27, 2015

Ok, let's wait for @rpspringuel 's answer, but otherwise you're right about what to do yes. User defined value might be useful, for instance if you don't want a blank of more than x cm at the end of your score, etc. But grejustifiedlastline, etc. are ok too, the two systems can live together.

@rpspringuel
Copy link
Contributor

While I may have been the author of the Issue here, it arose out of a discussion on the user list where Joei wanted to have the last line justified like the first.

Personally, I've no problem with an entirely TeX solution as it makes the gabc file more versatile. As for the possibility of a user defined value for parfillskip, there are already mechanisms for changing that value. If the user wants that level of control, then they should be able to make those changes themselves. For most users ragged and justified should be enough.

@eroux
Copy link
Contributor

eroux commented May 27, 2015

ok then, let's do that

@henryso
Copy link
Contributor Author

henryso commented May 27, 2015

I made the changes specified here. Please review.

Note: most tests pass. @eroux's fix has tightened up the score, so some of the PDF tests fail in an acceptable manner.

@eroux
Copy link
Contributor

eroux commented May 27, 2015

looks ok to me!

henryso added a commit that referenced this pull request May 27, 2015
Enabled 'z' to force-justify the last line of a score.
@henryso henryso merged commit d67fc41 into gregorio-project:develop May 27, 2015
@henryso henryso deleted the fix-43 branch May 27, 2015 15:39
@rpspringuel
Copy link
Contributor

I see not problems either, but haven't done any real testing myself.

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