From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Linking to ImageMagick by default Date: Fri, 28 Dec 2018 10:12:35 +0200 Message-ID: <834lay6pe4.fsf@gnu.org> References: <20181205223901.GA5543@breton.holly.idiocy.org> <20181208183810.GA2465@breton.holly.idiocy.org> <20181210220944.GA4793@breton.holly.idiocy.org> <20181219160308.GA43504@breton.holly.idiocy.org> <83efadcw3j.fsf@gnu.org> <20181227131105.GA18792@breton.holly.idiocy.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1545984695 30663 195.159.176.226 (28 Dec 2018 08:11:35 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 28 Dec 2018 08:11:35 +0000 (UTC) Cc: emacs-devel@gnu.org To: Alan Third Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Dec 28 09:11:31 2018 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gcnEu-0007qZ-7u for ged-emacs-devel@m.gmane.org; Fri, 28 Dec 2018 09:11:28 +0100 Original-Received: from localhost ([127.0.0.1]:57427 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gcnH1-0003q1-3m for ged-emacs-devel@m.gmane.org; Fri, 28 Dec 2018 03:13:39 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:36985) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gcnGA-0003pq-Sm for emacs-devel@gnu.org; Fri, 28 Dec 2018 03:12:48 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gcnG6-0003CE-MW for emacs-devel@gnu.org; Fri, 28 Dec 2018 03:12:46 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:52970) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gcnG5-0003Bt-6H; Fri, 28 Dec 2018 03:12:42 -0500 Original-Received: from [176.228.60.248] (port=3940 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1gcnFy-0008Uy-Qa; Fri, 28 Dec 2018 03:12:41 -0500 In-reply-to: <20181227131105.GA18792@breton.holly.idiocy.org> (message from Alan Third on Thu, 27 Dec 2018 13:11:05 +0000) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 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" Xref: news.gmane.org gmane.emacs.devel:232016 Archived-At: > Date: Thu, 27 Dec 2018 13:11:05 +0000 > From: Alan Third > Cc: emacs-devel@gnu.org > > I’ve now run into another issue. Now that I’m stripping the image spec > down to the basics when I load an image in image mode and then use + > or - to resize, it doesn’t redraw. > > It redraws correctly if you cause a redraw, by scrolling the window, > or making an animated gif animate, or similar. Does it redraw if you just type "M-x" and nothing else? > I think that during redisplay something must be looking at the image > and deciding it hasn’t changed, but I can’t find anything like that. My first suspicion is that redisplay doesn't even look at the image, because nothing tells it that it should. IOW, it's a side effect of redisplay optimizations that should be disabled in this case. > Patch attached. Thanks, a couple of comments below. > +/* Struct for use by display functions. Contains the desired size of > + the image, derived from the display properties. */ > + > +struct image_spec > +{ > + ptrdiff_t image_id; > + int width; > + int height; > + > + /* Relief to draw around the image. */ > + int relief; > + > + /* Optional margins around the image. This includes the relief. */ > + int hmargin, vmargin; > + > + /* Percent of image height used as ascent. A value of > + CENTERED_IMAGE_ASCENT means draw the image centered on the > + line. */ > + int ascent; > +#define DEFAULT_IMAGE_ASCENT 50 > +#define CENTERED_IMAGE_ASCENT -1 > +}; I'm not sure I understand the need for introducing this new struct. For starters, it repeats some of the fields we have already in other structures. Can you explain why you needed this? Why not store the results of calculating the size (in calc_image_spec) in the original structure returned by IMAGE_FROM_ID, for example? One reason to avoid this new struct is that it makes struct glyph, struct glyph_string, and struct it larger, which will make the display code slower, especially if the larger structs cause spilling of CPU caches. The display engine shuffles these structures to and fro in many places, so they should be as small as possible. > @@ -5213,17 +5210,8 @@ buffer_posn_from_coords (struct window *w, int *x, int *y, struct display_pos *p > } > > #ifdef HAVE_WINDOW_SYSTEM > - if (it.what == IT_IMAGE) > - { > - /* Note that this ignores images that are fringe bitmaps, > - because their image ID is zero, and so IMAGE_OPT_FROM_ID will > - return NULL. This is okay, since fringe bitmaps are not > - displayed in the text area, and so are never the object we > - are interested in. */ > - img = IMAGE_OPT_FROM_ID (it.f, it.image_id); > - if (img && !NILP (img->spec)) > - *object = img->spec; > - } > + if (it.what == IT_IMAGE && !NILP (it.object)) > + *object = it.object; > #endif This doesn't look right. The original code puts in *OBJECT the Lisp spec of the image, whereas you put there the object being iterated by struct it, which is a buffer or a Lisp string. Please make sure you test the functions that call buffer_posn_from_coords, as part of your testing, such as (AFAIR) posn-at-x-y, and verify that the info they return is unchanged, when the coordinates are on an image. > + /* FIXME: Surely there's a better way to do this? */ > + if (!EQ (property, QCwidth) > + && !EQ (property, QCheight) > + && !EQ (property, QCmax_width) > + && !EQ (property, QCmax_height) > + && !EQ (property, QCscale) > + && !EQ (property, QCmargin) > + && !EQ (property, QCascent) > + && !EQ (property, QCrelief)) > + cache_spec = Fcons (property, Fcons (value, cache_spec)); Why did you think there was something wrong with this code? One possible alternative would be not to cons a new image spec without these attributes, but compare specs ignoring these attributes instead of using EQ. Did you consider this alternative? > +/* Compute the desired size of an image with native size WIDTH x HEIGHT. > + Use SPEC to deduce the size. Store the desired size into > + *D_WIDTH x *D_HEIGHT. Store -1 x -1 if the native size is OK. */ > +static void > +compute_image_size (size_t width, size_t height, > + Lisp_Object spec, > + int *d_width, int *d_height) > +{ > + Lisp_Object value; > + int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1; > + double scale = 1; > + > + value = image_spec_value (spec, QCscale, NULL); > + if (NUMBERP (value)) > + scale = XFLOATINT (value); > + > + value = image_spec_value (spec, QCmax_width, NULL); > + if (FIXNATP (value)) > + max_width = min (XFIXNAT (value), INT_MAX); > + > + value = image_spec_value (spec, QCmax_height, NULL); > + if (FIXNATP (value)) > + max_height = min (XFIXNAT (value), INT_MAX); > + > + /* If width and/or height is set in the display spec assume we want > + to scale to those values. If either h or w is unspecified, the > + unspecified should be calculated from the specified to preserve > + aspect ratio. */ > + value = image_spec_value (spec, QCwidth, NULL); > + if (FIXNATP (value)) > + { > + desired_width = min (XFIXNAT (value) * scale, INT_MAX); > + /* :width overrides :max-width. */ > + max_width = -1; > + } > + > + value = image_spec_value (spec, QCheight, NULL); > + if (FIXNATP (value)) > + { > + desired_height = min (XFIXNAT (value) * scale, INT_MAX); > + /* :height overrides :max-height. */ > + max_height = -1; > + } > + > + /* If we have both width/height set explicitly, we skip past all the > + aspect ratio-preserving computations below. */ > + if (desired_width != -1 && desired_height != -1) > + goto out; This avoids the unnecessary calculations too late, IMO. Since I expect most images to not require resizing, I suggest to make that frequent case as fast as it is today, which means avoid any unnecessary lookups in the image spec. As a minimum, you don't need to look up :max-width and :max-height. Bonus points if you avoid calling this function altogether when the image needs no resizing.