bug#32359: [PATCH] Add svg-path

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
The patch adds `svg-path'. Among other things, this function makes it
possible to add arcs to SVG images. A path is drawn using commands
defined by the SVG standard:

https://www.w3.org/TR/SVG11/paths.html#PathData

add-svg-path.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Eli Zaretskii
> From: "Felix E. Klee" <[hidden email]>
> Date: Fri, 3 Aug 2018 13:07:42 +0200
>
> The patch adds `svg-path'. Among other things, this function makes it
> possible to add arcs to SVG images. A path is drawn using commands
> defined by the SVG standard:
>
> https://www.w3.org/TR/SVG11/paths.html#PathData

Thanks.  This contribution can be accepted without legal paperwork,
but the next one will need a copyright assignment, so I'd encourage
you to start your paperwork now.

A few comments, mainly about the documentation parts.

> diff --git a/ChangeLog.3 b/ChangeLog.3
> index a0a4794b4e..2a9832b67b 100644
> --- a/ChangeLog.3
> +++ b/ChangeLog.3
> @@ -1,3 +1,7 @@
> +2018-08-03  Felix E. Klee  <[hidden email]>
> +
> + * lisp/svg.el (svg-path): New function.
> +

We don't maintain a ChangeLog file; the above should be the commit log
message.

> +@defun svg-path svg commands &rest args
> +Add the outline of a shape to @var{svg}. The @var{commands} follow the
> +Scalable Vector Graphics standard. This function can be used to create
> +arcs.

This is too cryptic for the manual, and the example doesn't help
enough.  We should at least explain what "arcs" means in this context,
and in general what is this function about; also what kind of object
is SCG.

Also, please observe our standard of having 2 spaces between
sentences.

In general, GNU Coding Standards frown upon using "path" for anything
that is not PATH-style directory lists, so maybe use a different name
or explain what kind of "path" is being referenced here.  E.g., the
Web page to which you pointed does include a definition of "path" in
this context.

> +(defun svg-path (svg commands &rest args)
> +  "Add the outline of a shape to SVG. The COMMANDS follow the
> +Scalable Vector Graphics standard. This function can be used to
> +create arcs."

The first line of the doc string should be a single complete sentence,
and it should mention all of the arguments.  Also, please keep 2
spaces between sentences in the doc strings.  I also think the doc
string should say that COMMANDS are strings, or objects whose printed
representation yields valid SVG commands.

This new function should also be announced in NEWS.



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
Hi Eli,

thanks for the feedback!

On 8/3/18, Eli Zaretskii <[hidden email]> wrote:
> We don't maintain a ChangeLog file; the above should be the commit
> log message.

Well, the Emacs info page on committing patches states: “Write the
change log entries for your changes. […]”

Furthermore, it links to a page explaining `ChangeLog' files in
particular. Is that documentation outdated?

`ChangeLog.3' contains entries for previous additions to `svg.el', the
latest one as recent as September 2017.

Anyhow, if you want a commit log instead of a `ChangeLog' entry, how
should I submit the commit?

>> +@defun svg-path svg commands &rest args
>> +Add the outline of a shape to @var{svg}. The @var{commands} follow the
>> +Scalable Vector Graphics standard. This function can be used to create
>> +arcs.
>
> This is too cryptic for the manual,

It’s basically in line with the description for the other `svg' functions.

> and the example doesn't help enough.

For the intended audience, i.e. those knowing how to author SVG
documents, it should be clear. What I could do is add aliases for the
path commands:

  * `moveto-relative' → `m'

  * `moveto-absolute' → `M'

  * etc.

The example would then turn into:

    (svg-path svg '((moveto-absolute 100 300)
                    (arc-absolute 300 300 0 0 0 300 100)
                    (closepath-absolute))
              :stroke-color "blue" :fill-color "yellow")

Still users would need to know the command parameters which are
detailed in every comprehensive documentation about SVG.

> We should at least explain what "arcs" means in this context,

“arcs” in the context of SVG, i.e. vector graphics refers to curved
paths:

https://www.merriam-webster.com/dictionary/arc

> and in general what is this function about; also what kind of object
> is SCG.

Quote from the documentation in the elisp info page for `svg.el': “SVG
(Scalable Vector Graphics) is an XML format for specifying images.”

> In general, GNU Coding Standards frown upon using "path" for
> anything that is not PATH-style directory lists, so maybe use a
> different name or explain what kind of "path" is being referenced
> here.

Renaming `path' would be super confusing to those familiar with vector
graphics. If `path' needs to be avoided, then I’d rather not add
`svg-path'.

Instead one could add a more general:

    svg-node (parent tag &rest args)

