Re: First SVG graphic

On 07.02.19 12:06, Peter Eisentraut wrote:

> First, let's fix some of these whitespace errors:
>
> firstSvg_2.patch:677: trailing whitespace.
>    <rect x="1" y="1" width="99.4%" height="99.4%" rx="1%"
> firstSvg_2.patch:752: trailing whitespace.
>    <rect x="1" y="1" width="99.4%" height="99.4%" rx="1%"
> firstSvg_2.patch:705: new blank line at EOF.
> +
> firstSvg_2.patch:936: new blank line at EOF.
> +
> warning: 4 lines add whitespace errors.

Done.

> Let's not use mixed-case file names:
>
> Inkscape/
> PageLayout.svg
> gin.svg
> pgDump.svg
Done.
>> @@ -152,15 +156,15 @@ postgres.txt: postgres.html
>>   postgres.pdf:
>>    $(error Invalid target;  use postgres-A4.pdf or postgres-US.pdf as targets)
>>
>> -%-A4.fo: stylesheet-fo.xsl %.sgml $(ALLSGML)
>> +%-A4.fo: stylesheet-fo.xsl %.sgml
>>    $(XMLLINT) $(XMLINCLUDE) --noout --valid $(word 2,$^)
>>    $(XSLTPROC) $(XMLINCLUDE) $(XSLTPROCFLAGS) --stringparam paper.type A4 -o $@ $(wordlist 1,2,$^)
>>
>> -%-US.fo: stylesheet-fo.xsl %.sgml $(ALLSGML)
>> +%-US.fo: stylesheet-fo.xsl %.sgml
>>    $(XMLLINT) $(XMLINCLUDE) --noout --valid $(word 2,$^)
>>    $(XSLTPROC) $(XMLINCLUDE) $(XSLTPROCFLAGS) --stringparam paper.type USletter -o $@ $(wordlist 1,2,$^)
>>
>> -%.pdf: %.fo
>> +%.pdf: %.fo $(ALLSGML) $(SVGSRC)
>>    $(FOP) -fo $< -pdf $@
>>
> This seems a bit wrong.  The .fo target does depend on ALLSGML.  The
> .pdf target does not, but it presumably does depend on SVGSRC.
It's a transitive dependency: the pdf target is triggered after changes 
in svg (or sgml), this triggers the fo targets. Therefore it's not 
necessary to have svg (or sgml) dependencies at the fo level.
> The variable name SVGSRC is a bit confusing.  What is it the source of?
The variable SVGSRC points to all svg-files, similar to ALLSGML which 
points to the sgml files. Whenever any of them changes, certain targets 
will fire.
>> @@ -209,7 +213,7 @@ check: postgres.sgml $(ALLSGML) check-tabs
>>   install: install-html install-man
>>
>>   installdirs:
>> - $(MKDIR_P) '$(DESTDIR)$(htmldir)'/html $(addprefix '$(DESTDIR)$(mandir)'/man, 1 3 $(sqlmansectnum))
>> + $(MKDIR_P) '$(DESTDIR)$(htmldir)'/html/svg html/svg $(addprefix '$(DESTDIR)$(mandir)'/man, 1 3 $(sqlmansectnum))
>>
>>   # If the install used a man directory shared with other applications, this will remove all files.
>>   uninstall:
> html/svg is not an installation directory.  You need to create it
> somewhere else.
Please help. I haven't understood the distinction between installation 
directory and DESTDIR. On the other hand, in the Makefile there is a - 
redundant - command within the html-stamp target: $(MKDIR_P) html/svg. 
But this will run frequently, which is not necessary.
>> + <part>
>> +  <title>Lists of Figures, Tables and Examples</title>
>> +  <appendix id="list-of-figures">
>> +   <title>List of Figures</title>
>> +   <para />
>> +  </appendix>
>> +  <appendix id="list-of-tables">
>> +   <title>List of Tables</title>
>> +   <para />
>> +  </appendix>
>> +  <appendix id="list-of-examples">
>> +   <title>List of Examples</title>
>> +   <para />
>> +  </appendix>
>> + </part>
> These ought to be created by the stylesheet.  We have probably turned
> them off somewhere, so you should see where you can turn them on.

There is a simple mechanism to create those list of figures: change line 
55 of stylesheet-html-common.xsl to "book toc,title,*figure*". But the 
result is ugly - see attached screenshot. The list is out-of-line. 
Additionally, in the future we will have many figures (and examples and 
tables). This will lead to similar problems we actually faced with the 
release notes. The proposed solution moves this inflation of lists to 
deeper levels of the TOC. We can and have defined theirs layout within 
stylesheet-common.xsl.

>> diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
>> index 9e0bb93f08..d31ee2d210 100644
>> --- a/doc/src/sgml/ref/pg_dump.sgml
>> +++ b/doc/src/sgml/ref/pg_dump.sgml
>> @@ -73,6 +73,21 @@
>>      architectures.
>>     </para>
>>
>> +  <figure id="pg_dump_svg">
>> +   <title><command>pg_dump</command>: Formats and Restore Proceedings</title>
> This doesn't work for man page output.
>
> I think we should avoid putting images into reference pages.  This one
> could perhaps go into the Backup chapter.

What is the problem here? Actually I don't have enough time to evaluate 
it in deep. If it is an urgent problem (I have seen that the 
commitfest-entry is tagged as "release 12") we shall shift the pg_dump 
figure to a later release.

I think that the Backup chapter isn't a good place for this because it 
does not explain the complete interaction between pg_dump, psql and restore.

> Also, it should be linked to from somewhere.  An image that's just
> floating around and not referred to in the text seems odd.
>
> Also we tend to use hyphens instead of underscores for IDs.  (At some
> point, underscores where not allowed.  I'm surprised that that's no
> longer the case.)
Done. (But I hate the use of the minus-signs within any identifier of 
any language. For me it's an operator.)
>    I also wouldn't put "_svg" into the ID.  The format
> is irrelevant to the ID.
>
I changed _svg to _figure. I agree that the format is irrelevant. But it 
may be of interest, whether it is an id to a text or a figure.


Kind regards, Jürgen