From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Alex Gramiak Newsgroups: gmane.emacs.bugs Subject: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32 Date: Thu, 02 May 2019 13:41:56 -0600 Message-ID: <878svomynv.fsf@gmail.com> References: <877ebeor2d.fsf@gmail.com> <83tveit5ph.fsf@gnu.org> <87pnp5oqu1.fsf@gmail.com> <877ebcogg4.fsf@gmail.com> <83sgu0rsue.fsf@gnu.org> <8736lznzjf.fsf@gmail.com> <83zho6ox5u.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="173775"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) Cc: 35468@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu May 02 21:42:15 2019 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hMHaw-000izD-Rh for geb-bug-gnu-emacs@m.gmane.org; Thu, 02 May 2019 21:42:15 +0200 Original-Received: from localhost ([127.0.0.1]:57822 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hMHav-0003Qe-OV for geb-bug-gnu-emacs@m.gmane.org; Thu, 02 May 2019 15:42:13 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:45010) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hMHam-0003QW-O1 for bug-gnu-emacs@gnu.org; Thu, 02 May 2019 15:42:06 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hMHak-0002Lm-VD for bug-gnu-emacs@gnu.org; Thu, 02 May 2019 15:42:04 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:33508) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hMHak-0002Lf-RM for bug-gnu-emacs@gnu.org; Thu, 02 May 2019 15:42:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hMHak-0006XJ-Ib for bug-gnu-emacs@gnu.org; Thu, 02 May 2019 15:42:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alex Gramiak Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 02 May 2019 19:42:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 35468 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 35468-submit@debbugs.gnu.org id=B35468.155682612125118 (code B ref 35468); Thu, 02 May 2019 19:42:02 +0000 Original-Received: (at 35468) by debbugs.gnu.org; 2 May 2019 19:42:01 +0000 Original-Received: from localhost ([127.0.0.1]:47052 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hMHai-0006X4-Tn for submit@debbugs.gnu.org; Thu, 02 May 2019 15:42:01 -0400 Original-Received: from mail-pg1-f175.google.com ([209.85.215.175]:45615) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hMHah-0006Wp-Bx for 35468@debbugs.gnu.org; Thu, 02 May 2019 15:42:00 -0400 Original-Received: by mail-pg1-f175.google.com with SMTP id i21so1505328pgi.12 for <35468@debbugs.gnu.org>; Thu, 02 May 2019 12:41:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=uP266WGI6k98nZTLvbjKNHVsGHCCCLd+TX8eFfQZqE8=; b=R+biJmGOyWy98iNaeWznNmMe8xZczDtZGCShrd0LiAViVmvi1JEzIF7rm3CErkyE85 QtrWzU8LKIuYpQdYm/pL729M6evh/upixG0tslzGeOkf6dF6VVjRpnoEQg+cRQhjEExa wD/2CLxYXlGRZykjwQs8rCSNVWP8x1JGc3f4akM87Gd8vYJpolOArSVi73qLgb4IDK/v T2gmUKT/E7KW0t8b8+L2DhC9pqHSASpgQK+6VtbQOu3JA4FLnCJDDXiWwWD6HD6hqWjS CdJ3PAX27GOVyk3jFdeju5J6Qb07nRV0zQhVjfklJMUQRDa899ah68Ch6TZY/AgV9O2z 05FA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=uP266WGI6k98nZTLvbjKNHVsGHCCCLd+TX8eFfQZqE8=; b=jBLvrU4XLICqLP3kVFc7bG/Rqbm52rnyVvGZ5oZzudf2lmMb5PyKBl6UsP302zwPaS YERCxxPONXU/b1CADiD/PlNB1RJPbBouNUFXYIxtr6VSFGy+jgwCPcH+KZfHssvopXTQ 7Eewwj1jTJ6sMThAbC05qiyZyAoAGP25kIyLBSO8dgKMbyzn42l+CbLe2kRovWFSCAFB Hdu33hjxnwp3ML76bIPNnDYt5O8DjmvQWYW6mdZkV8YrO/rTdqMwwt8/7vomuKXoTYwY fa4lXoaKke+opbLZQsyBEXkyQBoHOqDvystR4CwHVm5xPLHNIWKepeq4smuSoHgPxVON z75w== X-Gm-Message-State: APjAAAXGnl/ePHoHaSLdwxTxdH26TlamD730wsCjGG2eSe1luZaaw2P9 OmB2b0YdIEdtvCK8hhqyMbFra0gQ X-Google-Smtp-Source: APXvYqwP4qJE4MdFRxADOvXVeAZddoMOZ/C0TwdxJJSIVFLBZ1TJaEaRf/P72I0n3AxSr56cL/kqRQ== X-Received: by 2002:a62:6845:: with SMTP id d66mr6142853pfc.21.1556826112986; Thu, 02 May 2019 12:41:52 -0700 (PDT) Original-Received: from lylat ([2604:3d09:e37f:1500:1a72:4878:e793:7302]) by smtp.gmail.com with ESMTPSA id d38sm60348pgd.40.2019.05.02.12.41.51 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 02 May 2019 12:41:52 -0700 (PDT) In-Reply-To: <83zho6ox5u.fsf@gnu.org> (Eli Zaretskii's message of "Wed, 01 May 2019 21:19:09 +0300") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.51.188.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:158660 Archived-At: Eli Zaretskii writes: >> From: Alex Gramiak >> Cc: 35468@debbugs.gnu.org >> Date: Tue, 30 Apr 2019 12:00:52 -0600 >> >> I think that there can be a single interface for both glyph_string >> filling (e.g., in *_clear_glyph_string_rect), and for non-glyph_string >> filling (e.g., in *_draw_bar_cursor), but I'm not sure it would be worth >> it. I would think that having two separate interfaces, one taking in a >> glyph string and the other taking in a frame, would be cleaner: >> >> fill_rectangle_glyph (struct glyph_string *, GC, int, int, int, int); >> fill_rectangle (struct frame *, GC, int, int, int, int); >> >> There needs to be a glyph_string argument in one version so that the w32 >> version can use s->HDC. If there was only one interface, then one would >> have to supply NULL to the glyph_string argument whenever not dealing >> with glyph strings, which IMO is unclean. > > It is slightly inelegant, but it certainly isn't a catastrophe. > Unused arguments that are there for the sake of a function signature > compatibility is not an uncommon technique in C. I suppose. It's only because of the w32 version that uses s->HDC that `s' needs to be passed in at all for the glyph string version. > What is needed in fill_rectangle_glyph from the glyph_string except > the frame pointer? It depends, sometimes it's just the frame and GC (and HDC), and sometimes it's other attributes like x/y/width/height. AFAICT, all arguments except the frame (it's always s->f) are needed for the glyph string filling. > (The name is also sub-optimal: what is a "rectangle glyph"?) I meant it as "fill rectangle from glyph", but I guess I was too terse. >> So is having to have two versions, but I think it's the best option >> unless you know of a way to embed HDC in w32's version of GC. > > I see no reason to add HDC to the w32 GC, they can remain separate at > this point. I'm not sure how it would affect the w32 side, but I just mentioned combining the two since it would make "more sense" from the generic POV. > But please note that gc->foreground and background used in some cases > are exactly the color numbers used directly in other cases, so I think > you could always pass colors or always pass a GC. Do you mean that I could leave out either the color or the GC argument? The X/Cairo side at least need the GC (since it's not always s->gc), and outside of the {get,set}_context_* way I posted, I don't see how I could leave out the color argument. >> As for the color manipulation, I went low-level as you said before: >> >> unsigned long foreground, background; >> gdif->get_context_foreground (gc, &foreground); >> gdif->get_context_background (gc, &background); >> gdif->set_context_foreground (gc, background); >> gdif->fill_rectangle (s, gc, x, y, w, h); >> gdif->set_context_foreground (gc, foreground); >> >> which replaces the XGetGCValues section in x_draw_stretch_glyph_string. >> This unfortunately is more work in the w32 case as it manipulates s->gc >> instead of just using the calculated gc->background. > > I don't think I understand what you mean by "manipulates s->gc". Can > you show the code which does that? I just meant that it does something like: unsigned long foreground = s->gc->foreground; unsigned long background = s->gc->background; s->gc->foreground = background; fill_rectangle (...) s->gc->foreground = foreground; Where previously the foreground didn't need to be saved/restored in the w32 implementation. >> If that is unsatisfactory), then instead I could do something like: >> >> gdif->fill_rectangle_with_color (s, gc->background, gc, x, y, w, h); >> >> Which wouldn't touch s->gc for the w32 version and would do the whole >> XGetGCValues dance for the X version. > > So fill_rectangle will fill with the current background and > fill_rectangle_with_color will fill with the specified color? It's > possible, yes. Possible, yes, but would you prefer that over the above? >> 1) Why does w32_set_mouse_face_gc use GCFont in its mask when the X >> version doesn't? > > It's a remnant of old code which was deleted from the X version. We > should remove that. Okay, I did that and removed all usages of GCFont in the W32 version (and an unused #define GCFont in nsgui.h). >> 4) The w32 versions of some procedures need to save the font around the >> calls to font->driver->draw; is this necessary? > > Yes. The X 'draw' methods accept the font as an argument, but the w32 > implementation relies on setting the font outside of the 'draw' method > because the 'draw' method draws using the "currently selected font". > Then we need to restore the original font. Where do the X 'draw' methods accept the font as an argument? Looking at, e.g., *_draw_glyph_string_foreground, font->driver->draw takes the same arguments. Since font->driver->draw takes in the glyph string, why can't the 'draw' methods use SelectObject (s->hdc, FONT_HANDLER (s->font)) and SelectObject (s->hdc, old_font)? If they can't, is there any other way to do it inside the draw methods? It seems like it would simplify the code on the w32 side and make it more in line with the other backends. > You could define a set_font interface, with only a w32 implementation. Did you have something else in mind besides the save/restore_secondary_context_font I posted? >> if (gdif->save_secondary_context_font) > > This name is misleading: this is not a "secondary" font. We are > selecting the font to be used for drawing by the font driver's 'draw' > method. The "secondary" here applies to "context", not "context font". I used the name since the code changes s->hdc's (what I dubbed to be the "secondary context") font. Would you prefer just {save,restore}_context_font? >> Currently this requires save_secondary_context_font to allocate memory, >> which is unideal. One could avoid this by introducing a new union type >> (backend_font or somesuch) and using that instead of a void*. WDYT? > > There's no need: the HFONT data type is just a pointer, so you can > store it in a 'void *' without allocating anything. If you want to be > "more Catholic than the Pope", make that variable a unitptr_t. Okay, I was thinking about other backends, but as long as the only backends using that interface just pass pointers void * works just fine. I'm having trouble with *_draw_image_foreground -- they just seem too different to nicely abstract. Would it be okay if some generic constructs leak into it (namely: s->img->mask)? Otherwise the common setup that w32 does would be problematic. Does the RestoreDC/SaveDC call need to be around both toplevel branches (i.e., also around the w32_draw_rectangle), or just the s->img->pixmap branch? For reference, this is what I have right now for gui_draw_image_foreground: static void gui_draw_image_foreground (struct glyph_string *s) { struct graphical_drawing_interface *gdif = FRAME_GDIF (s->f); int x = s->x; int y = s->ybase - image_ascent (s->img, s->face, &s->slice); /* If first glyph of S has a left box line, start drawing it to the right of that line. */ if (s->face->box != FACE_NO_BOX && s->first_glyph->left_box_line_p && s->slice.x == 0) x += eabs (s->face->box_line_width); /* If there is a margin around the image, adjust x- and y-position by that margin. */ if (s->slice.x == 0) x += s->img->hmargin; if (s->slice.y == 0) y += s->img->vmargin; if (gdif->save_secondary_context) gdif->save_secondary_context (s, CON_ALL); // SaveDC (s->hdc); if (gdif->glyph_has_image (s)) { gdif->draw_image (s, s->img->width, s->img->height, s->slice.x, s->slice.y, s->slice.width, s->slice.height, x, y, true); if (!gdif->glyph_image_uses_mask (s)) { /* When the image has a mask, we can expect that at least part of a mouse highlight or a block cursor will be visible. If the image doesn't have a mask, make a block cursor visible by drawing a rectangle around the image. I believe it's looking better if we do nothing here for mouse-face. */ if (s->hl == DRAW_CURSOR) { const int relief = eabs (s->img->relief); gdif->draw_rectangle (s, x - relief, y - relief, s->slice.width + relief*2 - 1, s->slice.height + relief*2 - 1); } } } else /* Draw a rectangle if image could not be loaded. */ gdif->draw_rectangle (s, x, y, s->slice.width - 1, s->slice.height - 1); if (gdif->restore_secondary_context) gdif->restore_secondary_context (s); // RestoreDC (s->hdc, -1); }