This function is needed anyhow. It would be for inserting custom SVG
nodes, i.e. nodes beyond those currently available in the `svg'
package.

BTW I got in contact with Lars, the original author of `svg.el', and
he’s OK with the changes I proposed, including some additional
functions.

/ Felix



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Eli Zaretskii
> From: "Felix E. Klee" <[hidden email]>
> Date: Thu, 23 Aug 2018 17:50:08 +0200
> Cc: [hidden email]
>
> thanks for the feedback!

Thanks for working on improving Emacs.

> > We don't maintain a ChangeLog file; the above should be the commit
> > log message.
>
> Well, the Emacs info page on committing patches states: “Write the
> change log entries for your changes. […]”

The relevant place to look nowadays is CONTRIBUTE in the top-level
directory of the Emacs tree.

> Furthermore, it links to a page explaining `ChangeLog' files in
> particular. Is that documentation outdated?

It says "change logs", not ChangeLog.  However, since it confused you,
I have now replaced that with "commit log".  The reference to the GNU
Coding Standards is still relevant, I think, because we still use the
ChangeLog style in our commit log messages.

> `ChangeLog.3' contains entries for previous additions to `svg.el', the
> latest one as recent as September 2017.

That file is auto-generated nowadays from Git log.

> Anyhow, if you want a commit log instead of a `ChangeLog' entry, how
> should I submit the commit?

The best is in "git format-patch" format, which will include your
commit log message.  If that's hard or complicated for you, a normal
patch will do with the commit log as preamble, i.e. not in Diff
format.

> >> +@defun svg-path svg commands &rest args
> >> +Add the outline of a shape to @var{svg}. The @var{commands} follow the
> >> +Scalable Vector Graphics standard. This function can be used to create
> >> +arcs.
> >
> > This is too cryptic for the manual,
>
> It’s basically in line with the description for the other `svg' functions.

Maybe so, but there's no reason to continue that practice when we add
new functions.  It is also okay to fix documentation of those other
functions, but of course it doesn't have to be part of this particular
changeset.

> > and the example doesn't help enough.
>
> For the intended audience, i.e. those knowing how to author SVG
> documents, it should be clear.

Okay, but the manual exists not only for those already "in the know".
It should also cater to those who are studying the subject with the
purpose of writing some code for the first time in this domain.

> What I could do is add aliases for the path commands:
>
>   * `moveto-relative' → `m'
>
>   * `moveto-absolute' → `M'
>
>   * etc.

I think it would be better simply to explain those in plain English,
not by adding code.

> The example would then turn into:
>
>     (svg-path svg '((moveto-absolute 100 300)
>                     (arc-absolute 300 300 0 0 0 300 100)
>                     (closepath-absolute))
>               :stroke-color "blue" :fill-color "yellow")
>
> Still users would need to know the command parameters which are
> detailed in every comprehensive documentation about SVG.

Exactly.  So it's slightly better (less cryptic), but still not clear
enough, because the meaning of moveto-relative and moveto-absolute is
not entirely obvious, only their general idea is evident ("relative"
vs "absolute").

I understand that it isn't reasonable to have the entire SVG docs
there, but we should explain a bit more before pointing to the
official SVG docs for further details.  I trust you that you will know
where to draw the line.

> > We should at least explain what "arcs" means in this context,
>
> “arcs” in the context of SVG, i.e. vector graphics refers to curved
> paths:
>
> https://www.merriam-webster.com/dictionary/arc

I didn't mean explain to me, I meant explain in the manual.

> > and in general what is this function about; also what kind of object
> > is SCG.
>
> Quote from the documentation in the elisp info page for `svg.el': “SVG
> (Scalable Vector Graphics) is an XML format for specifying images.”

We are mis-communicating.  I didn't mean SVG the acronym, I meant SVG
the argument of the function.  The doc strings says

  Add the outline of a shape to SVG.

It is quite clear that "SVG" here doesn't stand for Scalable Vector
Graphics, it stands for some object to which the function will add a
shape.  I'm asking to say a word or two about what kind of object is
that, from the Lisp program POV.  is it a string? a symbol? a list? a
buffer? something else?

> > In general, GNU Coding Standards frown upon using "path" for
> > anything that is not PATH-style directory lists, so maybe use a
> > different name or explain what kind of "path" is being referenced
> > here.
>
> Renaming `path' would be super confusing to those familiar with vector
> graphics. If `path' needs to be avoided, then I’d rather not add
> `svg-path'.

There's the second alternative: explain in a few words what kind of
"path" is meant here.  Then you can use the word freely.

