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: Tue, 30 Apr 2019 12:00:52 -0600 Message-ID: <8736lznzjf.fsf@gmail.com> References: <877ebeor2d.fsf@gmail.com> <83tveit5ph.fsf@gnu.org> <87pnp5oqu1.fsf@gmail.com> <877ebcogg4.fsf@gmail.com> <83sgu0rsue.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="33308"; 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 Tue Apr 30 20:08:40 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 1hLXBH-0008Vm-Pi for geb-bug-gnu-emacs@m.gmane.org; Tue, 30 Apr 2019 20:08:40 +0200 Original-Received: from localhost ([127.0.0.1]:51301 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hLXBG-0005nX-NJ for geb-bug-gnu-emacs@m.gmane.org; Tue, 30 Apr 2019 14:08:38 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:55779) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hLXAR-0005AF-Kk for bug-gnu-emacs@gnu.org; Tue, 30 Apr 2019 14:07:48 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hLX4u-0007eZ-Mi for bug-gnu-emacs@gnu.org; Tue, 30 Apr 2019 14:02:06 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:57062) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hLX4s-0007bT-S4 for bug-gnu-emacs@gnu.org; Tue, 30 Apr 2019 14:02:04 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hLX4s-000807-HQ for bug-gnu-emacs@gnu.org; Tue, 30 Apr 2019 14:02: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: Tue, 30 Apr 2019 18:02: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.155664726330670 (code B ref 35468); Tue, 30 Apr 2019 18:02:02 +0000 Original-Received: (at 35468) by debbugs.gnu.org; 30 Apr 2019 18:01:03 +0000 Original-Received: from localhost ([127.0.0.1]:42370 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hLX3v-0007yc-AY for submit@debbugs.gnu.org; Tue, 30 Apr 2019 14:01:03 -0400 Original-Received: from mail-pf1-f195.google.com ([209.85.210.195]:44620) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hLX3t-0007y1-3s for 35468@debbugs.gnu.org; Tue, 30 Apr 2019 14:01:01 -0400 Original-Received: by mail-pf1-f195.google.com with SMTP id y13so7426143pfm.11 for <35468@debbugs.gnu.org>; Tue, 30 Apr 2019 11:01:01 -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=ezKfEvfgubBMpX7/tObcxQOtzQSg3cWOg3IMxt9/KQI=; b=RWN404FSyf/BQJeRCTtW1QZ1QDgZu9dxSbkZtnqq6rynO7GR48xra6PBGfTS/p/hRJ lh62D8yqG+7NZjQTeEstIb9MhyfHfu9xZggKyGPckiT5hL8qLbt7ATweJjzKmjkAqIB0 p390NilqIwd2Dyr6nbezdqZfPHsuiZ4npETs8V7fhrwpzjUlJySis9q+jbW9+0JWgfml XCy9evT9BGKtMVPrWQXO10HB0aMNqmtRk4sDKmiLnK1kw1Jycoq8aAcwGpJ8EmOFRk0s msZS1QwWs5h3nJ5eJH7+Zp88cPBBYPfF7Hjz/2FvlwFET3bm8sPWpow6iTpgCojasFp8 PzzQ== 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=ezKfEvfgubBMpX7/tObcxQOtzQSg3cWOg3IMxt9/KQI=; b=HE90HCjoqXpZwwoRxAJR492dOIp7rqXnf8NKAQcfACXVxM7WB1IlBdCFnIjZK4pXTc jU/4nY35+FQ/976J0XOEe0d+fezG4ndfhLKZKP//mwZTUcbnmVXdfEbpzdVCsdthPpw4 736L6S8q7Ln/lwup+i7p9rh0xDAXgdKi98j/vGNvJhY7EzRelXo5mLWm5fr+bebMHvJ7 lyt39+7Pw4D6HbkQdhWGUGd+UiMGOb8YKgW7rddrkBBTvCF0QSHg3UWYGoEOVhP1oBap eBSL89gDjf0br8dLJ8jc5os3iQ53M1rB9EH7WMwWVKNEHp3euDbRkG/t4U973i6pHP8a GC+Q== X-Gm-Message-State: APjAAAXqEAo+kCwo4cLRfuq2Ea8XaRYZSHOAVHIn3SeojsZJ23pJfPWG 3V2UHYiHKEitq97RsVe7J70NaypG X-Google-Smtp-Source: APXvYqzkYr4lYSzqTg3JHRCnWSNkpGa/ewn8xcQIbOBtv0dT4llS6U4mCkRVCDFZSj9IQxRpX6Xnzg== X-Received: by 2002:a63:f503:: with SMTP id w3mr64621606pgh.60.1556647254366; Tue, 30 Apr 2019 11:00:54 -0700 (PDT) Original-Received: from lylat ([2604:3d09:e37f:1500:1a72:4878:e793:7302]) by smtp.gmail.com with ESMTPSA id c28sm27401657pgm.42.2019.04.30.11.00.51 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 30 Apr 2019 11:00:52 -0700 (PDT) In-Reply-To: <83sgu0rsue.fsf@gnu.org> (Eli Zaretskii's message of "Tue, 30 Apr 2019 07:59:37 +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:158525 Archived-At: Eli Zaretskii writes: >> From: Alex Gramiak >> Cc: 35468@debbugs.gnu.org >> Date: Mon, 29 Apr 2019 11:43:23 -0600 >> >> I'm in {x,w32}_draw_box_rect right now, trying to generalize both >> versions. The issue is that the fill command in each accepts different >> arguments; specifically the w32 version takes in the color explicitly >> and uses s->hdc instead of s->gc. So I think there will have to be 2 >> different fill_rectangle interface procedures: one for glyph strings (so >> that the w32 version can access s->hdc), and another for other >> procedures like *_draw_bar_cursor. > > Would it be possible to generalize this into a single interface? The > GC vs HDC part can be generalized by passing both (HDC will be NULL in > the X and NS cases), and the color will be passed as in the w32 > version, with the X version doing its thing with GCForeground > internally. Alternatively, we could have a set_foreground interface > that will do nothing for w32 and for X call XGetGCValues and > XSetForeground, to set up s->gc. The second alternative will probably > be more efficient. > > If this doesn't work, can you tell why? 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. 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. 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'm not sure how one would make a no-op version of setting the context foreground work in all fill calls. 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. Alternatively, if you don't want another version, there could be a mandatory color argument that one could supply with a pre-defined invalid color to instead use the GC color. I have some more questions regarding w32 incompatibilities that I ran into: 1) Why does w32_set_mouse_face_gc use GCFont in its mask when the X version doesn't? 2) Why does w32_draw_glyphless_glyph_string_foreground have the boolean `with_background' instead of using false unconditionally like the X version does? Should the X version be updated? 3) Why does w32_draw_glyph_string for FACE_UNDER_LINE use a thickness of 1 for w32_fill_area instead of using s->underline_thickness like X/NS do? This seems like a bug. 4) The w32 versions of some procedures need to save the font around the calls to font->driver->draw; is this necessary? Specifically, see w32_draw_glyph_string_foreground. Assuming it's necessary, I generalized it as: static void gui_draw_glyph_string_foreground (struct glyph_string *s) { int i, x; struct graphical_drawing_interface *gdif = FRAME_GDIF (s->f); /* If first glyph of S has a left box line, start drawing the text of S to the right of that box line. */ if (s->face->box != FACE_NO_BOX && s->first_glyph->left_box_line_p) x = s->x + eabs (s->face->box_line_width); else x = s->x; /* Currently just used by the w32 port to update S->hdc. */ if (gsif->update_secondary_context) gsif->update_secondary_context (s, CON_BACK | CON_FORE | CON_ALIGN); /* Draw characters of S as rectangles if S's font could not be loaded. */ if (s->font_not_found_p) { for (i = 0; i < s->nchars; ++i) { struct glyph *g = s->first_glyph + i; gdif->draw_rectangle (s, x, s->y, g->pixel_width - 1, s->height - 1); x += g->pixel_width; } } else { struct font *font = s->font; int boff = font->baseline_offset; int y; void *old_font; if (gdif->save_secondary_context_font) old_font = gdif->save_secondary_context_font (s); if (font->vertical_centering) boff = VCENTER_BASELINE_OFFSET (font, s->f) - boff; y = s->ybase - boff; if (s->for_overlaps || (s->background_filled_p && s->hl != DRAW_CURSOR)) font->driver->draw (s, 0, s->nchars, x, y, false); else font->driver->draw (s, 0, s->nchars, x, y, true); if (s->face->overstrike) font->driver->draw (s, 0, s->nchars, x + 1, y, false); if (gdif->set_secondary_context_font) gdif->set_secondary_context_font (s, old_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?