From mboxrd@z Thu Jan  1 00:00:00 1970
From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=)
Subject: Re: patch for list-packages
Date: Thu, 15 Aug 2013 16:54:47 +0200
Message-ID: <8761v7c8co.fsf@gnu.org>
References: <87mwoxw6i0.fsf@honeybear.home> <874naw9dm6.fsf@honeybear.home>
	<5208205F.1010103@gmail.com> <87eh9ykjao.fsf@karetnikov.org>
	<87d2pi5ykq.fsf@honeybear.home>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Return-path: <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([2001:4830:134:3::10]:60152)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1V9z6l-0007KM-2R
	for guix-devel@gnu.org; Thu, 15 Aug 2013 11:05:11 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1V9z6e-0004xz-LZ
	for guix-devel@gnu.org; Thu, 15 Aug 2013 11:05:02 -0400
Received: from hera.aquilenet.fr ([141.255.128.1]:52693)
	by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <ludo@gnu.org>)
	id 1V9z6e-0004xC-C1
	for guix-devel@gnu.org; Thu, 15 Aug 2013 11:04:56 -0400
In-Reply-To: <87d2pi5ykq.fsf@honeybear.home> (Alex Sassmannshausen's message
	of "Tue, 13 Aug 2013 00:33:25 +0200")
List-Id: <guix-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/guix-devel>
List-Post: <mailto:guix-devel@gnu.org>
List-Help: <mailto:guix-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=subscribe>
Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
To: Alex Sassmannshausen <alex.sassmannshausen@gmail.com>
Cc: guix-devel@gnu.org

Hi Alex,

Alex Sassmannshausen <alex.sassmannshausen@gmail.com> skribis:

> Yes, sorry =E2=80=94 still getting used to gnus as my mail client.

FWIW, in Gnus I make inlined text/x-patch attachments when posting
patches.  Yours in this message were marked as text/x-diff, and not
inlined, so I typed =E2=80=98t text/x-patch RET=E2=80=99 on each of them, a=
nd that=E2=80=99s
fine.  :-)

Regarding your work, I really like
<http://www.atheia.org/guix/output-final.html>!

> From c3a9ba4635e0af47423391b9777ec64f872bd2ab Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <alex.sassmannshausen@gmail.com>
> Date: Sun, 4 Aug 2013 21:46:26 +0200
> Subject: [PATCH 1/3] list-packages: Centralise CSS styling in <head>.
>
> * build-aux/list-packages.scm (package-logo): Assign class of
>   'package-description' to package synopsis div; 'package-logo'.  Move in=
line
>   CSS where possible.
>   (packages->sxml): Assign id of 'intro' to intro div, 'packages' to the
>   table. Move inline CSS.
>   (list-packages): Create new <style> section, containing all inline CSS.
>   Move JavaScript <script> section to above banner include to place it in
>   <head>.

Applied.

> From 1bc1c348e8a792d3ac7b22bbb82ea9285d03f1ea Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <alex.sassmannshausen@gmail.com>
> Date: Sun, 11 Aug 2013 19:53:15 +0200
> Subject: [PATCH 2/3] list-packages tidying: tidying and refactoring in
>  preparation for substantive changes.
>
> * build-aux/list-packages.scm (license package): add title for <a> elemen=
t.
>   (status package): add title for <a> element.
>   (package->sxml package): add alt and title for gnu-logo <img> element.
>   (package->sxml package): add title to package website <a> element.
>   (packages->sxml packages): wrap <div id=3D"intro"> intro paragraph in <=
p> element.
>   (packages->sxml packages): add table header row to <table id=3D"package=
s">
>   (packages->sxml packages): add <a> back to top of the page beneath tabl=
e.
>   (insert-css): create new function returning page's CSS; apply whole loa=
d of new CSS.
>   (insert-js): create new function returning page's JavaScript.
>   (list-packages . args): move JavaScript to (insert-js).
>   (list-packages . args): move CSS to (insert-css).

Applied, with an additional copyright line for you, and with commit log
slightly modified to match GNU ChangeLog conventions (for instance, only
the procedure name is shown, not its formal parameters, and it doesn=E2=80=
=99t
need to be repeated; new procedures/vars do not need to be documented in
the change log, just mentioned.)

One comment:

> +(define (insert-css)
> +  "Return the CSS for the list-packages page."
> +  (format #t
> +"<style>
> +a {transition: all 0.3s}
> +div#intro {margin-bottom: 5em}
> +div#intro div, div#intro p {padding:0.5em}
> +div#intro div {float:left}
> +table#packages, table#packages tr, table#packages tbody, table#packages =
td,
> +table#packages th {border: 0px solid black}
> +div.package-description {position: relative}
> +table#packages tr:nth-child(even) {background-color: #FFF}
> +table#packages tr:nth-child(odd) {background-color: #EEE}
> +table#packages tr:hover, table#packages tr:focus, table#packages tr:acti=
ve {background-color: #DDD}
> +table#packages tr:first-child, table#packages tr:first-child:hover, tabl=
e#packages tr:first-child:focus, table#packages tr:first-child:active {
> +background-color: #333;
> +color: #fff;
> +}

I would rather indent it in C style, for improved readability.

More importantly, I think it=E2=80=99s worth moving to a separate file now,
like:

  ...
    #:use-module (guix build utils)
  ...

  (define %load-directory
    (dirname (current-filename)))

  (define %css-file
    (string-append %load-directory "/package-list.css"))

  (define (insert-css)
    (with-input-from-file %css-file
      (cute dump-port <> (current-output-port))))

WDYT?  Could you do this?  I think the CSS itself can be put under CC0,
so the file would need the appropriate header.

> From 2b9542cfcc180a6b5fc3b0ed28217eb0896c0d2b Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <alex.sassmannshausen@gmail.com>
> Date: Sun, 11 Aug 2013 21:40:29 +0200
> Subject: [PATCH 3/3] list-packages.scm: cause description to be shown, and
>  'void' links to not be shown when JS is disabled.
>
> * build-aux/list-packages.scm (package->sxml package): no longer use
>   package-synopsis as link as we cannot assume the JavaScript in the link=
 will
>   work (no JS safety net); instead use a span.
>   (package->sxml package): remove default invisibility of package
>   descriptions.
>   (package->sxml package): call (show_hide-grouper description-id) after =
each
>   package is processed to collect the package description-ids to be made
>   invisible.
>   (show_hide-grouper id-or-force): new function; collect n package IDs, a=
nd to
>   insert a JS call to bulk_show_hide with the package IDs to be processed=
 into
>   the xml.
>   (packages->sxml packages): call (show_hide-grouper #f) after (map
>   package->sxml packages) returns to force the remaining package IDs to be
>   processed, even if n have not been reached yet.
>   (insert-js): (show_hide(idThing)): add general clause to check whether
>   working, DOM conforming JS exists; introduce thingLink variable to allo=
w its
>   text to be modified depending on the show/hide package description stat=
e of
>   a given package.
>   (insert-js): (prep(idThing)): new JS function that generates a link in =
the
>   span containing package-synopsis. New link provides show/hide
>   package-description functionality.
>   (insert-js): (bulk_show_hide()): new JS function that is able to process
>   (first call prep, then show_hide on a package-ID) any number of package=
-IDs
>   sequentially (for now defaults to 15, as provided by (show_hide-grouper
>   id-or-force)).

Overall OK, modulo two things:

  1. Could you adjust the commit log as mentioned above?

  2. See below:

> +  (define show_hide-grouper
> +    (let ((lid '())
> +          (group-counter 15))
> +      (lambda (id-or-force)
> +        "Collect GROUP-COUNTER element IDs, then apply them to
> +bulk_show_hide. Pass ID-OR-FORCE #f to apply collected IDs to bulk_show_=
hide
> +even when GROUP-COUNTER IDs have not been collected."

Apparently this procedure is only called with #f.  Can you remove the
=E2=80=98id-or-force=E2=80=99 parameter and the =E2=80=98else=E2=80=99 case=
 of the =E2=80=98cond=E2=80=99 then?

Seems like this would allow you to get rid of =E2=80=98lid=E2=80=99 and the=
 =E2=80=98set!=E2=80=99
statements, right?  (=E2=80=98set!=E2=80=99 is evil, you know.  ;-))

> +        (letrec
> +            ((lid->js-argl
> +              (lambda (l separator)
> +                "Parse a list into JavaScript style arguments."
> +                (if (null? l)
> +                    ""
> +                    (string-append separator "'" (car l) "'"
> +                                   (lid->js-argl (cdr l) ", "))))))

For readability, better use =E2=80=98define=E2=80=99 here (internal defines=
 are
equivalent to =E2=80=98letrec=E2=80=99.)

Thank you for taking care of this!

Ludo=E2=80=99.