> BTW I got in contact with Lars, the original author of `svg.el', and
> he’s OK with the changes I proposed, including some additional
> functions.

That's fine, but the way our patch review process works, the comments
are additive ;-)

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
On Thu, Aug 23, 2018 at 7:23 PM, Eli Zaretskii <[hidden email]> wrote:
> I think it would be better simply to explain those in plain English,
> not by adding code.

OK. Still the aliases do increase readability. They are not made up.
They are based on the “names” listed in the [specs][1].

For now, I am considering creating a separate package `svg+.el'. This
would allow me to play with various approaches, possibly with user
feedback. If that all works, one could merge the additions into Emacs.

[1]: https://www.w3.org/TR/SVG11/paths.html



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Lars Ingebrigtsen
In reply to this post by Felix E. Klee
"Felix E. Klee" <[hidden email]> writes:

> The example would then turn into:
>
>     (svg-path svg '((moveto-absolute 100 300)
>                     (arc-absolute 300 300 0 0 0 300 100)
>                     (closepath-absolute))
>               :stroke-color "blue" :fill-color "yellow")
>
> Still users would need to know the command parameters which are
> detailed in every comprehensive documentation about SVG.

I think this looks very nice -- the svg library is quite close to the
SVG specification, but when it makes sense to clarify some of the more
cryptic bits with clearer names, it should.  So these are better names
for an svg path than M/A/Z, which are extremely obscure.

So I'd welcome the patch with these name mappings.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
Please have a look at:

https://github.com/feklee/svg-plus

Now the example is:

    (svg+-path svg
               '((moveto ((100 . 300)))
                 (elliptical-arc ((300 300 0 0 :large-arc nil :sweep nil)))
                 (closepath :relative nil))
               :stroke-color "blue"
               :fill-color "yellow")

I wanted to announce this package once it’s finished, which is always a
bad idea.  IIRC only complete documentation is missing, and of course
more SVG features could be added.  On Wednesday there is the Emacs
meetup in Berlin, and indeed I was thinking about presenting the package
there.  So that could be some motivation to finish it, and then we can
talk about a patch.

Thanks for bringing it up again. :)



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Lars Ingebrigtsen
"Felix E. Klee" <[hidden email]> writes:

> Please have a look at:
>
> https://github.com/feklee/svg-plus
>
> Now the example is:
>
>     (svg+-path svg
>                '((moveto ((100 . 300)))
>                  (elliptical-arc ((300 300 0 0 :large-arc nil :sweep nil)))
>                  (closepath :relative nil))
>                :stroke-color "blue"
>                :fill-color "yellow")

That's really good -- makes it actually understandable how a path is
composed.

> I wanted to announce this package once it’s finished, which is always a
> bad idea.  IIRC only complete documentation is missing, and of course
> more SVG features could be added.  On Wednesday there is the Emacs
> meetup in Berlin, and indeed I was thinking about presenting the package
> there.  So that could be some motivation to finish it, and then we can
> talk about a patch.

Bringing that function into svg.el in Emacs would be nice.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
Hi Lars,

finally I got around to finishing documentation.  Find the latest
release at:

https://github.com/feklee/svg-plus

In addition to `svg+-path`, I added:

  * `svg+-clip-path`

  * `svg+-node`

If you want to play with the new functions, simply run the code examples
from the info page inside of `example.el`.  Concerning `svg+-path`, I’d
like to hear your thoughts: Do you see room for further improvement, or
could this be the final interface?

I was thinking about removing the double parentheses in commands such
as:

    (lineto ((100 . 200)))

One could simplify that to:

    (lineto (100 . 200))

Polylines would change from:

    (lineto ((100 . 200) (300 . 200)))

To:

    (lineto (100 . 200) (300 . 200))

Reasons against this change:

  * Parsing optional arguments (plist) could be a pain.  Currently what
    works nicely:

        (lineto ((100 . 200) (200 . 0)) :relative t)

    Elliptical arc allows additional optional arguments, such as:

        (elliptical-arc ((50 100 200 300 :large-arc 1)) :relative t)

  * Aesthetically, some standard Lisp commands aren’t “better.”  For
    example `let` also requires double parentheses for single
    assignments:

        (let ((x 3)) (* x x))

―Felix



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Lars Ingebrigtsen
"Felix E. Klee" <[hidden email]> writes:

> If you want to play with the new functions, simply run the code examples
> from the info page inside of `example.el`.  Concerning `svg+-path`, I’d
> like to hear your thoughts: Do you see room for further improvement, or
> could this be the final interface?

I haven't read it closely, but it looks good to me.  Could you post it
as a patch to svg.el?  That'll make it easier to read.

> I was thinking about removing the double parentheses in commands such
> as:
>
>     (lineto ((100 . 200)))
>
> One could simplify that to:
>
>     (lineto (100 . 200))
>
> Polylines would change from:
>
>     (lineto ((100 . 200) (300 . 200)))
>
> To:
>
>     (lineto (100 . 200) (300 . 200))
>
> Reasons against this change:
>
>   * Parsing optional arguments (plist) could be a pain.  Currently what
>     works nicely:
>
>         (lineto ((100 . 200) (200 . 0)) :relative t)

Yeah, I think you should keep the argument as a single argument.
Varargs with optional keywords parameters just isn't very nice.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
Hi Lars,

find attached a patch against (coincidentally) your latest Emacs commit:

dea9970bc0deaf320e78c46a2e7456cbb6e7a0ea

Included are changes to the code and to the manual.  If you need
anything else, let me know!

―Felix

0001-Add-functions-svg-path-svg-clip-path-svg-node.patch.gz (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Lars Ingebrigtsen
"Felix E. Klee" <[hidden email]> writes:

> Included are changes to the code and to the manual.  If you need
> anything else, let me know!

Thanks!  It looks great, so I applied it to the trunk.  The only
possible tweak was with using moveto vs. move-to.  The latter looks more
Emacsish, but in SVG speak they're moveto (etc), so I think it makes
sense to leave them that way.

But after applying, I was suddenly unsure whether you'd signed FSF
copyright assignment papers yet or not.  I thought I remembered seeing
your name in the copyright.list file before, but now I can't see it.

Have you gone through the paperwork process with the FSF?

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
On Mon, Jul 15, 2019 at 5:12 PM Lars Ingebrigtsen <[hidden email]>
wrote:
> The only possible tweak was with using moveto vs. move-to.  The latter
> looks more Emacsish, but in SVG speak they're moveto (etc), so I think
> it makes sense to leave them that way.

Good catch.  I had the same thought and decided to stick with `moveto`.

> Have you gone through the paperwork process with the FSF?

I just started with that, following:

https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html

A Savannah account I have, and I now requested an fencepost account.
Once I have that, I’ll try to find the appropriate template.

Sorry, I hope this is no problem.  Luckily, there is no issue with
copyright assignment: I am 1. old enough (Emacs user for >18y), and
2. not employed, wrote the code for my own pleasure.  But I understand
that the process can take a while, although nowadays it’s possible to
submit the form electronically:

“Contributors located in […] Germany […] can print, sign, and then email
(or fax) a scanned copy back to the FSF.”



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Noam Postavsky
"Felix E. Klee" <[hidden email]> writes:

>> Have you gone through the paperwork process with the FSF?
>
> I just started with that, following:
>
> https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html
>
> A Savannah account I have, and I now requested an fencepost account.
> Once I have that, I’ll try to find the appropriate template.

You don't need a Savannah nor fencepost account for this.  The form and
instructions are at
https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future





Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
In reply to this post by Felix E. Klee
Update: fencepost account got rejected because I’m only a contributor,
not a maintainer of a GNU project

How do I get access to the necessary copyright papers now?



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Lars Ingebrigtsen
In reply to this post by Felix E. Klee
"Felix E. Klee" <[hidden email]> writes:

> Sorry, I hope this is no problem.  Luckily, there is no issue with
> copyright assignment: I am 1. old enough (Emacs user for >18y), and
> 2. not employed, wrote the code for my own pleasure.  But I understand
> that the process can take a while, although nowadays it’s possible to
> submit the form electronically:

Sounds good, but I'll have to revert the patch in the meantime, I think.
Should be no problem to re-apply it later when the paperwork is done.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
On Tue, Jul 16, 2019 at 8:05 AM Lars Ingebrigtsen <[hidden email]>
wrote:
> Sounds good, but I'll have to revert the patch in the meantime, I
> think.

I understand.  It’s no problem at all.  I’ll let you know once the
paperwork is done.



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Felix E. Klee
In reply to this post by Noam Postavsky
On Tue, Jul 16, 2019 at 3:01 AM Noam Postavsky <[hidden email]> wrote:
> You don't need a Savannah nor fencepost account for this.  The form
> and instructions are at
> https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future

I just submitted that form, thanks!

(Savannah account would’ve been needed for fencepost, though.)



Reply | Threaded
Open this post in threaded view
|

bug#32359: [PATCH] Add svg-path

Lars Ingebrigtsen
In reply to this post by Felix E. Klee
"Felix E. Klee" <[hidden email]> writes:

> On Tue, Jul 16, 2019 at 8:05 AM Lars Ingebrigtsen <[hidden email]>
> wrote:
>> Sounds good, but I'll have to revert the patch in the meantime, I
>> think.
>
> I understand.  It’s no problem at all.  I’ll let you know once the
> paperwork is done.

Great!

I've now reverted the patch (and Glenn's follow-up patch that added a
menu).  Both will be re-reverted once the paperwork's in place.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no