Thread: doc: add missing "id" attributes to extension packaging page
Hi On this page: https://www.postgresql.org/docs/current/extend-extensions.html three of the <sect2> sections are missing an "id" attribute; patch adds these. Noticed when trying to create a stable link to one of the affected sections. Regards Ian Barwick
Attachment
On 2022-Dec-05, Ian Lawrence Barwick wrote: > On this page: > > https://www.postgresql.org/docs/current/extend-extensions.html > > three of the <sect2> sections are missing an "id" attribute; patch adds > these. Noticed when trying to create a stable link to one of the affected > sections. Hm, I was reminded of this patch here that adds IDs in a lot of places https://postgr.es/m/3bac458c-b121-1b20-8dea-0665986faa40@gmx.de and this other one https://postgr.es/m/76287ac6-f415-8562-fdaa-5876380c05f3@gmx.de which adds XSL stuff for adding selectable anchors next to each id-carrying item. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
2022年12月5日(月) 18:56 Alvaro Herrera <alvherre@alvh.no-ip.org>: > > On 2022-Dec-05, Ian Lawrence Barwick wrote: > > > On this page: > > > > https://www.postgresql.org/docs/current/extend-extensions.html > > > > three of the <sect2> sections are missing an "id" attribute; patch adds > > these. Noticed when trying to create a stable link to one of the affected > > sections. > > Hm, I was reminded of this patch here that adds IDs in a lot of places > https://postgr.es/m/3bac458c-b121-1b20-8dea-0665986faa40@gmx.de > and this other one > https://postgr.es/m/76287ac6-f415-8562-fdaa-5876380c05f3@gmx.de > which adds XSL stuff for adding selectable anchors next to each > id-carrying item. Oh, now you mention it, I vaguely recall seeing those. However the thread stalled back in March and the patches don't seem to have made it to a CommitFest entry. Brar, would you like to add an entry so they don't get lost? See: https://commitfest.postgresql.org/41/ The items in my patch are covered by the above so disregard that. Regards Ian Barwick
On 06.12.2022 at 01:55, Ian Lawrence Barwick wrote: > Oh, now you mention it, I vaguely recall seeing those. However the thread > stalled back in March and the patches don't seem to have made it to a > CommitFest entry. Yes, my patches added quite a few ids and also some xsl/css logic to make them more discoverable in the browser but I had gotten the impression that nobody besides me cares about this, so I didn't push it any further. > Brar, would you like to add an entry so they don't get > lost? See: https://commitfest.postgresql.org/41/ Yes. I can certainly add them to the commitfest although I'm not sure if they still apply cleanly. I can also rebase or extend them if somebody cares. Regards, Brar
On 2022-Dec-06, Brar Piening wrote: > On 06.12.2022 at 01:55, Ian Lawrence Barwick wrote: > > > Oh, now you mention it, I vaguely recall seeing those. However the thread > > stalled back in March and the patches don't seem to have made it to a > > CommitFest entry. > > Yes, my patches added quite a few ids and also some xsl/css logic to > make them more discoverable in the browser but I had gotten the > impression that nobody besides me cares about this, so I didn't push it > any further. I care. The problem last time is that we were in the middle of the last commitfest, so we were (or at least I was) distracted by other stuff. Looking at the resulting psql page, https://pgdocs.piening.info/app-psql.html#APP-PSQL-OPTIONS-EXPANDED I note that the ID for the -x option is called "options-blah". I understand where does this come from: it's the "expanded" bit in the "options" section. However, put together it's a bit silly to have "options" in plural there; it would make more sense to have it be https://pgdocs.piening.info/app-psql.html#APP-PSQL-OPTION-EXPANDED (where you can read more naturally "the expanded option for psql"). How laborious would it be to make it so? > Yes. I can certainly add them to the commitfest although I'm not sure if > they still apply cleanly. It'll probably have some conflicts, yeah. > I can also rebase or extend them if somebody cares. I would welcome separate patches: one to add the IDs, another for the XSL/CSS stuff. That allows us to discuss them separately. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 06.12.2022 at 09:38, Alvaro Herrera wrote: > I care. The problem last time is that we were in the middle of the last > commitfest, so we were (or at least I was) distracted by other stuff. Ok, thanks. That's appreciated and understood. > Looking at the resulting psql page, > https://pgdocs.piening.info/app-psql.html#APP-PSQL-OPTIONS-EXPANDED > I note that the ID for the -x option is called "options-blah". I > understand where does this come from: it's the "expanded" bit in the > "options" section. However, put together it's a bit silly to have > "options" in plural there; it would make more sense to have it be > https://pgdocs.piening.info/app-psql.html#APP-PSQL-OPTION-EXPANDED > (where you can read more naturally "the expanded option for psql"). > How laborious would it be to make it so? No problem. I've already done it. Your second link should work now. > It'll probably have some conflicts, yeah. I've updated and rebased my branch on current master now. > I would welcome separate patches: one to add the IDs, another for the > XSL/CSS stuff. That allows us to discuss them separately. I'll send two patches in two separate e-mails in a moment. Regards, Brar
On 06.12.2022 at 18:59, Brar Piening wrote: > On 06.12.2022 at 09:38, Alvaro Herrera wrote: >> I would welcome separate patches: one to add the IDs, another for the >> XSL/CSS stuff. That allows us to discuss them separately. > > I'll send two patches in two separate e-mails in a moment. This is patch no 1 that adds ids to various elements without making them visible on the HTML surface. It is an updated and rebased version of the work discussed in the following thread: https://www.postgresql.org/message-id/f900c5e1-a18a-84cc-6536-e85ec655c7d7%40gmx.de The current statistics for docbook elements with/without ids after applying the patch are the following: name | with_id | without_id | id_coverage | min_id_len | max_id_len ---------------+---------+------------+-------------+------------+------------ sect2 | 870 | 0 | 100.00 | 7 | 57 sect1 | 739 | 0 | 100.00 | 4 | 46 refentry | 307 | 0 | 100.00 | 8 | 37 sect3 | 206 | 0 | 100.00 | 11 | 57 chapter | 77 | 0 | 100.00 | 5 | 24 sect4 | 28 | 0 | 100.00 | 16 | 47 biblioentry | 23 | 0 | 100.00 | 6 | 15 simplesect | 20 | 0 | 100.00 | 24 | 39 appendix | 15 | 0 | 100.00 | 7 | 23 part | 8 | 0 | 100.00 | 5 | 20 co | 4 | 0 | 100.00 | 18 | 30 figure | 3 | 0 | 100.00 | 13 | 28 reference | 3 | 0 | 100.00 | 14 | 18 anchor | 1 | 0 | 100.00 | 21 | 21 bibliography | 1 | 0 | 100.00 | 8 | 8 book | 1 | 0 | 100.00 | 10 | 10 index | 1 | 0 | 100.00 | 11 | 11 legalnotice | 1 | 0 | 100.00 | 13 | 13 preface | 1 | 0 | 100.00 | 9 | 9 glossentry | 119 | 14 | 89.47 | 13 | 32 table | 285 | 162 | 63.76 | 12 | 56 example | 27 | 16 | 62.79 | 12 | 42 refsect3 | 5 | 3 | 62.50 | 19 | 24 refsect2 | 41 | 56 | 42.27 | 10 | 36 varlistentry | 1701 | 3172 | 34.91 | 9 | 64 footnote | 5 | 18 | 21.74 | 17 | 32 step | 28 | 130 | 17.72 | 7 | 28 refsect1 | 151 | 1333 | 10.18 | 15 | 40 informaltable | 1 | 15 | 6.25 | 25 | 25 phrase | 2 | 94 | 2.08 | 20 | 26 indexterm | 5 | 3262 | 0.15 | 17 | 26 variablelist | 1 | 813 | 0.12 | 21 | 21 function | 4 | 4011 | 0.10 | 12 | 28 entry | 11 | 17740 | 0.06 | 21 | 40 para | 3 | 25734 | 0.01 | 21 | 27 Regards, Brar
Attachment
On 06.12.2022 at 18:59, Brar Piening wrote: > On 06.12.2022 at 09:38, Alvaro Herrera wrote: >> I would welcome separate patches: one to add the IDs, another for the >> XSL/CSS stuff. That allows us to discuss them separately. > > I'll send two patches in two separate e-mails in a moment. This is patch no 2 that adds links to html elements with ids to make them visible on the HTML surface when hovering the element. Regards, Brar
Attachment
On 06.12.2022 at 19:11, Brar Piening wrote: > The current statistics for docbook elements with/without ids after > applying the patch are the following: Somehow my e-mail client destroyed the table. That's how it was supposed to look like: name | with_id | without_id | id_coverage | min_id_len | max_id_len ---------------+---------+------------+-------------+------------+------------ sect2 | 870 | 0 | 100.00 | 7 | 57 sect1 | 739 | 0 | 100.00 | 4 | 46 refentry | 307 | 0 | 100.00 | 8 | 37 sect3 | 206 | 0 | 100.00 | 11 | 57 chapter | 77 | 0 | 100.00 | 5 | 24 sect4 | 28 | 0 | 100.00 | 16 | 47 biblioentry | 23 | 0 | 100.00 | 6 | 15 simplesect | 20 | 0 | 100.00 | 24 | 39 appendix | 15 | 0 | 100.00 | 7 | 23 part | 8 | 0 | 100.00 | 5 | 20 co | 4 | 0 | 100.00 | 18 | 30 figure | 3 | 0 | 100.00 | 13 | 28 reference | 3 | 0 | 100.00 | 14 | 18 anchor | 1 | 0 | 100.00 | 21 | 21 bibliography | 1 | 0 | 100.00 | 8 | 8 book | 1 | 0 | 100.00 | 10 | 10 index | 1 | 0 | 100.00 | 11 | 11 legalnotice | 1 | 0 | 100.00 | 13 | 13 preface | 1 | 0 | 100.00 | 9 | 9 glossentry | 119 | 14 | 89.47 | 13 | 32 table | 285 | 162 | 63.76 | 12 | 56 example | 27 | 16 | 62.79 | 12 | 42 refsect3 | 5 | 3 | 62.50 | 19 | 24 refsect2 | 41 | 56 | 42.27 | 10 | 36 varlistentry | 1701 | 3172 | 34.91 | 9 | 64 footnote | 5 | 18 | 21.74 | 17 | 32 step | 28 | 130 | 17.72 | 7 | 28 refsect1 | 151 | 1333 | 10.18 | 15 | 40 informaltable | 1 | 15 | 6.25 | 25 | 25 phrase | 2 | 94 | 2.08 | 20 | 26 indexterm | 5 | 3262 | 0.15 | 17 | 26 variablelist | 1 | 813 | 0.12 | 21 | 21 function | 4 | 4011 | 0.10 | 12 | 28 entry | 11 | 17740 | 0.06 | 21 | 40 para | 3 | 25734 | 0.01 | 21 | 27
Hello,
Attached is a new patch (add_html_ids_v2.patch), almost identical to
the old but modified so that it applies. There were 2 changes (made
to sgml/plpgsql.stml and sgml/ddl.sgml) that prevented the patch from
applying. (In ddl.sgml the VACUUM and ANALYZE privileges were
combined into MAINTAIN. In plpgsql.sgml an id attribute was added.)
If the author will look over my version of the patch I believe it can
be approved and sent on to the committers.
What follows is my notes for the committers:
The ids look reasonably consistent, with "nesting" so that ids of
sub-sections mostly have (at least some of) the id of the parent
section as a prefix. There are a few inconsistencies. A sect3 has
id="collation-managing-standard" and sect4 has
id="collation-managing-predefined". There is a slight possibility of
conflict, as in this case sect4 ids omit the last word of the section
3 ids it is possible to have conflicts with the ids of the sect4s in
other sect3s of the same file. I don't have a problem with this.
(I see establishing strict standards for id values as excessive.)
The above was the only case I noticed. I also tried counting words,
"-" delimited, in ids and found no cases with fewer words than the
number of section levels. Here's the hack:
egrep '^\+ *<sect' /tmp/add_html_ids.patch \
| gawk '{if (int(substr($2, length($2), 1)) < split($2, dummy, "-"))
print $0;}'
As far as I know the ids are consistent with the rest of the
documentation. They are not entirely consistent in construction.
Mostly they copy the section title, but sometimes words are omitted.
E.g in sgml/charset.sgml where sect2 is "Managing Collations" with
id="collation-managing" and sect3 is "Standard Collations" with
id="collation-managing-standard". Also there is at least one
abbreviation in the id of a word in the title.
(id="installation-notes-aix-mem-management" v.s a title of "Memory
Management") All this seems fine to me.
The ids are sometimes very long. This also seems ok.
I did not do a particularly close look at the id values for
varlistentrys. Scanning the patch they seem fine.
I can confirm that all the patch does is add ids.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
Attachment
On 02.01.2023 at 21:53, Karl O. Pinc wrote: > If the author will look over my version of the patch I believe it can > be approved and sent on to the committers. LGTM. Thanks! Brar
On Tue, 3 Jan 2023 21:35:09 +0100
Brar Piening <brar@gmx.de> wrote:
> On 02.01.2023 at 21:53, Karl O. Pinc wrote:
> > If the author will look over my version of the patch I believe it
> > can be approved and sent on to the committers.
>
> LGTM.
Approved for committer!
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On Wed, 4 Jan 2023 at 04:13, Karl O. Pinc <kop@karlpinc.com> wrote: > > On Tue, 3 Jan 2023 21:35:09 +0100 > Brar Piening <brar@gmx.de> wrote: > > > On 02.01.2023 at 21:53, Karl O. Pinc wrote: > > > If the author will look over my version of the patch I believe it > > > can be approved and sent on to the committers. > > > > LGTM. > > Approved for committer! The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID cd9479af2af25d7fa9bfd24dd4dcf976b360f077 === === applying patch ./add_html_ids_v2.patch .... patching file doc/src/sgml/ref/pgbench.sgml patching file doc/src/sgml/ref/psql-ref.sgml Hunk #73 FAILED at 1824. .... 2 out of 208 hunks FAILED -- saving rejects to file doc/src/sgml/ref/psql-ref.sgml.rej [1] - http://cfbot.cputube.org/patch_41_4041.log Regards, Vignesh
On Mon, 9 Jan 2023 at 08:01, vignesh C <vignesh21@gmail.com> wrote: > > On Wed, 4 Jan 2023 at 04:13, Karl O. Pinc <kop@karlpinc.com> wrote: > > > > On Tue, 3 Jan 2023 21:35:09 +0100 > > Brar Piening <brar@gmx.de> wrote: > > > > > On 02.01.2023 at 21:53, Karl O. Pinc wrote: > > > > If the author will look over my version of the patch I believe it > > > > can be approved and sent on to the committers. > > > > > > LGTM. > > > > Approved for committer! > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: > === Applying patches on top of PostgreSQL commit ID > cd9479af2af25d7fa9bfd24dd4dcf976b360f077 === > === applying patch ./add_html_ids_v2.patch > .... > patching file doc/src/sgml/ref/pgbench.sgml > patching file doc/src/sgml/ref/psql-ref.sgml > Hunk #73 FAILED at 1824. > .... > 2 out of 208 hunks FAILED -- saving rejects to file > doc/src/sgml/ref/psql-ref.sgml.rej > > [1] - http://cfbot.cputube.org/patch_41_4041.log There are couple of commitfest entries for this: https://commitfest.postgresql.org/41/4041/ https://commitfest.postgresql.org/41/4042/ Can one of them be closed? Regards, Vignesh
On 09.01.2023 at 03:31, vignesh C wrote: > The patch does not apply on top of HEAD as in [1], please post a rebased patch: Voilà This one applies on top of 3c569049b7b502bb4952483d19ce622ff0af5fd6 and the documentation build succeeds. Beyond rebasing I've added a few more ids (to make the other patch (make_html_ids_discoverable.patch) build without warnings again) but nothing that would justify another review. We probably have to move quickly with this patch since it touches pretty much any file in the documentation and will be outdated in a minute. Regards, Brar
Attachment
On 09.01.2023 at 03:38, vignesh C wrote: > There are couple of commitfest entries for this: > https://commitfest.postgresql.org/41/4041/ > https://commitfest.postgresql.org/41/4042/ Can one of them be closed? I've split the initial patch into two parts upon Álvaro's request in [1] so that we can discuss them separately https://commitfest.postgresql.org/41/4041/ is tracking the patch you've been trying to apply and that I've just sent a rebased version for. It only adds (invisible) ids to the HTML documentation and can be closed once you've applied the patch. https://commitfest.postgresql.org/41/4042/ is tracking a different patch that makes the ids and the corresponding links discoverable at the HTML surface. Hover one of the psql options in [2] to see the behavior. This one still needs reviewing and there is no discussion around it yet. Regards, Brar [1] https://www.postgresql.org/message-id/20221206083809.3kaygnh2xswoxslj%40alvherre.pgsql [2] https://pgdocs.piening.info/app-psql.html#APP-PSQL-OPTION-PORT
On Mon, 9 Jan 2023 08:09:02 +0100
Brar Piening <brar@gmx.de> wrote:
> On 09.01.2023 at 03:31, vignesh C wrote:
> > The patch does not apply on top of HEAD as in [1], please post a
> > rebased patch:
> This one applies on top of 3c569049b7b502bb4952483d19ce622ff0af5fd6
> and the documentation build succeeds. Beyond rebasing I've added a
> few more ids (to make the other patch
> (make_html_ids_discoverable.patch) build without warnings again) but
> nothing that would justify another review.
Agreed. I believe that as long as your system has xmllint installed
and the documentation builds there's not a lot that can go wrong.
This patch only adds lots-of-id attributes.
> We probably have to move quickly with this patch since it touches
> pretty much any file in the documentation and will be outdated in a
> minute.
+1
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
Brar Piening <brar@gmx.de> writes: > On 09.01.2023 at 03:38, vignesh C wrote: >> There are couple of commitfest entries for this: >> https://commitfest.postgresql.org/41/4041/ >> https://commitfest.postgresql.org/41/4042/ Can one of them be closed? > I've split the initial patch into two parts upon Álvaro's request in [1] > so that we can discuss them separately It's not great to have multiple CF entries pointing at the same email thread --- it confuses both people and bots. Next time please split off a thread for each distinct patch. I pushed the ID-addition patch, with a few fixes: * AFAIK our practice is to use "-" never "_" in XML ID attributes. You weren't very consistent about that even within this patch, and the overall effect would have been to have no standard about that at all, which doesn't seem great. I changed them all to "-". * I got rid of a couple of "-et-al" additions, because it did not seem like a good precedent. That would tempt people to modify existing ID tags when adding variables to an entry, which'd defeat the purpose I think. * I fixed a couple of things that looked like typos or unnecessary inconsistencies. I have to admit that my eyes glazed over after awhile, so there might be remaining infelicities. It's probably going to be necessary to have follow-on patches, because I'm sure there is stuff in the pipeline that adds more ID-less tags. Or do we have a way to create warnings about that? I'm unqualified to review CSS stuff, so you'll need to get somebody else to review that patch. But I'd suggest reposting it, else the cfbot is going to start whining that the patch-of-record in this thread no longer applies. regards, tom lane
On Mon, 09 Jan 2023 15:18:18 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I pushed the ID-addition patch, with a few fixes:
>
> * AFAIK our practice is to use "-" never "_" in XML ID attributes.
> You weren't very consistent about that even within this patch, and
> the overall effect would have been to have no standard about that
> at all, which doesn't seem great. I changed them all to "-".
Apologies for not catching this.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On Mon, 09 Jan 2023 15:18:18 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote: > Brar Piening <brar@gmx.de> writes: > > On 09.01.2023 at 03:38, vignesh C wrote: > >> There are couple of commitfest entries for this: > >> https://commitfest.postgresql.org/41/4041/ > >> https://commitfest.postgresql.org/41/4042/ Can one of them be > >> closed? > > > I've split the initial patch into two parts upon Álvaro's request > > in [1] so that we can discuss them separately > I pushed the ID-addition patch, with a few fixes: > It's probably going to be necessary to have follow-on patches, > because I'm sure there is stuff in the pipeline that adds more > ID-less tags. Or do we have a way to create warnings about that? I am unclear on how to make warnings with xslt. You can make a listing of problems, but who would read it if the build completed successfully? You can make errors and abort. But my xslt and docbook and pg-docs-fu are a bit stale. I think there's more to comment on regards the xslt in the other patch, but I'll wait for the new thread for that patch. That might be where there should be warnings raised, etc. Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
"Karl O. Pinc" <kop@karlpinc.com> writes:
> I think there's more to comment on regards the xslt in the
> other patch, but I'll wait for the new thread for that patch.
> That might be where there should be warnings raised, etc.
We can continue using this thread, now that the other commit is in.
regards, tom lane
On 09.01.2023 at 21:18, Tom Lane wrote: > It's not great to have multiple CF entries pointing at the same email > thread --- it confuses both people and bots. Next time please split > off a thread for each distinct patch. I agree. I had overestimated the cfbot's ability to handle branched threads. I'll create separate threads next time. > * AFAIK our practice is to use "-" never "_" in XML ID attributes. > You weren't very consistent about that even within this patch, and > the overall effect would have been to have no standard about that > at all, which doesn't seem great. I changed them all to "-". Noted. Maybe it's worth to write a short paragraph about Ids and their style somewhere in the docs (e.g. Appendix J.5). > * I got rid of a couple of "-et-al" additions, because it did not > seem like a good precedent. That would tempt people to modify > existing ID tags when adding variables to an entry, which'd defeat > the purpose I think. I tried to use it sparsely, mostly where a varlistentry had multiple child items and I had arbitrarily pick one of them. It's not important, though. I'm curious how you solved this. > * I fixed a couple of things that looked like typos or unnecessary > inconsistencies. I have to admit that my eyes glazed over after > awhile, so there might be remaining infelicities. I'm all for consistency. The only places where I intentionally refrained from being consistent was where I felt Ids would get too long or where there were already ids in place that didn't match my naming scheme. > It's probably going to be necessary to have follow-on patches, > because I'm sure there is stuff in the pipeline that adds more > ID-less tags. Or do we have a way to create warnings about that? Agreed. And yes, we do have a limited way to create warnings (that's part of the other patch). > I'm unqualified to review CSS stuff, so you'll need to get somebody > else to review that patch. But I'd suggest reposting it, else > the cfbot is going to start whining that the patch-of-record in > this thread no longer applies. I will do that. Thanks for your feedback! Regards, Brar
Brar Piening <brar@gmx.de> writes:
> On 09.01.2023 at 21:18, Tom Lane wrote:
>> * I got rid of a couple of "-et-al" additions, because it did not
>> seem like a good precedent. That would tempt people to modify
>> existing ID tags when adding variables to an entry, which'd defeat
>> the purpose I think.
> I tried to use it sparsely, mostly where a varlistentry had multiple
> child items and I had arbitrarily pick one of them. It's not important,
> though. I'm curious how you solved this.
I just removed "-et-al", I didn't question your choice of the principal
variable name. As you say, it didn't seem to matter that much.
regards, tom lane
On 09.01.2023 at 23:28, Karl O. Pinc wrote: > On Mon, 09 Jan 2023 15:18:18 -0500 > Tom Lane <tgl@sss.pgh.pa.us> wrote: >> It's probably going to be necessary to have follow-on patches, >> because I'm sure there is stuff in the pipeline that adds more >> ID-less tags. Or do we have a way to create warnings about that? > I am unclear on how to make warnings with xslt. You can make > a listing of problems, but who would read it if the build > completed successfully? You can make errors and abort. You can emit warnings to the command line or you can abort with an error. I've opted for warnings + comments in the output in the styling patch. The biggest issue about errors and warnings is the fact that xslt does not process files in a line-based way which makes it pretty much impossible to give hints where the problem causing the warning is located. Since everything is bound together via XML entities, you can't even tell the source file. I've worked around this by also emitting an HTML comment to the output so that I can find a somewhat unique string next to it and the grep the documentation sources for this string. It's a bit ugly but the best I could come up with. I'll repost a rebased version of the styling patch in a minute. Regards, Brar
On 10.01.2023 at 06:28, Brar Piening wrote:
>
> I'll repost a rebased version of the styling patch in a minute.
After checking that there's no need for rebasing I'm reposting the
original patch here, to make cfbot pick it up as the latest one in a
somewhat screwed up thread mixing two patches (sorry for that - won't
happen again).
Althoug the patch is pretty compact you probably need some understanding
of both XSLT and CSS to understand and judge the changes it introduces.
It pretty much does two things:
1. Make html ids discoverable in the browser by adding a link with a
hover effect to items sections and varlistentries that have an id.
Hover one of the psql options and click on the hash mark in [1] to
see the behavior.
2. Emit a warning to the command line and a comment to the HTML output
when the docs build runs into a section without id or a varlistentry
without id where at least one entry in the varlist already has an id.
The original mail for the patch is at [2], the commitfest entry is at
[3] and the initial discussion leading to this patch starts at [4].
Regards,
Brar
[1] https://pgdocs.piening.info/app-psql.html#APP-PSQL-OPTION-PORT
[2]
https://www.postgresql.org/message-id/d6695820-af71-5e84-58b0-ff9f1c189603%40gmx.de
[3] https://commitfest.postgresql.org/41/4042/
[4]
https://www.postgresql.org/message-id/4364ab38-a475-a1fc-b104-ecd6c72010d0%40enterprisedb.com
Attachment
On 09.01.23 21:18, Tom Lane wrote: > * AFAIK our practice is to use "-" never "_" in XML ID attributes. > You weren't very consistent about that even within this patch, and > the overall effect would have been to have no standard about that > at all, which doesn't seem great. I changed them all to "-". In the olden says, "_" was invalid in ID attribute values. This is no longer the case. But of course it's good to stay consistent with existing practice where reasonable.
Hi Brar, Here's my first review of the make_html_ids_discoverable.patch. Overall: To start with, I'd like to say I like the goal and how everything works when the patch is applied. I was a bit skeptical, thinking that the whole thing was going to be distracting when reading the docs or otherwise odd, but it really works. The thought comes to mind to take accessibility into account. I am unclear on what that would mean in this case. Regards CSS: The CSS looks good. Also, it passes the W3C CSS validator ( https://jigsaw.w3.org/css-validator/) for CSS level 3. Regards XSLT: I believe the XSLT needs work. Without really knowing the style of PG's XSLT, I'm going to suggest putting the comment <!-- Make HTML ids discoverable (by hover) by adding links to the ids --> above the new XSLT. Precede the comment with 2 blank lines and follow it by one blank line. The idea is to make the code you're adding an identifiable "separate section". I have a question, it is the docbook html stylesheets for XML that are being overridden? It looks like it, and what else would it be, but.... (Sorry, I'm a bit stale on this stuff.) I believe that overriding the XSLT by copying the original and making modifications is the "wrong way" (TM). I think that the right way is to declare a xsl:default-mode somewhere in the stylesheets. There does not seem to be one, so the default mode for PG doc processing could be something like "postgres-mode". And I'd expect it to be declared somewhere right at the top of the xml hierarchy. (I forget what that is, probably something like "document" or "book" or something.) Then, you'd write your for the <xsl:template match="varlistentry"> and <xsl:template name="section.heading"> with a mode of "postgres-mode", and have your templates call the "traditional default", "modeless", templates. That way your not copying and patching upstream code, but are instead, in effect, calling it as a subroutine. This should work especially well since, I think, you're just adding new output to what the upstream templates do. You're not trying to insert new output into the middle of the stock output or otherwise modify the stock output. You're adding only about 6 lines of XSLT to the upstream templates, and copying 100+ lines. There must be a better way. See: https://www.w3.org/TR/xslt-30/#modes I've never tried this, although I do recall doing something or another with modes in the past. And I've not gone so far as to figure out (again?) how to call a "modeless template", so you can invoke the original, upstream, templates. And setting a default mode seems like something of a "big hammer", so should probably be checked over by someone who's more steeped in Postgres docs than myself. (Like a committer. :) But I believe it is the way to go. To make it work you'll need to figure out the XSLT mode selection process and make sure that it first selects the "postgres-mode", and then the modeless templates, and also still works right when a template calls another and explicitly sets a mode. Regards visual presentation: Here's the fun question, what to have "appear" when a section or varlistentry with an id is hovered over? I kind of like your choice of the "#" character as the screen element that becomes visible when you hover over a section or varlistentry with the mouse, which then shows you the URL of the thing over which you are hovering. That's what's in the patch now. But I wonder why you chose it. Is there some sort of standard? I've seen the anchor Unicode character before, (⚓) \u2693. I don't find it particularly helpful. The "place of interest" symbol,(⌘) \u2318, might be nice. There is (◎), \u25ce, the "bullseye" symbol. There is the link symbol (🔗), \U0001f517. Like the anchor, it has generally been confusing when I come across it. The link symbol with the "text variant form", (🔗︎) \U0001f517\ufe0e, looks more link an actual chain and is somewhat better. (The opposite of "text variant" is "emoji variant".) There is also the paperclip, (📎) \U0001f4ce. And the paperclip text variant, (📎︎) \U0001f4ce\ufe0e. Of all the Unicode choices above, I think I prefer the last. The text paperclip. 📎︎ (The hex representations above are Python 3 string literals. So print("\U0001f4ce\ufe0e") prints the text paperclip.) The actual representation of the various Unicode characters is going to depend on the font. (Things change for me when looked at in an email v.s. in a browser.) The question I have is should we use "Unicode icons" at all or is plain old UTF-8 good enough for us (because it was good enough for our ancestors ;) ? Of course an image is also possible.... For now I'm not going to advocate for a change from the "#" already used in the patch. Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
On Sun, 15 Jan 2023 18:01:50 -0600
"Karl O. Pinc" <kop@karlpinc.com> wrote:
> Regards XSLT:
>
> I believe the XSLT needs work.
I also think that the XSLT should error and halt
when there's no id (in the expected places).
Instead of just giving a warning and keeping going. Otherwise
they'll constantly be ignored warnings and periodically
there will have to be patches to supply missing ids.
To solve the "which id is missing where so I can fix it"
problem, I propose the error text show the chapter title,
all the enclosing sub-section titles, and any previous existing
varlistentry ids occurring before the tag with the
missing attribute. At least for varlistentry-s. For
sections you could do chapter and enclosing sub-section
titles and the title of the section with the problem.
That should be enough for an author to find the place
in the source sgml that needs fixing.
Maybe, possibly, you can see how this is done by looking
at whatever XSLT there is that automatically generates
ids for sections without ids, so that the table of contents
have something to link to. In any case, XSLT is really
good at "looking at" parent/enclosing XML, so producing
a useful error message shouldn't be _that_ hard. I've
definitely done this sort of thing before so I can tell you
it's readily doable.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On Sun, 15 Jan 2023 18:01:50 -0600 "Karl O. Pinc" <kop@karlpinc.com> wrote: > Regards XSLT: > > I believe the XSLT needs work. > I believe that overriding the XSLT by copying the original and making > modifications is the "wrong way" (TM). I think that the right way is > to declare a xsl:default-mode somewhere in the stylesheets. There > does not seem to be one, so the default mode for PG doc processing > could be something like "postgres-mode". And I'd expect it to be > declared somewhere right at the top of the xml hierarchy. (I forget > what that is, probably something like "document" or "book" or > something.) Then, you'd write your for the <xsl:template > match="varlistentry"> and <xsl:template name="section.heading"> with > a mode of "postgres-mode", and have your templates call the > "traditional default", "modeless", templates. That way your not > copying and patching upstream code, but are instead, in effect, > calling it as a subroutine. > > This should work especially well since, I think, you're just adding > new output to what the upstream templates do. You're not trying to > insert new output into the middle of the stock output or otherwise > modify the stock output. > > You're adding only about 6 lines of XSLT to the upstream templates, > and copying 100+ lines. There must be a better way. > > See: https://www.w3.org/TR/xslt-30/#modes > > I've never tried this, although I do recall doing something or another > with modes in the past. And I've not gone so far as to figure out > (again?) how to call a "modeless template", so you can invoke the > original, upstream, templates. And setting a default mode seems like > something of a "big hammer", so should probably be checked over by > someone who's more steeped in Postgres docs than myself. (Like a > committer. :) But I believe it is the way to go. To make it work > you'll need to figure out the XSLT mode selection process and make > sure that it first selects the "postgres-mode", and then the modeless > templates, and also still works right when a template calls another > and explicitly sets a mode. Drat. I forgot. We're using xsltproc which is XSLT 1.0. So this is the relevant spec: https://www.w3.org/TR/1999/REC-xslt-19991116#modes In XSLT 1.0 there is no xml:default-mode. So I _think_ what you do then is modify the built-in template rules so that the (default) template (mode='') is invoked when there is no 'postgres-mode' version of the template, but otherwise the 'postgres-mode' version of the template is invoked. Your 'postgres-mode' templates will xsl:call-template the default template, adding whatever they want to the output produced by the default template. See: https://www.w3.org/TR/1999/REC-xslt-19991116#built-in-rule Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
On Mon, 16 Jan 2023 11:14:35 -0600
"Karl O. Pinc" <kop@karlpinc.com> wrote:
> On Sun, 15 Jan 2023 18:01:50 -0600
> "Karl O. Pinc" <kop@karlpinc.com> wrote:
>
> > Regards XSLT:
> >
> > I believe the XSLT needs work.
> In XSLT 1.0 there is no xml:default-mode. So I _think_ what you do
> then is modify the built-in template rules so that the (default)
> template (mode='') is invoked when there is no 'postgres-mode'
> version of the template, but otherwise the 'postgres-mode' version of
> the template is invoked. Your 'postgres-mode' templates will
> xsl:call-template the default template, adding whatever they want to
> the output produced by the default template.
Or maybe the right way is to set a mode at the very top,
the first apply-templates call, and not mess with the
built-in templates at all. (You'd write your own
"postgres-mode" templates the same way, to "wrap"
and call the default templates.)
Think of the mode as an implicit argument that's preserved and
passed down through each template invocation without having to
be explicitly specified by the calling code.
Regards
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On 17.01.2023 at 02:05, Karl O. Pinc wrote: > Or maybe the right way is to set a mode at the very top, > the first apply-templates call, and not mess with the > built-in templates at all. (You'd write your own > "postgres-mode" templates the same way, to "wrap" > and call the default templates.) > > Think of the mode as an implicit argument that's preserved and > passed down through each template invocation without having to > be explicitly specified by the calling code. I think the document you're missing is [1]. There are multiple ways to customize DocBook XSL output and it sounds like you want me to write a customization layer which I didn't do because there is precedent that the typical "way to do it" (TM) in the PostgreSQL project is [2]. Regards, Brar [1] http://www.sagehill.net/docbookxsl/CustomizingPart.html [2] http://www.sagehill.net/docbookxsl/ReplaceTemplate.html
On Tue, 17 Jan 2023 06:57:23 +0100 Brar Piening <brar@gmx.de> wrote: > On 17.01.2023 at 02:05, Karl O. Pinc wrote: > > Or maybe the right way is to set a mode at the very top, > > the first apply-templates call, and not mess with the > > built-in templates at all. (You'd write your own > > "postgres-mode" templates the same way, to "wrap" > > and call the default templates.) > > > > Think of the mode as an implicit argument that's preserved and > > passed down through each template invocation without having to > > be explicitly specified by the calling code. > > I think the document you're missing is [1]. > > There are multiple ways to customize DocBook XSL output and it sounds > like you want me to write a customization layer which I didn't do > because there is precedent that the typical "way to do it" (TM) in the > PostgreSQL project is [2]. > > Regards, > > Brar > > [1] http://www.sagehill.net/docbookxsl/CustomizingPart.html > [2] http://www.sagehill.net/docbookxsl/ReplaceTemplate.html > Sagehill is normally vary good. But in this case [2] does not apply. Or rather it applies but it is overkill because you do not want to replace what a template is producing. You want to add to what a template is producing. So you want to wrap the template, with your new code adding output before/ after what the original produces. [1] does not contain this technique. If you're not willing to try I am willing to see if I can produce an example to work from. My XSLT is starting to come back. Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
On 17.01.2023 at 14:12, Karl O. Pinc wrote: > If you're not willing to try I am willing to see if I can > produce an example to work from. My XSLT is starting to > come back. I'm certainly willing to try but I'd appreciate an example in any case. My XSLT skills are mostly learning by doing combined with trial and error. Regards, Brar
On Tue, 17 Jan 2023 19:13:38 +0100 Brar Piening <brar@gmx.de> wrote: > On 17.01.2023 at 14:12, Karl O. Pinc wrote: > > If you're not willing to try I am willing to see if I can > > produce an example to work from. My XSLT is starting to > > come back. > > I'm certainly willing to try but I'd appreciate an example in any > case. It's good you asked. After poking about the XSLT 1.0 spec to refresh my memory I abandoned the "mode" approach entirely. The real "right way" is to use the import mechanism. I've attached a patch that "wraps" the section.heading template and adds extra stuff to the HTML output generated by the stock template. (example_section_heading_override.patch) There's a bug. All that goes into the html is a comment, not a hoverable link. But the technique is clear. On my system (Debian 11, bullseye) I found the URI to import by looking at: /usr/share/xml/docbook/stylesheet/docbook-xsl/catalog.xml (Which is probably the right place to look.) Ultimately, that file is findable via: /etc/xml/catalog The "best way" on Debian is: /usr/share/doc/docbook-xsl/README.gz In other words, the README that comes with the style sheets. Supposedly, the href=URLs are really URIs and will be good no matter what/when. The XSLT processor should know to look at the system catalogs and read the imported style sheet from the local file system. It might be useful to add --nonet to the xsltproc invocation(s) in the Makefile(s). Just in case; to keep from retrieving stylesheets from the net. (If the option is not already there. I didn't look.) If this is the first time that PG uses the XSLT import mechanism I imagine that "things could go wrong" depending on what sort of system is being used to build the docs. I'm not worried, but it is something to note for the committer. > My XSLT skills are mostly learning by doing combined with trial and > error. I think of XSLT as a functional programming language. Recursion is a big deal, and data directed programming can be a powerful technique because XSLT is so good with data structures. (https://mitp-content-server.mit.edu/books/content/sectbyfn/books_pres_0/6515/sicp.zip/full-text/book/book-Z-H-17.html#%_sec_2.4.3) Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
Attachment
On 17.01.2023 at 23:43, Karl O. Pinc wrote: > It's good you asked. After poking about the XSLT 1.0 spec to refresh > my memory I abandoned the "mode" approach entirely. The real "right > way" is to use the import mechanism. > > I've attached a patch that "wraps" the section.heading template > and adds extra stuff to the HTML output generated by the stock > template. (example_section_heading_override.patch) Thanks! I'll give it a proper look this weekend. Regards, Brar
On Tue, 17 Jan 2023 16:43:13 -0600
"Karl O. Pinc" <kop@karlpinc.com> wrote:
> It might be useful to add --nonet to the xsltproc invocation(s)
> in the Makefile(s). Just in case; to keep from retrieving
> stylesheets from the net. (If the option is not already there.
> I didn't look.)
>
> If this is the first time that PG uses the XSLT import mechanism
> I imagine that "things could go wrong" depending on what sort
> of system is being used to build the docs. I'm not worried,
> but it is something to note for the committer.
Looks like doc/src/sgml/stylesheet-fo.xsl already uses
xsl:import, although it is unclear to me whether the import
is applied.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On 18.01.2023 at 06:50, Brar Piening wrote: > I'll give it a proper look this weekend. It turns out I didn't get a round tuit. ... and I'm afraid I probably will not be able to work on this until mid/end February so we'll have to move this to the next commitfest until somebody wants to take it over and push it through.
Hi, On 2023-01-26 21:48:54 +0100, Brar Piening wrote: > On 18.01.2023 at 06:50, Brar Piening wrote: > > > I'll give it a proper look this weekend. > > It turns out I didn't get a round tuit. > > ... and I'm afraid I probably will not be able to work on this until > mid/end February so we'll have to move this to the next commitfest until > somebody wants to take it over and push it through. A small note: As-is this fails on CI, because we don't allow network access during the docs build anymore (since it always fails these days): https://cirrus-ci.com/task/5474029402849280?logs=docs_build#L297 [17:02:03.114] time make -s -j${BUILD_JOBS} -C doc [17:02:04.092] I/O error : Attempt to load network entity http://cdn.docbook.org/release/xsl/current/html/sections.xsl [17:02:04.092] warning: failed to load external entity "http://cdn.docbook.org/release/xsl/current/html/sections.xsl" [17:02:04.092] compilation error: file stylesheet-html-common.xsl line 17 element import [17:02:04.092] xsl:import : unable to load http://cdn.docbook.org/release/xsl/current/html/sections.xsl I think this is just due to the common URL in docbook packages being http://docbook.sourceforge.net/release/xsl/current/* Because of that the docbook catalog matching logic won't work for that file: E.g. I have the following in /etc/xml/docbook-xsl.xml, on debian unstable: <delegateURI uriStartString="http://docbook.sourceforge.net/release/xsl/" catalog="file:///usr/share/xml/docbook/stylesheet/docbook-xsl/catalog.xml"/> As all our other references use the sourceforge address, this should too. Greetings, Andres Freund
On Tue, 14 Feb 2023 12:13:18 -0800 Andres Freund <andres@anarazel.de> wrote: > A small note: As-is this fails on CI, because we don't allow network > access during the docs build anymore (since it always fails these > days): > > https://cirrus-ci.com/task/5474029402849280?logs=docs_build#L297 > > [17:02:03.114] time make -s -j${BUILD_JOBS} -C doc > [17:02:04.092] I/O error : Attempt to load network entity > http://cdn.docbook.org/release/xsl/current/html/sections.xsl > [17:02:04.092] warning: failed to load external entity > "http://cdn.docbook.org/release/xsl/current/html/sections.xsl" > [17:02:04.092] compilation error: file stylesheet-html-common.xsl > line 17 element import [17:02:04.092] xsl:import : unable to load > http://cdn.docbook.org/release/xsl/current/html/sections.xsl This makes me think that it would be useful to add --nonet to the xsltproc invocations. That would catch this error before it goes to CI. > I think this is just due to the common URL in docbook packages being > http://docbook.sourceforge.net/release/xsl/current/* > Because of that the docbook catalog matching logic won't work for > that file: > > E.g. I have the following in /etc/xml/docbook-xsl.xml, on debian > unstable: <delegateURI > uriStartString="http://docbook.sourceforge.net/release/xsl/" > catalog="file:///usr/share/xml/docbook/stylesheet/docbook-xsl/catalog.xml"/> > > As all our other references use the sourceforge address, this should > too. Agreed. I'm also noticing that the existing xsl:import-s all import entire docbook stylesheets. It does not hurt to do this; the output is unaffected, although I can't say what it means for build performance. It does keep it simple. Only one import is needed no matter which templates we use the import mechanism to extend. And by importing "everything" there's no concern about any (unlikely) changes to the the "internals" of the catalog. Should we import only what we need or all of docbook? I don't know. Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
Hi,
On 2023-02-15 13:34:37 -0600, Karl O. Pinc wrote:
> This makes me think that it would be useful to add --nonet to the
> xsltproc invocations. That would catch this error before it goes to
> CI.
We are doing that now :)
commit 969509c3f2e3b4c32dcf264f9d642b5ef01319f3
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: 2023-02-08 17:15:23 -0500
Stop recommending auto-download of DTD files, and indeed disable it.
> I'm also noticing that the existing xsl:import-s all import entire
> docbook stylesheets. It does not hurt to do this; the output is
> unaffected, although I can't say what it means for build performance.
> It does keep it simple. Only one import is needed no matter which
> templates we use the import mechanism to extend. And by importing
> "everything" there's no concern about any (unlikely) changes to
> the the "internals" of the catalog.
>
> Should we import only what we need or all of docbook? I don't know.
It couldn't hurt to check if performance improves when you avoid doing so. I
suspect it won't make much of a difference, because the time is actually spent
evaluating xslt rather than parsing it.
Greetings,
Andres Freund
Re: doc: add missing "id" attributes to extension packaging page
From
"Gregory Stark (as CFM)"
Date:
On Thu, 26 Jan 2023 at 15:55, Brar Piening <brar@gmx.de> wrote: > > On 18.01.2023 at 06:50, Brar Piening wrote: > > > I'll give it a proper look this weekend. > > It turns out I didn't get a round tuit. > > ... and I'm afraid I probably will not be able to work on this until > mid/end February so we'll have to move this to the next commitfest until > somebody wants to take it over and push it through. Looks like a lot of good work was happening on this patch right up until mid-February. Is there a lot of work left? Do you think you'll have a chance to wrap it up this commitfest for this release? -- Gregory Stark As Commitfest Manager
On 20.03.2023 at 19:47, Gregory Stark (as CFM) wrote: > Looks like a lot of good work was happening on this patch right up > until mid-February. Is there a lot of work left? Do you think you'll > have a chance to wrap it up this commitfest for this release? Thanks for the ping. I had another look this morning and I think I can probably finish this by the end of the week.
On 17.01.2023 at 23:43, Karl O. Pinc wrote: > It's good you asked. After poking about the XSLT 1.0 spec to refresh > my memory I abandoned the "mode" approach entirely. The real "right > way" is to use the import mechanism. It actually is not. After quite some time trying to figure out why things don't work as intended, I ended up reading the XSLT 1.0 spec. As the name already suggests, <xsl:apply-imports> is closely related to <xsl:apply-templates> with the difference that it *applies* a *template rule* from an imported style sheet instead of applying a template rule from the current style sheet (https://www.w3.org/TR/1999/REC-xslt-19991116#apply-imports). What it does not do is *calling* a *named template* (https://www.w3.org/TR/1999/REC-xslt-19991116#named-templates). What this basically means is that in XSLT 1.0 you can use <xsl:apply-imports> to override template rules (<xsl:template match="this-pattern-inside-match-makes-it-a-template-rule">) but you cannot use it to override named templates (<xsl:template name="this-id-inside-name-makes-it-a-named-template">). If you want to override named templates you basically have to duplicate and change them. While there are mechanisms to call overriden named templates in XSLT 3, this is out of scope here, since we're bound to XSLT 1.0 As a consequence, there was little I could change in the initial patch to avoid the code duplication and all attempts to do so, ultimately led to even longer and more complex code without really reducing the amount of duplication. The <xsl:apply-imports> approach actually does work in the varlistentry case, although this doesn't really change a lot regarding the length of the patch (it's a bit nicer though since in this case it really avoids duplication). I've also taken the advice to terminate the build and print the xpath if a required id is missing. The attached patch is my best-effort approach to implement discoverable links. Best regards, Brar
Attachment
On Tue, 21 Mar 2023 23:16:25 +0100 Brar Piening <brar@gmx.de> wrote: > On 17.01.2023 at 23:43, Karl O. Pinc wrote: > > It's good you asked. After poking about the XSLT 1.0 spec to > > refresh my memory I abandoned the "mode" approach entirely. The > > real "right way" is to use the import mechanism. > After quite some time trying to figure out why things don't work as > intended, I ended up reading the XSLT 1.0 spec. > > As the name already suggests, <xsl:apply-imports> is closely related > to <xsl:apply-templates> with the difference that it *applies* a > *template rule* from an imported style sheet instead of applying a > template rule from the current style sheet > (https://www.w3.org/TR/1999/REC-xslt-19991116#apply-imports). What it > does not do is *calling* a *named template* > (https://www.w3.org/TR/1999/REC-xslt-19991116#named-templates). > > What this basically means is that in XSLT 1.0 you can use > <xsl:apply-imports> to override template rules (<xsl:template > match="this-pattern-inside-match-makes-it-a-template-rule">) but you > cannot use it to override named templates (<xsl:template > name="this-id-inside-name-makes-it-a-named-template">). If you want to > override named templates you basically have to duplicate and change > them. > > While there are mechanisms to call overriden named templates in XSLT > 3, this is out of scope here, since we're bound to XSLT 1.0 (It was reassuring to see you take the steps above; I once did exactly the same with and had the same excitements and disappointments. I feel validated. ;-) (One of my disappointments is that xsltproc supports only XSLT 1.0, and nothing later. IIRC, apparently one big reason is not the amount work needed to develop the program but the work required to develop a test suite to validate conformance.) > As a consequence, there was little I could change in the initial patch > to avoid the code duplication and all attempts to do so, ultimately > led to even longer and more complex code without really reducing the > amount of duplication. You're quite right. I clearly didn't have my XSLT turned on. Importing only works when templates are matched, not called by name. Sorry for the extra work I've put you through. > The <xsl:apply-imports> approach actually does work in the > varlistentry case, although this doesn't really change a lot > regarding the length of the patch (it's a bit nicer though since in > this case it really avoids duplication). You've put in a lot of good work. I'm attaching 2 patches with only minor changes. 001-add-needed-ids_v1.patch This separates out the addition of ids from the XSLT changes, just to keep things tidy. Content is from your patch. 002-make_html_ids_discoverable_v4.patch I changed the linked text, the #, so that the leading space is not linked. This is arguable, as the extra space makes it easier to put the mouse on the region. But it seems tidy. I've tided up so the lines are no longer than 80 chars. > I've also taken the advice > to terminate the build and print the xpath if a required id is > missing. This looks awesome. I love the xpath! I've changed the format of the error message. What do you think? (Try it out by _not_ applying 001-add-needed-ids_v1.patch.) Also, the error message now has leading and trailing newlines to make it stand out. I'm normally against this sort of thing but thought I'd add it anyway for others to review. I'm ready to send these on to a committer but if you don't like what I did please send more patches for me to review. Outstanding questions (for committer?): The 002-make_html_ids_discoverable_v4.patch generates xhtml <h1>, <h2>, etc. attributes using a XSLT <element> element with a "namespace" attribute. I'm unclear on the relationship PG has with xhtml and namespaces. Looks right to me, since the generated html has the same namespace name appearing in the xmlns attribute of the html tag, but somebody who knows more than me might want to review this. Using the namespace attribute does not seem to have affected the generated html, as far as my random sampling of output can tell. What character should be used to represent a link anchor? I've left your choice of "#" in the patch. If we go to unicode, My preference is the text paperclip 📎︎ Here's a table of all the choices I came up with, there may be others that are suitable. (The hex representations are Python 3 string literals. So print("\U0001f4ce\ufe0e") prints the text paperclip.) Hash mark # (ASCII, used in the patch, \u0023) Anchor ⚓ \u2693 Place of interest ⌘ \u2318 Bullseye ◎ \u25ce Link (emoji variant) 🔗 \U0001f517 Link (text variant) 🔗︎ \U0001f517\ufe0e Paperclip (emoji variant) 📎 \U0001f4ce Paperclip (text variant) 📎︎ \U0001f4ce\ufe0e Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
Attachment
On 23.03.2023 at 04:09, Karl O. Pinc wrote: > You're quite right. I clearly didn't have my XSLT turned on. Importing > only works when templates are matched, not called by name. > > Sorry for the extra work I've put you through. No problem. As always I've learnt something which may help me in the future. > You've put in a lot of good work. I'm attaching 2 patches > with only minor changes. Thanks. When comparing things I also realized that I had accidentally created a reversed patch. Thanks for fixing this. > 001-add-needed-ids_v1.patch > > This separates out the addition of ids from the XSLT changes, just > to keep things tidy. Content is from your patch. +1 > 002-make_html_ids_discoverable_v4.patch > > I changed the linked text, the #, so that the leading space > is not linked. This is arguable, as the extra space makes > it easier to put the mouse on the region. But it seems > tidy. I tend to prefer a slightly bigger mouseover-region but I don't really mind. > I've tided up so the lines are no longer than 80 chars. +1 > This looks awesome. I love the xpath! I've changed the format of the > error message. What do you think? (Try it out by _not_ applying > 001-add-needed-ids_v1.patch.) > > Also, the error message now has leading and trailing newlines to make > it stand out. I'm normally against this sort of thing but thought I'd > add it anyway for others to review. +1 > I'm ready to send these on to a committer but if you don't > like what I did please send more patches for me to review. I like it and think it's ready for commiter. > Outstanding questions (for committer?): > > The 002-make_html_ids_discoverable_v4.patch generates xhtml <h1>, > <h2>, etc. attributes using a XSLT <element> element with a > "namespace" attribute. I'm not sure I follow. I cannot see any namespacing weirdness in my output. Are you using the v1.79.2 styleshhets? > What character should be used to represent a link anchor? It's not the first time this is coming up. See my response in the old thread: https://www.postgresql.org/message-id/e50193ea-ca5c-e178-026a-f3fd8942252d%40gmx.de Personally I'd advise to stick with ASCII for now. In any case changing the symbol at some point would be a very minor effort if we deem it necessary. Maybe this could be part of some general overhaul of the visual apperance and website styling by a person with more talent for this than I have. Regards, Brar
Thanks, Brar and Karl, I hope we can get this done soon. As with the <simplelist> patch, we'll need to patch the CSS used in the website for the docs too, as that's the most important place where docs are visited. See this commit for an example: https://git.postgresql.org/gitweb/?p=pgweb.git;a=commitdiff;h=0b89ea0fff28d29ed177c82a274144453e3c7f82 In order to test locally that your patched stylesheet works correctly, you'd have to compile the docs with "make html STYLE=website" in the doc subdir, and tweak one of the CSS files there (I think it's docs-complete.css) so that it references your local copy instead of fetching it from the website. > diff --git a/doc/src/sgml/stylesheet.css b/doc/src/sgml/stylesheet.css > index cc14efa1ca..15bcc95d41 100644 > --- a/doc/src/sgml/stylesheet.css > +++ b/doc/src/sgml/stylesheet.css > @@ -169,3 +169,13 @@ acronym { font-style: inherit; } > width: 75%; > } > } > + > +/* Links to ids of headers and definition terms */ > +a.id_link { > + color: inherit; > + visibility: hidden; > +} > + > +*:hover > a.id_link { > + visibility: visible; > +} I'm not clear on what exactly becomes visible when one hovers over what. Can you please share a screenshot? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On 23.03.2023 at 10:35, Alvaro Herrera wrote: > As with the <simplelist> patch, we'll need to patch the CSS used in the > website for the docs too, as that's the most important place where docs > are visited. See this commit for an example: > https://git.postgresql.org/gitweb/?p=pgweb.git;a=commitdiff;h=0b89ea0fff28d29ed177c82a274144453e3c7f82 > > In order to test locally that your patched stylesheet works correctly, > you'd have to compile the docs with "make html STYLE=website" in the doc > subdir, and tweak one of the CSS files there (I think it's > docs-complete.css) so that it references your local copy instead of > fetching it from the website. Thanks I'll take care of this tonight. > I'm not clear on what exactly becomes visible when one hovers over what. > Can you please share a screenshot? I could, but since hover effects don't really come across in screenshots I've posted a build of the docs including the patch to https://pgdocs.piening.info See https://pgdocs.piening.info/app-psql.html as an example.
On Thu, 23 Mar 2023 08:24:48 +0100
Brar Piening <brar@gmx.de> wrote:
> On 23.03.2023 at 04:09, Karl O. Pinc wrote:
> > Sorry for the extra work I've put you through.
>
> No problem. As always I've learnt something which may help me in the
> future.
I don't know about you, but sadly, my brain eventually leaks. ;-)
> > I'm attaching 2 patches
> > with only minor changes.
> > 001-add-needed-ids_v1.patch
> >
> > This separates out the addition of ids from the XSLT changes, just
> > to keep things tidy.
> > 002-make_html_ids_discoverable_v4.patch
> >
> > I changed the linked text, the #, so that the leading space
> > is not linked. This is arguable, as the extra space makes
> > it easier to put the mouse on the region.
> I tend to prefer a slightly bigger mouseover-region but I don't
> really mind.
I'm leaving it for the committer to review.
> I've changed the format of
> > the error message. What do you think? (Try it out by _not_
> > applying 001-add-needed-ids_v1.patch.)
> >
> > Also, the error message now has leading and trailing newlines to
> > make it stand out.
Including the error message/make output here, so everyone can see
easily.
--------------<snip>------------
/usr/bin/xsltproc --nonet --path . --stringparam pg.version '16devel' stylesheet.xsl postgres-full.xml
Ids are required in order to provide the public HTML documentation with stable URLs for <varlistentry> element content;
idmissing at: /book[@id = 'postgres']/part[@id = 'appendixes']/appendix[@id = 'contrib']/sect1[@id =
'pgwalinspect']/sect2[@id= 'pgwalinspect-funcs']/variablelist
no result for postgres-full.xml
make: *** [Makefile:146: html-stamp] Error 10
--------------<snip>------------
> I like it and think it's ready for commiter.
I've marked it ready for the committer in the commitfest.
> > Outstanding questions (for committer?):
> >
> > The 002-make_html_ids_discoverable_v4.patch generates xhtml <h1>,
> > <h2>, etc. attributes using a XSLT <element> element with a
> > "namespace" attribute.
>
> I'm not sure I follow. I cannot see any namespacing weirdness in my
> output.
There's nothing weird in the output, it's all about how
you're generating it in the xslt with
<xsl:element name="h$level" namespace="...
Output looks right to me.
> Are you using the v1.79.2 styleshhets?
Yes. But I've got both the ones with namespaces and without
installed.
I've just never had to look at what PG is doing with namespaces
before. What you've done looks right to me, but I'm pretty
clueless so somebody else should double check.]
> > What character should be used to represent a link anchor?
> Personally I'd advise to stick with ASCII for now.
+1
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On Thu, 23 Mar 2023 10:35:55 +0100 Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > As with the <simplelist> patch, we'll need to patch the CSS used in > the website for the docs too, as that's the most important place > where docs are visited. See this commit for an example: > https://git.postgresql.org/gitweb/?p=pgweb.git;a=commitdiff;h=0b89ea0fff28d29ed177c82a274144453e3c7f82 > > In order to test locally that your patched stylesheet works correctly, > you'd have to compile the docs with "make html STYLE=website" in the > doc subdir, and tweak one of the CSS files there (I think it's > docs-complete.css) so that it references your local copy instead of > fetching it from the website. I should have known to put the css in a separate patch. > I'm not clear on what exactly becomes visible when one hovers over > what. Can you please share a screenshot? Attached are 2 screenshots. I don't know why, but for some reason you can't see the mouse pointer. "#' shows up when the mouse is anywhere over the html heading element. Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
Attachment
On 23.03.2023 at 10:35, Alvaro Herrera wrote: > As with the <simplelist> patch, we'll need to patch the CSS used in the > website for the docs too, as that's the most important place where docs > are visited. Ok, I've created and tested a patch for this too. Since the need for ids is starting to grow again (ecb696527c added an id to a varlistentry in doc/src/sgml/ref/create_subscription.sgml) I've also amended the add-needed-ids patch once again so that the build does not fail after applying the make_html_ids_discoverable patch. I've also attached the (unchanged) make_html_ids_discoverable patch for convenience so this email now contains two patches for postgresql (ending with .postgresql.patch) and one patch for pgweb (ending with .pgweb.patch). TBH I'm a bit afraid that people will immediately start complaining about the failing docs builds after this got applied since it forces them to add ids to all varlistenries in a variablelist if they add one, which can be perceived as quite a burden (also committers and reviewers will have to get used to always watch out for failing docs builds because of this). Since breaking the build on missing ids was an intentional decision we can theoretically soften this by only issuing a warning or removing the check for missing id's altogether but this would probably defeat the purpose since it would lead to an increasing number of entries that lack an id after a while. Regards, Brar
Attachment
TBH I'm a bit afraid that people will immediately start complaining
about the failing docs builds after this got applied since it forces
them to add ids to all varlistenries in a variablelist if they add one,
which can be perceived as quite a burden (also committers and reviewers
will have to get used to always watch out for failing docs builds
because of this).
As a person who had to re-rebase a patch because I discovered that id tags had been added to one of the files I was working on, I can confidently say "don't worry". It wasn't that big of a deal, I wasn't even following this thread at the time and I immediately figured out what had happened and what was expected of me. So it isn't that much of an inconvenience. If there is a negative consequence to this change, it would be that it might incentivize patch writers to omit documentation completely at the early stages. But I don't think that's a problem because people generally see a lack of documentation as a clue that maybe the patch isn't ready to be reviewed, and this change would only reinforce that litmus test.
I had suggested we do something like this a few years back (the ids, that is. the idea that we could check for compliance was beyond my imagination at the time), and I'm glad to see both finally happening.
While I can foresee people overlooking the docs build, such oversights won't go long before being caught, and the fix is simple. Now if we can just get a threaded version of xlstproc to make the builds faster...
p.s. I'm "Team Paperclip" when it comes to the link hint, but let's get the feature in first and worry about the right character later.
Hi Brar,
It occurs to me that I had not actually tested the
way the anchor is put only after the last term in a
varlistentry. (The code looked obviously right
and should work if any varlistentry terms have anchors,
which they do, but....)
Have you tested this? If not, catalog.sgml, the
DEPENDENCY_PARTITION_SEC term is a 2nd term and usable
for at test case. But, at the moment there are no ids
for any of these varlistentries so you'd have to hack
them in.
Apologies for missing this.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
This is for the committer, as an FYI.
I cut out the <xsl:template name="section.heading"> portion
of the docbook XSLT and diffed it with the code for the
same template in the patch. The diff looks like:
-- /tmp/sections.xsl 2023-03-22 13:00:33.432968357 -0500
+++ /tmp/make_html_ids_discoverable_v3.patch 2023-03-22 13:03:39.776930603 -0500
@@ -52,5 +52,8 @@
</xsl:call-template>
</xsl:if>
<xsl:copy-of select="$title"/>
+ <xsl:call-template name="pg.id.link">
+ <xsl:with-param name="object" select="$section"/>
+ </xsl:call-template>
</xsl:element>
</xsl:template>
(So, this output would start with line 52 of the template,
not from the top of the stock sections.xsl file.)
However, I am not really familiar with exactly what flavor
of docbook, version, namespace-d or not, etc., that PG
uses. So I could be diffing with the wrong thing.
Hope this helps and is not just noise.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
Hi Brar,
An observation: The # that shows up when hovering
over section-level headings is styled as the
section-level heading is. But the # that shows
up when hovering over varlistentrys has the default
text style.
This works for me. It's nice to have the "section #"s
look like the section heading. But the varlistentry's
terms are smaller than the normal font, and their
line width is less heavy than normal. I'm not really
invested one way or the other, but I find it kind of
nice that the varlistentry's #s are easier to click
on and more noticable because they're slightly larger
than might be expected.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On 23.03.2023 at 23:31, Karl O. Pinc wrote: > Hi Brar, > > It occurs to me that I had not actually tested the > way the anchor is put only after the last term in a > varlistentry. (The code looked obviously right > and should work if any varlistentry terms have anchors, > which they do, but....) > > Have you tested this? Yes, I have. See https://pgdocs.piening.info/app-psql.html#APP-PSQL-META-COMMAND-DE for an extreme case.
On 24.03.2023 at 05:09, Karl O. Pinc wrote: > Hi Brar, > > An observation: The # that shows up when hovering > over section-level headings is styled as the > section-level heading is. But the # that shows > up when hovering over varlistentrys has the default > text style. > > This works for me. It's nice to have the "section #"s > look like the section heading. But the varlistentry's > terms are smaller than the normal font, and their > line width is less heavy than normal. I'm not really > invested one way or the other, but I find it kind of > nice that the varlistentry's #s are easier to click > on and more noticable because they're slightly larger > than might be expected. TBH I didn't bother a lot with this. Most of the time it's actually not the font size but rather the font-family which gets inherited from the parent element if you don't set it explicitly. The link just inherits everithing (including the color, which I have set to inherit explicitly since links don't inherit the parent's color by default) from it's parent, which is the HTML <dt> element (ultimately the inheritance probably goes up to the <body> element style in pretty much all cases). In some instances the input <term> element contains elements that are styled differently in the output (e.g.: <literal> which translates to HTML <code> which has "font-family: monospace;") which makes the # from the link appear differently than the visible element it appears after. Since (after tweaking the color) the general visual appearence looked ok to me, I didn't bother with this any further. Regards, Brar
On 24.03.2023 at 06:48, Brar Piening wrote: > > Since (after tweaking the color) the general visual appearence looked > ok to me, I didn't bother with this any further. Also, if we go on with this we'll probably end up in an almost prototypical bikeshedding scenario where PostgreSQL itself is the nuclear power plant and the visual appearence of the hover links on the documentation website is the color of the bikeshed. ;-)
On 2023-Mar-24, Brar Piening wrote: > On 23.03.2023 at 23:31, Karl O. Pinc wrote: > > Hi Brar, > > > > It occurs to me that I had not actually tested the > > way the anchor is put only after the last term in a > > varlistentry. (The code looked obviously right > > and should work if any varlistentry terms have anchors, > > which they do, but....) > > > > Have you tested this? > > Yes, I have. See > https://pgdocs.piening.info/app-psql.html#APP-PSQL-META-COMMAND-DE for > an extreme case. But why are there no anchors next to <h3> items on that page? For example, how do I get the link for the "Meta Commands" subsection? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ <inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell <crab> inflex: you know that "amalgam" means "mixture with mercury", more or less, right? <crab> i.e., "deadly poison"
> But why are there no anchors next to
> <h3> items on that page? For example,
> how do I get the link for the
> "Meta Commands" subsection?
I can't look at the code right now, but I suspect the headers are refsections (not sections) which this patch does not add links for yet.
I already have plans to add this in a follow-up patch at some point, but while I had already added ids to all section elements in the previous patch that added ids, this has yet to be done for all refsect elements (wich is not a small effort again).
Regards,
Brar
On 2023-Mar-24, brar wrote: > Alvaro wrote: > > But why are there no anchors next to <h3> items on that page? For > > example, how do I get the link for the "Meta Commands" subsection? > I can't look at the code right now, but I suspect the headers are > refsections (not sections) which this patch does not add links for > yet. I already have plans to add this in a follow-up patch at some > point, but while I had already added ids to all section elements in > the previous patch that added ids, this has yet to be done for all > refsect elements (wich is not a small effort again). You are right, those are <refsect2>. Understood, thanks. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ <inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell <crab> inflex: you know that "amalgam" means "mixture with mercury", more or less, right? <crab> i.e., "deadly poison"
On 24.03.2023 at 10:45, Alvaro Herrera wrote: > But why are there no anchors next to <h3> items on that page? For > example, how do I get the link for the "Meta Commands" subsection? I somehow knew that responding from a crappy mobile phone e-mail client will mess up the format and the thread... For those trying to follow the thread in the archives: my response (it's probably a refsect which isn't supported yet) ended up here: https://www.postgresql.org/message-id/1N1fn0-1qd4xd1MyG-011ype%40mail.gmx.net Regards, Brar
On 23.03.2023 at 20:08, Brar Piening wrote: > Since the need for ids is starting to grow again (ecb696527c added an > id to a varlistentry in doc/src/sgml/ref/create_subscription.sgml) > I've also amended the add-needed-ids patch once again so that the > build does not fail after applying the make_html_ids_discoverable patch. New add-needed-ids patch since it was outdated again. Regards, Brar
Attachment
On Tue, Mar 28, 2023 at 4:06 AM Brar Piening <brar@gmx.de> wrote: > > On 23.03.2023 at 20:08, Brar Piening wrote: > > Since the need for ids is starting to grow again (ecb696527c added an > > id to a varlistentry in doc/src/sgml/ref/create_subscription.sgml) > > I've also amended the add-needed-ids patch once again so that the > > build does not fail after applying the make_html_ids_discoverable patch. > > New add-needed-ids patch since it was outdated again. > FYI, there is a lot of overlap between this last attachment and the patches of Kuroda-san's current thread here [1] which is also adding ids to create_subscription.sgml. (Anyway, I guess you might already be aware of that other thread because your new ids look like they have the same names as those chosen by Kuroda-san) ------ [1] https://www.postgresql.org/message-id/flat/CAHut%2BPvzo6%3DKKLqMR6-mAQdM%2Bj_dse0eUreGmrFouL7gbLbv2w%40mail.gmail.com#7da8d0e3b73096375847c16c856b4aed Kind Regards, Peter Smith. Fujitsu Australia
On 28.03.2023 at 00:11, Peter Smith wrote: > FYI, there is a lot of overlap between this last attachment and the > patches of Kuroda-san's current thread here [1] which is also adding > ids to create_subscription.sgml. > > (Anyway, I guess you might already be aware of that other thread > because your new ids look like they have the same names as those > chosen by Kuroda-san) > > ------ > [1] https://www.postgresql.org/message-id/flat/CAHut%2BPvzo6%3DKKLqMR6-mAQdM%2Bj_dse0eUreGmrFouL7gbLbv2w%40mail.gmail.com#7da8d0e3b73096375847c16c856b4aed Thanks, I actually was not aware of this. Also, kudos for capturing the missing id and advocating for consistency regarding ids even before this is actively enforced. This nurtures my optimism that consistency might actually be achieveable without everybody getting angry at me because my patch will enforce it. Regarding the overlap, I currently try to make it as easy as possible for a potential committer and I'm happy to rebase my patch upon request or if Kuroda-san's patch gets applied first. Regards, Brar
RE: doc: add missing "id" attributes to extension packaging page
From
"Hayato Kuroda (Fujitsu)"
Date:
Dear my comrade Brar, > Thanks, I actually was not aware of this. > > Also, kudos for capturing the missing id and advocating for consistency > regarding ids even before this is actively enforced. This nurtures my > optimism that consistency might actually be achieveable without > everybody getting angry at me because my patch will enforce it. > > Regarding the overlap, I currently try to make it as easy as possible > for a potential committer and I'm happy to rebase my patch upon request > or if Kuroda-san's patch gets applied first. FYI - my patch is pushed [1]. Could you please rebase your patch? I think it's ok to just remove changes from logical-replication.sgml, ref/alter_subscription.sgml, and ref/create_subscription.sgml. [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=de5a47af2d8003dee123815bb7e58913be9a03f3 Best Regards, Hayato Kuroda FUJITSU LIMITED
On 29.03.2023 at 06:52, Hayato Kuroda (Fujitsu) wrote: > FYI - my patch is pushed Thanks! > Could you please rebase your patch? Done and tested. Patch is attached. Regards, Brar
Attachment
RE: doc: add missing "id" attributes to extension packaging page
From
"Hayato Kuroda (Fujitsu)"
Date:
Dear Brar, Thank you for updating the patch. The patch looks good to me. Best Regards, Hayato Kuroda FUJITSU LIMITED
On 29.03.23 18:03, Brar Piening wrote: > On 29.03.2023 at 06:52, Hayato Kuroda (Fujitsu) wrote: >> FYI - my patch is pushed > > Thanks! > > >> Could you please rebase your patch? > > Done and tested. Patch is attached. I have committed the most recent patch that added some missing IDs. (I also added a missing xreflabel in passing.) I'll look at the XSLT patch next.
On 23.03.23 20:08, Brar Piening wrote: > I've also attached the (unchanged) make_html_ids_discoverable patch for > convenience so this email now contains two patches for postgresql > (ending with .postgresql.patch) and one patch for pgweb (ending with > .pgweb.patch). Here is my view on this: First of all, it works very nicely and is very useful. Very welcome. The XSLT implementation looks sound to me. It would be a touch better if it had some comments about which parts of the templates were copied from upstream stylesheets and which were changed. There are examples of such commenting in the existing customization layer. Also, avoid introducing whitespace differences during said copying. However, I wonder if this is the right way to approach this. I don't think we should put these link markers directly into the HTML. It feels like this is the wrong layer. For example, if you have CSS turned off, then all these # marks show up by default. It seems to me that the correct way to do this is to hook in some JavaScript that does this transformation directly on the DOM. Then we don't need to carry this presentation detail in the HTML. Moreover, it would avoid tight coupling between the website and the documentation sources. You can produce the exact same DOM, that part seems okay, just do it elsewhere. Was this approach considered? I didn't see it in the thread.
On 04.04.2023 at 16:54, Peter Eisentraut wrote: > > First of all, it works very nicely and is very useful. Very welcome. Thank you! > The XSLT implementation looks sound to me. It would be a touch better > if it had some comments about which parts of the templates were copied > from upstream stylesheets and which were changed. There are examples > of such commenting in the existing customization layer. Also, avoid > introducing whitespace differences during said copying. I will amend the patch if we agree that this is the way forward. > However, I wonder if this is the right way to approach this. I don't > think we should put these link markers directly into the HTML. It > feels like this is the wrong layer. For example, if you have CSS > turned off, then all these # marks show up by default. I'd consider this a feature rather than a problem but this is certainly debatable. I cannot reliably predict what expectations a user who is browsing the docs with CSS disabled might have. The opposite is true too. If we'd move the id links feature to javascript, a user who has javascript disabled will not see them. Is this what they'd want? I don't know. Also, while about 1-2% of users have Javascript disabled, I haven't heard of disabling CSS except for debugging purposes. In general I'd consider the fact that CSS or Javascript might be disabled a niche problem that isn't really worth much debating but there is definitely something to consider regarding people using screen readers who might suffer from one or the other behavior and I'd definitely be interested what behavior these users would expect. Would they want to use the id link feature or would the links rather disrupt their reading experience - I have no idea TBH and I hate speculating about other people's preferences. > It seems to me that the correct way to do this is to hook in some > JavaScript that does this transformation directly on the DOM. Then we > don't need to carry this presentation detail in the HTML. Moreover, it > would avoid tight coupling between the website and the documentation > sources. You can produce the exact same DOM, that part seems okay, > just do it elsewhere. Was this approach considered? I didn't see it > in the thread. I briefly touched the topic in [1] and [2] but we didin't really follow up on the best approach. Regards, Brar [1] https://www.postgresql.org/message-id/68b9c435-d017-93cc-775a-c604db9ec683%40gmx.de [2] https://www.postgresql.org/message-id/a75b6d7c-3fa4-d6a8-cf23-6b5180237392%40gmx.de
On Tue, 4 Apr 2023 21:52:31 +0200
Brar Piening <brar@gmx.de> wrote:
> On 04.04.2023 at 16:54, Peter Eisentraut wrote:
> > The XSLT implementation looks sound to me. It would be a touch
> > better if it had some comments about which parts of the templates
> > were copied from upstream stylesheets and which were changed.
I like this idea. A lot. (Including which stylesheets were copied
from.)
> > However, I wonder if this is the right way to approach this. I
> > don't think we should put these link markers directly into the
> > HTML. It feels like this is the wrong layer. For example, if you
> > have CSS turned off, then all these # marks show up by default.
>
> I'd consider this a feature rather than a problem but this is
> certainly debatable.
I too would consider this a feature. If you don't style your
html presentation, you see everything. The "#" links to content
are, well, content.
> > It seems to me that the correct way to do this is to hook in some
> > JavaScript that does this transformation directly on the DOM. Then
> > we don't need to carry this presentation detail in the HTML.
I would argue the opposite. The HTML/CSS is delivered to the browser
which is then free to present the content to the user in the
form preferred by the user. This puts control of presentation
in the hands of the end-user, where, IMO, it belongs.
Using JavaScript to manipulate the DOM is all well and good when
using AJAX to interact with the server to produce dynamic content.
But otherwise manipulating the DOM with JavaScript seems overly
heavy-handed, even though popular. It seems like JavaScript is
used more because CSS is difficult and an "extra technology" when
instead JavaScript can "do everything". So CSS is put aside.
I may be biased, not being a JavaScript fan. (I tend to be one
of those cranky individuals who keep JavaScript turned off.)
I'd rather not have code executing when such overhead/complication
can be avoided. (Insert here exciting argument about "what is code
and what is data".)
Glancing at the documentation source, I don't see JavaScript used
at all. Introducing it would be adding something else to the mix.
Not that this would be bad if it provides value.
In the end, I don't _really_ care. And don't do web development
all day either so my fundamentals could be just wrong. But
this is my take.
> > Moreover, it would avoid tight coupling between the website and the
> > documentation sources.
I don't really understand this statement. Are you saying that
the documentation's source CSS needn't/shouldn't be the CSS used
on the website? That seems counter-intuitive. But then I don't
understand why the default CSS used when developing the documentation
is not the CSS used on the website.
I can imagine administrative arguments around server maintenance for
wanting to keep the website decoupled from the PG source code.
(I think.) But I can't speak to any of that.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
For what it's worth, I think having the anchors be always-visible when CSS disabled is a feature. The content is still perfectly readable, and the core feature from this patch is available. Introducing JavaScript to lose that functionality seems like a step backwards. By the way, the latest patch attachment was not the full patch series, which I think confused cfbot: [1] (unless I'm misunderstanding the state of the patch series). And thanks for working on this. I've hunted in the page source for ids to link to a number of times. I look forward to not doing that anymore. Thanks, Maciek [1]: https://commitfest.postgresql.org/42/4042/
On 04.04.23 21:52, Brar Piening wrote: >> The XSLT implementation looks sound to me. It would be a touch better >> if it had some comments about which parts of the templates were copied >> from upstream stylesheets and which were changed. There are examples >> of such commenting in the existing customization layer. Also, avoid >> introducing whitespace differences during said copying. > > I will amend the patch if we agree that this is the way forward. Ok, it appears everyone agrees that this is the correct approach. Please post an updated patch. There have been so many partial patches posted recently, I'm not sure which one is the most current one and who is really the author.
On 06.04.2023 at 11:06, Peter Eisentraut wrote: > On 04.04.23 21:52, Brar Piening wrote: >>> The XSLT implementation looks sound to me. It would be a touch better >>> if it had some comments about which parts of the templates were copied >>> from upstream stylesheets and which were changed. There are examples >>> of such commenting in the existing customization layer. Also, avoid >>> introducing whitespace differences during said copying. >> >> I will amend the patch if we agree that this is the way forward. > > Ok, it appears everyone agrees that this is the correct approach. > Please post an updated patch. There have been so many partial patches > posted recently, I'm not sure which one is the most current one and > who is really the author. > Attached are two patches: 001-make_html_ids_discoverable_v5.postgresql.patch which needs to be applied to the postgresql repository. It adds the XSLT to generate the id links and the CSS to hide/display them. I've added comments as suggested above. 002-add-discoverable-id-style_v1.pgweb.patch which needs to be applied to the pgweb repository. It adds the CSS to the offical documentation site. At the moment (commit 983ec23007) there are no missing ids, so the build should just work after applying the patch but, as we already know, this may change with every commit that gets added. Reviewer is Karl O. Pink Author is Brar Piening (with some additions from Karl O. Pink) Best regards, Brar
Attachment
On Thu, 6 Apr 2023 16:19:30 +0200
Brar Piening <brar@gmx.de> wrote:
> Reviewer is Karl O. Pink
"Karl O. Pinc" actually, with a "c".
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On 06.04.23 16:19, Brar Piening wrote: > 001-make_html_ids_discoverable_v5.postgresql.patch which needs to be > applied to the postgresql repository. It adds the XSLT to generate the > id links and the CSS to hide/display them. I've added comments as > suggested above. > > 002-add-discoverable-id-style_v1.pgweb.patch which needs to be applied > to the pgweb repository. It adds the CSS to the offical documentation site. The first patch has been committed. The second patch should be sent to pgsql-www for integrating into the web site. Side project: I noticed that these new hover links don't appear in the single-page HTML output (make postgres.html), even though the generated HTML source code looks correct. Maybe someone has an idea there.
On 13.04.2023 at 10:31, Peter Eisentraut wrote: > The first patch has been committed. Yay - thank you! > The second patch should be sent to pgsql-www for integrating into the > web site. Done via [1]. Thanks for the hint. > Side project: I noticed that these new hover links don't appear in the > single-page HTML output (make postgres.html), even though the > generated HTML source code looks correct. Maybe someone has an idea > there. I feel responsible for the feature to work for all use cases where it makes sense. I'll investigate this and post back. Regards, Brar [1] https://www.postgresql.org/message-id/d987a4a7-62c3-7e0c-860f-1c96fc2117d9%40gmx.de
Re: doc: add missing "id" attributes to extension packaging page
From
Dagfinn Ilmari Mannsåker
Date:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: > On 06.04.23 16:19, Brar Piening wrote: >> 001-make_html_ids_discoverable_v5.postgresql.patch which needs to be >> applied to the postgresql repository. It adds the XSLT to generate the >> id links and the CSS to hide/display them. I've added comments as >> suggested above. >> 002-add-discoverable-id-style_v1.pgweb.patch which needs to be applied >> to the pgweb repository. It adds the CSS to the offical documentation site. > > The first patch has been committed. > > The second patch should be sent to pgsql-www for integrating into the > web site. > > Side project: I noticed that these new hover links don't appear in the > single-page HTML output (make postgres.html), even though the generated > HTML source code looks correct. Maybe someone has an idea there. Another side note: I notice the links don't appear on <refsectN> elements (e.g. https://www.postgresql.org/docs/devel/sql-select.html#SQL-WITH), only <sectN>. - ilmari
On Thu, 13 Apr 2023 15:58:03 +0100 Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: > > The first patch has been committed. > Another side note: I notice the links don't appear on <refsectN> > elements (e.g. > https://www.postgresql.org/docs/devel/sql-select.html#SQL-WITH), only > <sectN>. This we know. Working with <refsectN> elements is a different dive into the XSLT which was deliberately put off for future work. Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
On Thu, 13 Apr 2023 16:01:35 +0200
Brar Piening <brar@gmx.de> wrote:
> On 13.04.2023 at 10:31, Peter Eisentraut wrote:
> > The first patch has been committed.
>
> Yay - thank you!
>
> > The second patch should be sent to pgsql-www for integrating into
> > the web site.
> Done via [1]. Thanks for the hint.
>
> > Side project: I noticed that these new hover links don't appear in
> > the single-page HTML output (make postgres.html), even though the
> > generated HTML source code looks correct. Maybe someone has an idea
> > there.
> I feel responsible for the feature to work for all use cases where it
> makes sense. I'll investigate this and post back.
Looks to me like the ">" in the CSS was transformed into the >
HTML entity when the stylesheet was included into the single-file
HTML.
Regards,
Karl <kop@karlpinc.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On Thu, 13 Apr 2023 10:53:31 -0500 "Karl O. Pinc" <kop@karlpinc.com> wrote: > On Thu, 13 Apr 2023 16:01:35 +0200 > Brar Piening <brar@gmx.de> wrote: > > > On 13.04.2023 at 10:31, Peter Eisentraut wrote: > > > Side project: I noticed that these new hover links don't appear in > > > the single-page HTML output (make postgres.html), even though the > > > generated HTML source code looks correct. Maybe someone has an > > > idea there. > > I feel responsible for the feature to work for all use cases where > > it makes sense. I'll investigate this and post back. > > Looks to me like the ">" in the CSS was transformed into the > > HTML entity when the stylesheet was included into the single-file > HTML. The XSLT 1.0 spec says that characters in <style> elements should not be escaped when outputting HTML. [4] But (I think) the generate.css.header parameter method [1][2] of inserting the CSS into the HTML expands the CSS content in an XML context, not a HTML context. I've played around with it, going so far as to make stylesheet.css look like: --------------<snip>-------- <!DOCTYPE xsl:stylesheet [ <!ENTITY css SYSTEM "stylesheet.css"> ]> <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> &css; </xsl:stylesheet> --------------<snip>-------- and even substituted the actual text of stylesheet.css in place of the &css; entity reference. In these cases the "<" character is still entity expanded, resulting in broken CSS. My conclusion is that this method is broken. (The other possibility, I suppose, is that xsltproc is broken.) I think the thing to try is the sagehill.net approach [4]. This overrides the user.head.content template. My hope is that because the &css; entity is seen in a <style> element in the template, that xsltproc then recognizances style element content in a HTML output context. (I don't know how xsltproc is supposed to know that it is in a HTML output context. I suppose exploring this would be another avenue should the above fail.) 1 https://docbook.sourceforge.net/release/xsl/current/doc/html/custom.css.source.html 2 https://docbook.sourceforge.net/release/xsl/current/doc/html/generate.css.header.html 3 http://sagehill.net/docbookxsl/HtmlHead.html#EmbedCSS 4 https://www.w3.org/TR/xslt-10/#section-HTML-Output-Method Regards, Karl <kop@karlpinc.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein