From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32 Date: Sun, 28 Apr 2019 20:11:54 +0300 Message-ID: <83tveit5ph.fsf@gnu.org> References: <877ebeor2d.fsf@gmail.com> Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="45620"; mail-complaints-to="usenet@blaine.gmane.org" Cc: 35468@debbugs.gnu.org To: Alex Gramiak Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sun Apr 28 19:13:20 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 1hKnMe-000Bkz-64 for geb-bug-gnu-emacs@m.gmane.org; Sun, 28 Apr 2019 19:13:20 +0200 Original-Received: from localhost ([127.0.0.1]:46631 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hKnMd-0000i1-6t for geb-bug-gnu-emacs@m.gmane.org; Sun, 28 Apr 2019 13:13:19 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:36727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hKnMS-0000hj-Er for bug-gnu-emacs@gnu.org; Sun, 28 Apr 2019 13:13:09 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hKnMN-0000Mh-Sx for bug-gnu-emacs@gnu.org; Sun, 28 Apr 2019 13:13:06 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:52407) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hKnMM-0000M8-IO for bug-gnu-emacs@gnu.org; Sun, 28 Apr 2019 13:13:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hKnMM-0006gz-CG for bug-gnu-emacs@gnu.org; Sun, 28 Apr 2019 13:13:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 28 Apr 2019 17:13: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.155647153425635 (code B ref 35468); Sun, 28 Apr 2019 17:13:02 +0000 Original-Received: (at 35468) by debbugs.gnu.org; 28 Apr 2019 17:12:14 +0000 Original-Received: from localhost ([127.0.0.1]:37718 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hKnLZ-0006fP-Po for submit@debbugs.gnu.org; Sun, 28 Apr 2019 13:12:14 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:50414) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hKnLY-0006fC-PA for 35468@debbugs.gnu.org; Sun, 28 Apr 2019 13:12:13 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:38449) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hKnLT-0007GL-Ms; Sun, 28 Apr 2019 13:12:07 -0400 Original-Received: from [176.228.60.248] (port=3612 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1hKnLS-0006Xh-R9; Sun, 28 Apr 2019 13:12:07 -0400 In-reply-to: <877ebeor2d.fsf@gmail.com> (message from Alex Gramiak on Sat, 27 Apr 2019 19:29:30 -0600) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] 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:158400 Archived-At: > From: Alex Gramiak > Date: Sat, 27 Apr 2019 19:29:30 -0600 > > This merges x_draw_glyph_string and w32_draw_glyph_string together to > remove duplicated code. > > I wanted to do it for NS as well, but it seems just a bit too different > to make it easily work. If we don't include NS in this, we won't actually gain enough to justify the reshuffle. What prevented you from including NS, it looks to me the code is very similar? But maybe wait with the answer until you read the rest of the comments below. > +struct draw_glyph_string_interface I'd prefer draw_glyphs_interface, it's shorter. And given the comments below, maybe the name should be gui_interface. > -x_draw_glyph_string (struct glyph_string *s) > +x_draw_underwave (struct glyph_string *s) > { > - bool relief_drawn_p = false; > - > - /* If S draws into the background of its successors, draw the > - background of the successors first so that S can draw into it. > - This makes S->next use XDrawString instead of XDrawImageString. */ > - if (s->next && s->right_overhang && !s->for_overlaps) > + if (s->face->underline_defaulted_p) > + x_draw_underwave_1 (s); > + else > { > - int width; > - struct glyph_string *next; > - > - for (width = 0, next = s->next; > - next && width < s->right_overhang; > - width += next->width, next = next->next) > - if (next->first_glyph->type != IMAGE_GLYPH) > - { > - x_set_glyph_string_gc (next); > - x_set_glyph_string_clipping (next); > - if (next->first_glyph->type == STRETCH_GLYPH) > - x_draw_stretch_glyph_string (next); > - else > - x_draw_glyph_string_background (next, true); > - next->num_clips = 0; > - } > + XGCValues xgcv; > + XGetGCValues (s->display, s->gc, GCForeground, &xgcv); > + XSetForeground (s->display, s->gc, s->face->underline_color); > + x_draw_underwave_1 (s); > + XSetForeground (s->display, s->gc, xgcv.foreground); > } > +} I think this stops short of the goal. The goal is to refactor the code into terminal-independent code and terminal-dependent code, and then arrange the latter in meaningful interfaces that will be useful henceforth for writing display code. By contrast, what you did leaves in terminal-specific parts code that is completely independent of the window-system, and OTOH doesn't try to identify terminal-specific parts which are different implementations of the same general idea. For example, all GUI terminals have code that sets foreground and background colors, code that clears a rectangle, code that fills a rectangle with a given color, code that draws a line between 2 points, etc. We should identify such pieces of code, give them names, and make them the methods called via the redisplay interface pointers. As a rule, no terminal-independent code should ever appear in the window-system specific part, not even logic that looks at terminal-independent data structures, like this, for example: + if (s->face->strike_through_color_defaulted_p) + { + w32_fill_area (s->f, s->hdc, s->gc->foreground, s->x, + y, s->width, height); + } + else + { + w32_fill_area (s->f, s->hdc, s->face->strike_through_color, s->x, + y, s->width, height); + } The meaning of s->face->strike_through_color_defaulted_p has no place in w32term.c, it should be in the terminal-independent part, and w32_fill_area should be a w32-specific implementation of a GUI drawing primitive. Likewise with stuff like XGetGCValues on X and DC on w32. IOW, if we are refactoring this stuff, let's do it right, not just ad-hoc because the current code by some chance lends itself to a particular division into parts, not just because it's easy. If we go the easy way, chances are that tomorrow someone who'd like to add a new GUI feature will again have to duplicate code because of lack of necessary primitive building blocks, which were left lumped together with terminal-independent code instead of being separated. The result of this refactoring should be more low-level and more primitive interfaces, and they should each one make sense, not be ad-hoc. It means the job becomes more complex, and you will probably need to ask questions regarding the GUI systems you are less familiar with. But the result will IMO much better and future-proof. And once again, please put all the new gui_* functions extracted from *term.c on a new file gui_term.c, not in xdisp.c. Thanks.