From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: image-transform.el and image-mode.el rewrite Date: Thu, 18 Dec 2014 10:15:07 -0500 Message-ID: References: <87a9lp8uxk.fsf@gmail.com> <87ppuk6qqb.fsf@gmail.com> <87vc49y793.fsf@gmail.com> <87ppuhxk04.fsf@gmail.com> <87ehavihbp.fsf_-_@gmail.com> <1a7ggiwcta.fsf@fencepost.gnu.org> <1uwqlnr5yh.fsf@fencepost.gnu.org> <87siwbnxan.fsf@gmail.com> <878uy348h3.fsf@web.de> <87tx0xdzqo.fsf@gmail.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1418915759 24025 80.91.229.3 (18 Dec 2014 15:15:59 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 18 Dec 2014 15:15:59 +0000 (UTC) Cc: Michael Heerdegen , Glenn Morris , emacs-devel@gnu.org To: Vitalie Spinu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Dec 18 16:15:52 2014 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Y1cnv-0005kS-Cu for ged-emacs-devel@m.gmane.org; Thu, 18 Dec 2014 16:15:51 +0100 Original-Received: from localhost ([::1]:54360 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1cno-0006Ph-VB for ged-emacs-devel@m.gmane.org; Thu, 18 Dec 2014 10:15:44 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:41150) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1cnL-0006OZ-QL for emacs-devel@gnu.org; Thu, 18 Dec 2014 10:15:23 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y1cnF-0002Ep-So for emacs-devel@gnu.org; Thu, 18 Dec 2014 10:15:15 -0500 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:54736) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1cnF-0002E4-Oa; Thu, 18 Dec 2014 10:15:09 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AkIPAOwQflTO+ILA/2dsb2JhbABbgwdSgw6FWr8EhhkEAgKBJBcBAQEBAQF8hAMBAQMBVhUOBQsLNBIUGA0kiEoJDdZMAQEBAQEBBAEBAQEBGQSQbweESAWLAYoemhCBeIQZIYJ3AQEB X-IPAS-Result: AkIPAOwQflTO+ILA/2dsb2JhbABbgwdSgw6FWr8EhhkEAgKBJBcBAQEBAQF8hAMBAQMBVhUOBQsLNBIUGA0kiEoJDdZMAQEBAQEBBAEBAQEBGQSQbweESAWLAYoemhCBeIQZIYJ3AQEB X-IronPort-AV: E=Sophos;i="5.07,502,1413259200"; d="scan'208";a="102890490" Original-Received: from 206-248-130-192.dsl.teksavvy.com (HELO pastel.home) ([206.248.130.192]) by ironport2-out.teksavvy.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 18 Dec 2014 10:15:07 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id 7F4D02D79; Thu, 18 Dec 2014 10:15:07 -0500 (EST) In-Reply-To: <87tx0xdzqo.fsf@gmail.com> (Vitalie Spinu's message of "Mon, 15 Dec 2014 01:33:03 -0800") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.154.181 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:180274 Archived-At: > https://github.com/vspinu/image-transform/compare/emacs...master Thanks Vitalie, overall it looks pretty good and I think it's pretty much ready for inclusion. This is not my area of expertise, so hopefully someone else can take another look, but besides Michael's comments I'll add: - You seem to have rename a few "image-mode-FOO" to "image-BAR" and also a few "image-TOTO" to "image-mode-TITI". I'm not necessarily opposed to those changes, but I'm not sure I understand the rationale behind it. I think it would be good to document (e.g. in the "Commentary:" section) the convention used to decide whether the name should be "image-mode-FOO" or "image-BAR". - While it's OK to install such a change only for image-mode.el (i.e. take it one step at a time), I recommend you take a look at doc-view.el and plan on sharing some of the code there as well. E.g. the image-scale-step should probably be merged with doc-view-shrink-factor. - Use (require 'cl-lib) instead of (require 'cl-macs). - You don't need to (require 'pcase), it's autoloaded. - Don't use `(lambda ...) when you could use a closure instead. - Use `define-error' instead of (put 'next-backend 'error-conditions ...). - Use "convert" instead of (executable-find "convert"), especially since you define the variable to hold a string, and executable-find can return nil. - (executable-find "convert") does not return a "path" but a "file name". A "path" is a list of directories, as in $PATH, load-path, ... - The :flatten arg to image-transform looks wrong. Instead of ":flatten" it should be ":flatten t" (i.e. all keywords should be followed by a value and a nil value should be equivalent to not having mentioned the keyword). IOW the "Boolean specs can miss the value, in which case t is assumed" is a misfeature. - The image-transform-features:convert seems over-engineered. I think I'd rather have a ":convert-args" that provides a mechanism to pass any args to "convert" and then remove the bulk of those keywords. Stefan