unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Michael Heerdegen <michael_heerdegen@web.de>
To: Vitalie Spinu <spinuvit@gmail.com>
Cc: Glenn Morris <rgm@gnu.org>, emacs-devel@gnu.org
Subject: Re: image-transform.el and image-mode.el rewrite
Date: Thu, 18 Dec 2014 15:17:01 +0100	[thread overview]
Message-ID: <87ppbhvy8y.fsf@web.de> (raw)
In-Reply-To: <87tx0xdzqo.fsf@gmail.com> (Vitalie Spinu's message of "Mon, 15 Dec 2014 01:33:03 -0800")

Vitalie Spinu <spinuvit@gmail.com> writes:

> I have finally got down to this.
> [...]
> At this stage the stuff is ready for a review as I have only minor
> things to fix. I will be traveling till 18th and will resume once I am
> back. Would be really great if interested people could have a quick look
> till then.

Looks like it was a lot of work - many thanks!

I can't speak for Emacs dev, hope someone with background knowledge can
also review your code.  I played with it a bit, and it was working
nicely in general.

Some comments:

- When I visit an image file, `image-manipulation-map' doesn't seem to be
  installed.  E.g. "+" is not working etc.  OTOH when I hit C-c C-c two
  times, it works then.  Is this expected?

- I think it would be good when there would be entries in the
  "Manipulate" menu (defined in `image-manipulation-map') for increasing
  and decreasing the image size.

- I see some warnings about unused lexical variables.  I think some are
  concerning pcase (where you probably need to prefix unused vars with
  underscore in some patterns), but there are also some concerning
  function arguments.

- And there are some warnings about using cl functions at runtime.  I
  guess Emacs maintainers will want you to use only cl-lib?


HTH,

Michael.



  reply	other threads:[~2014-12-18 14:17 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-14  7:25 imagemagic in image-mode and image-dired-thumbnail-mode? Vitalie Spinu
2013-07-14  8:32 ` joakim
2013-07-14 11:48   ` Vitalie Spinu
2013-07-14 12:40     ` joakim
2013-07-14 13:01       ` Vitalie Spinu
2013-07-14 12:42     ` Lars Magne Ingebrigtsen
2013-07-14 18:21       ` Glenn Morris
2013-07-14 19:50         ` Lars Magne Ingebrigtsen
2013-07-14 20:06           ` Eli Zaretskii
2013-07-14 20:11             ` Lars Magne Ingebrigtsen
2013-07-14 22:00             ` Vitalie Spinu
2013-07-15  4:38               ` Eli Zaretskii
2013-07-15  4:15             ` Stephen J. Turnbull
2013-07-15  4:46               ` Eli Zaretskii
2013-07-15  5:45                 ` Stephen J. Turnbull
2013-07-15 10:39                   ` Óscar Fuentes
2013-08-02 15:32                     ` Steinar Bang
2013-07-15 15:50                   ` Eli Zaretskii
2013-07-14 18:33   ` Glenn Morris
2013-07-14 19:17     ` joakim
2013-07-15 10:51       ` Vitalie Spinu
2013-07-16 15:57         ` Glenn Morris
2013-07-16 21:26         ` Stefan Monnier
2013-07-17  7:29           ` Vitalie Spinu
2013-07-17 15:51             ` Vitalie Spinu
2013-07-18  8:47               ` Lars Magne Ingebrigtsen
2013-07-18 22:27                 ` Vitalie Spinu
2013-07-19  9:22                   ` Stefan Monnier
2013-07-20  7:25                     ` Vitalie Spinu
2013-07-22 20:17                 ` Vitalie Spinu
2013-07-22 20:31                   ` Lars Magne Ingebrigtsen
2013-07-23  8:31                     ` Vitalie Spinu
2013-07-18 23:22               ` image-transform.el and image-mode.el rewrite Vitalie Spinu
2013-07-19 11:52                 ` Wolfgang Jenkner
2013-07-19 12:21                   ` Wolfgang Jenkner
2013-07-20  7:18                     ` Vitalie Spinu
2013-07-22 20:37                 ` Glenn Morris
2013-07-22 21:05                   ` Vitalie Spinu
2013-10-08 18:08                   ` Glenn Morris
2013-10-08 23:43                     ` Vitalie Spinu
2013-10-09  0:02                       ` Michael Heerdegen
2014-12-15  9:33                         ` Vitalie Spinu
2014-12-18 14:17                           ` Michael Heerdegen [this message]
2014-12-18 21:32                             ` Vitalie Spinu
2014-12-18 15:15                           ` Stefan Monnier
2014-12-18 23:23                             ` Vitalie Spinu
2014-12-19  4:19                               ` Stefan Monnier
2014-12-19  4:46                                 ` Vitalie Spinu
2014-12-19  8:56                                 ` Eli Zaretskii
2014-12-19 17:50                                   ` Stefan Monnier
2014-12-19 19:37                                     ` Eli Zaretskii
2014-12-19 21:31                                       ` Stefan Monnier
2014-12-19 21:49                                         ` Eli Zaretskii
2014-12-19 10:24                                 ` Vitalie Spinu
2014-12-19 17:51                                   ` Stefan Monnier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87ppbhvy8y.fsf@web.de \
    --to=michael_heerdegen@web.de \
    --cc=emacs-devel@gnu.org \
    --cc=rgm@gnu.org \
    --cc=spinuvit@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).