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.devel Subject: Re: Fill column indicator functionality Date: Thu, 14 Mar 2019 08:40:27 +0200 Message-ID: <835zsm2c2s.fsf@gnu.org> References: <20190309132207.w2ho3j6p5on6fyzw@Ergus> <838sxo87gc.fsf@gnu.org> <20190311104814.kp2nv6arv47hcykz@Ergus> <83y35l4ee0.fsf@gnu.org> <20190312152928.73o4b5fk4paz7wm5@Ergus> <834l883w15.fsf@gnu.org> <20190312192017.fkfd4h5gsbdue5q3@Ergus> <83imwm3fxf.fsf@gnu.org> <20190313200225.dpqrw7xthkj47fqw@Ergus> <83bm2e35a1.fsf@gnu.org> <20190314030224.l5zseslncw3xc5ox@Ergus> Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="156892"; mail-complaints-to="usenet@blaine.gmane.org" Cc: emacs-devel@gnu.org To: Ergus Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Mar 14 07:41:25 2019 Return-path: Envelope-to: ged-emacs-devel@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 1h4K3Q-000egU-3U for ged-emacs-devel@m.gmane.org; Thu, 14 Mar 2019 07:41:24 +0100 Original-Received: from localhost ([127.0.0.1]:57904 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4K3O-0004FJ-VL for ged-emacs-devel@m.gmane.org; Thu, 14 Mar 2019 02:41:22 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:39051) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4K2q-0004F9-8l for emacs-devel@gnu.org; Thu, 14 Mar 2019 02:40:49 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:60135) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4K2p-0004jK-TI; Thu, 14 Mar 2019 02:40:47 -0400 Original-Received: from [176.228.60.248] (port=1669 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1h4K2p-0003S1-AY; Thu, 14 Mar 2019 02:40:47 -0400 In-reply-to: <20190314030224.l5zseslncw3xc5ox@Ergus> (message from Ergus on Thu, 14 Mar 2019 04:02:24 +0100) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] 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:234140 Archived-At: > Date: Thu, 14 Mar 2019 04:02:24 +0100 > From: Ergus > Cc: emacs-devel@gnu.org > > 1) In the graphical mode the character I use now (?\u2502) does not look > like a vertical line (there is a small vertical space between them so > the line now looks like with ?|); in terminal it looks fine. It looks fine with the default font I use here in GUI frames: no vertical space between lines. So this obviously depends on the font used for the indicator character. > What's the proper way to change the font size or the character > itself to fit the entire row height in the graphical mode? I don't recommend changing the size, because that will change the height of the entire screen line, and will have the unpleasant side effect of changing the vertical spacing of those lines that don't exceed the fill-column, while lines that do exceed it will have the original vertical spacing. Plus, it won't make the vertical space between the indicator characters disappear. Instead, I suggest to change the font used for that character, if your default font has this problem. The way to change both the size and the font of the character is by customizing the face used for the indicator. And since you already let users customize that face, you can leave this issue alone, and rely on users to find a suitable font. > 2) Are there any extra considerations needed to use the character > ?\u2502 ? Is it needed to check (for example) if the actual fonts can > display it properly? If the user customized the face, you don't need to make sure the font can display it: it's up to the user to do so. For the default, you could perhaps check in the function which turns on this mode that U+2502 is displayable, and if not, fall back on the ASCII '|'. See char-displayable-p. There's also font-get-glyphs, if you need a more accurate test (but I don't think you do). > 3) Actually there are 3 possible configuration options the column > number, the character, and the font. Should we provide some other > alternative? See above: since you already provide a special face, the font customization is already covered and doesn't need a separate knob. > The next step is to implement the R2L version of this, but I don't have > used R2L editing ever, so it may take a while. Are you sure you need to do anything at all for supporting R2L? Both PRODUCE_GLYPHS and append_stretch_glyph DTRT transparently with R2L screen lines, so I think you don't need anything else. You can try using TUTORIAL.he as your playing ground; I don't expect you to see any problems with this feature in R2L lines. A couple of comments to the patch you posted: > + int local_default_face_id = lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID); Please keep the source lines shorter than 78 columns; longer lines should be broken into several shorter lines (here and elsewhere in your patch). > - the end of the row, there will be no stretch glyph, > - so leave the box flag set. */ > + the end of the row, there will be no stretch glyph, > + so leave the box flag set. */ > && saved_x + FRAME_COLUMN_WIDTH (it->f) < it->last_visible_x) > - it->end_of_box_run_p = false; > + it->end_of_box_run_p = false; This part seems to change only the whitespace, and I don't see any reason for doing that here. > + it->char_to_display = XFIXNAT(Vdisplay_fill_column_indicator_character); Please leave one space between the macro name and the opening parenthesis that follows it. > - while (it->current_x <= it->last_visible_x) > - PRODUCE_GLYPHS (it); > + /* Display fill-column-line if mode is active */ > + if (!NILP (Vdisplay_fill_column_indicator)) > + { > + const int fill_column_indicator_line = > + XFIXNAT (Vdisplay_fill_column_indicator_column) + it->lnum_pixel_width; > + do > + { > + if (it->current_x == fill_column_indicator_line) > + { > + const int saved_face = it->face_id; > + it->face_id = merge_faces (it->w, Qfill_column, 0, DEFAULT_FACE_ID); > + it->c = it->char_to_display = XFIXNAT(Vdisplay_fill_column_indicator_character); > + PRODUCE_GLYPHS (it); > + it->face_id = saved_face; > + it->c = it->char_to_display = ' '; > + } > + else > + PRODUCE_GLYPHS (it); > + } while (it->current_x < it->last_visible_x); > + } > + else > + { > + do > + { > + PRODUCE_GLYPHS (it); > + } while (it->current_x < it->last_visible_x); > + } Here you replaced a while loop with a do-while loop, which means you always produce at least one glyph, where the original code might not have produced any glyphs. Did you verify this cannot give us any trouble? Can this code be entered with it->current_x == it->last_visible_x? > + /* Names of the faces used to display fill column indicator character. */ > + DEFSYM (Qfill_column, "fill-column"); "Name of the face", in singular, right? Also, please keep 2 spaces after the last sentence of a comment, like this: /* Names of the faces used to display fill column indicator character. */ ^^^ > + DEFVAR_LISP ("display-fill-column-indicator-character", Vdisplay_fill_column_indicator_character, > + doc: /* Character to draw the indicator when `display-fill-column-indicator' is non-nil. > +The default is ?\u2502 the but a good alternative is (ascii 124) if > +yours font doesn't support unicode characters. */); We use the U+NNNN notation in documentation and comments, not ?\uNNNN. Also, please capitalize "Unicode". And "yours font" should be "your font" (I'd actually reword to refer to the font specified by the fill-column face). Thanks. P.S. Btw, there's no need to post diffs for ldefs-boot.el, that file is regenerated and committed automatically from time to time, courtesy of Glenn's scripts.