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

[patterns.meta] Remaining bugs when using definitions and there is a space in the wrong place #851

Closed
1 of 2 tasks
Mo-Gul opened this issue Apr 29, 2020 · 5 comments
Closed
1 of 2 tasks

Comments

@Mo-Gul
Copy link
Contributor

Mo-Gul commented Apr 29, 2020

This bug/these bugs were already mentioned in issue #602 (comment), but since I am not sure if these are really bugs and if something can be done against them (easily) I report them separately here again.

To raise the errors follow the comment instructions at the use of the pattern.

  • 1. remove the comment sign after old lines[

  • 2. add a space before the [ in old lines[ (i.e. make it old lines [)

\documentclass[border=5pt]{standalone}
\usepackage{pgfplots}
    \usetikzlibrary{patterns.meta}
    % copied from `pgfmanual-en-library-patterns.tex` and changed the `name`
    \tikzdeclarepattern{
        name=old lines,
        parameters={
            \pgfkeysvalueof{/pgf/pattern keys/size},
            \pgfkeysvalueof{/pgf/pattern keys/angle},
            \pgfkeysvalueof{/pgf/pattern keys/line width},
        },
        bounding box={
            (-.1pt,-.1pt) and
            (\pgfkeysvalueof{/pgf/pattern keys/size}+.1pt,
             \pgfkeysvalueof{/pgf/pattern keys/size}+.1pt)
        },
        tile size={
            (\pgfkeysvalueof{/pgf/pattern keys/size},
             \pgfkeysvalueof{/pgf/pattern keys/size})
        },
        tile transformation={rotate=\pgfkeysvalueof{/pgf/pattern keys/angle}},
        defaults={
            size/.initial=5pt,
            angle/.initial=0,
            line width/.initial=.4pt,
        },
        code={
            \draw[line width=\pgfkeysvalueof{/pgf/pattern keys/line width}]
                (0,0) -- (\pgfkeysvalueof{/pgf/pattern keys/size},
                          \pgfkeysvalueof{/pgf/pattern keys/size});
        },
    }
\begin{document}
\begin{tikzpicture}
    \draw [
        pattern={
            % remove the comment sign in the next line to get an error
            % add a space before the `[` to get another error
            old lines[%
                size = 10pt,
                line width = 2pt,
                angle = 0,
            ]
        },
        pattern color = red,
    ] (0,0) rectangle ++(2,2);
\end{tikzpicture}
\end{document}
@hmenke
Copy link
Member

hmenke commented Apr 29, 2020

--- a/tex/generic/pgf/libraries/pgflibrarypatterns.meta.code.tex
+++ b/tex/generic/pgf/libraries/pgflibrarypatterns.meta.code.tex
@@ -217,7 +217,7 @@
 
 \def\pgf@pat@macroaskeys#1#2{%
   \pgfutil@toks@\expandafter{#2}%
-  \edef\pgf@marshal{\noexpand\pgfkeys{#1, \the\pgfutil@toks@}}%
+  \edef\pgf@marshal{\noexpand\pgfkeys{,#1,\the\pgfutil@toks@}}%
   \pgf@marshal%
 }%
 \def\pgfsetfillpattern#1#2{%

hmenke added a commit to hmenke/pgf that referenced this issue Apr 29, 2020
@hmenke hmenke added this to the 3.1.6 milestone Apr 29, 2020
@hmenke hmenke closed this as completed Apr 29, 2020
@Mo-Gul
Copy link
Contributor Author

Mo-Gul commented Apr 29, 2020

That was fast! Unfortunately that fixed "only" part 1 of the bug ...
So reopen the issue or is there nothing that can be done about that?

@hmenke hmenke reopened this Apr 29, 2020
@hmenke
Copy link
Member

hmenke commented Apr 29, 2020

I found where the second one and here is a patch, but that is kind of an edge case. If I fix it like that it would break every pattern that deliberately uses leading or trailing spaces in its name for some reason. So either I have to say “pattern with surrounding whitespace in the name are not supported” or “whitespace before pattern options is not supported”.

--- a/tex/generic/pgf/libraries/pgflibrarypatterns.meta.code.tex
+++ b/tex/generic/pgf/libraries/pgflibrarypatterns.meta.code.tex
@@ -185,7 +185,7 @@
   \pgf@pat@@checkname#1[]\pgf@patstop}%
 
 \def\pgf@pat@@checkname#1[#2]#3\pgf@patstop{%
-  \def\pgf@pat@onlinename{#1}%
+  \expandafter\def\expandafter\pgf@pat@onlinename\expandafter{\romannumeral-`0\pgfutil@trimspaces{#1}}%
   \def\pgf@pat@onlineoptions{#2}%
 }%
 

@Mo-Gul
Copy link
Contributor Author

Mo-Gul commented Apr 30, 2020

I remember that "edge case". Could it be the same/something similar to issue #747?

I agree to PhelypeOleinik's opinion that one should not rely on leading or trailing spaces, but you didn't want to break possible use of that as a "feature".

Just for curiosity: Do you know any instance in the manual that relies on that feature? Than I definitely would agree to not change that. But my guess is that this is not the case.

Instead of having to decide which way to go, couldn't there be added key to choose which behavior one wants to use? Maybe this could be combined with something that PGFPlots does with its compat key. But I would use it the other way round, i.e. use the new behavior as default and the old one when the value is set.
(Did you have that or something similar in mind with #747 (comment)?)

@hmenke
Copy link
Member

hmenke commented Apr 30, 2020

The patterns.meta is relatively new, so I think I could just break it and ship it, because probably not many people are already using this. In my opinion, pgfkeys is way too lax about surrounding whitespace, which results in this general do-what-I-mean behavior, where 99% of the time it will do the right thing, but then you get those nasty edge cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants