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: Wed, 13 Mar 2019 18:19:40 +0200 Message-ID: <83imwm3fxf.fsf@gnu.org> References: <20190211165636.ch5x4wb2ibdt2dzy@Ergus> <83ef8el03u.fsf@gnu.org> <20190308185744.a4vnfoab5wdvqyny@Ergus> <83y35p871q.fsf@gnu.org> <20190309132207.w2ho3j6p5on6fyzw@Ergus> <838sxo87gc.fsf@gnu.org> <20190311104814.kp2nv6arv47hcykz@Ergus> <83y35l4ee0.fsf@gnu.org> <20190312152928.73o4b5fk4paz7wm5@Ergus> <834l883w15.fsf@gnu.org> <20190312192017.fkfd4h5gsbdue5q3@Ergus> Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="261035"; 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 Wed Mar 13 17:19:52 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 1h46bf-0015o9-Iu for ged-emacs-devel@m.gmane.org; Wed, 13 Mar 2019 17:19:51 +0100 Original-Received: from localhost ([127.0.0.1]:47485 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h46be-0004Pl-Kg for ged-emacs-devel@m.gmane.org; Wed, 13 Mar 2019 12:19:50 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:55036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h46bY-0004Pg-W7 for emacs-devel@gnu.org; Wed, 13 Mar 2019 12:19:46 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:49451) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h46bY-00026X-9v; Wed, 13 Mar 2019 12:19:44 -0400 Original-Received: from [176.228.60.248] (port=4142 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1h46bX-0000jZ-JI; Wed, 13 Mar 2019 12:19:44 -0400 In-reply-to: <20190312192017.fkfd4h5gsbdue5q3@Ergus> (message from Ergus on Tue, 12 Mar 2019 20:20:17 +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:234129 Archived-At: > Date: Tue, 12 Mar 2019 20:20:17 +0100 > From: Ergus > Cc: emacs-devel@gnu.org > > Please tell me any problem you see related or not with my questions to > fix them too. Thanks, see below. > >> I tried: > >> if (!row->reversed_p) > >> { > >> while (glyph >= start > >> && (glyph->type == CHAR_GLYPH > >> || glyph->type == STRETCH_GLYPH) > >> && NILP (glyph->object)) > >> --glyph; > >> } > >> > >> But it didn't work. The behavior is the same: the whitespace is > >> highlighted only when the line is crossed. > > > >I guess at this point I'd need to see the code to help you more. The problem here is that the above loop also looks at the object from which the glyph came, and will only skip glyphs whose object is nil. But in your code, you didn't set up the object of the '|' character, which comes from it->object, to be nil, you left it at its previous value, which usually will be either a buffer or a string, depending from where the last character of the text line came. To fix this, add this: Lisp_Object save_object = it->object; it->object = Qnil; before the call to PRODUCE_GLYPHS, and then restore it->object to its previous value after PRODUCE_GLYPHS. In the TTY case, this was already done for you by the code which extends the face, that's why it worked on TTY frames. > >> The other corner case I have is because in graphical mode the space for > >> the dot is always reserved, so when the last character in the line is > >> just before the line, the line is not drawn for that line. > > > >Sorry, I don't understand: what is "the dot" in this context, and what > >do you mean by "just before the line"? There are too many "lines" in > >this sentence, so I'm confused. I guess you mean that when the place for the cursor is at fill-column, the indicator is not visible? I think to fix this, you will need to modify the code in the function append_space_for_newline, which is responsible for adding the space glyph for displaying the cursor at the end of the line. That function currently adds a ' ' space character there; you want to replace that with the indicator character '|', when this mode is in effect. A few more comments regarding the code: > + if (!NILP (Vdisplay_fill_column_indicator)) > + { > + const int fill_column_indicator_column = > + XFIXNAT (Vdisplay_fill_column_indicator_column) - 1; Why "- 1"? It's confusing, and should at least be explained in a comment, if not changed to not subtract 1. Also, it is unsafe to call XFIXNAT without verifying the value is a fixnum, because if it isn't, Emacs will crash. > + const int column_x = it->pixel_width * fill_column_indicator_column > + + it->lnum_pixel_width; Using it->pixel_width here will not work in general, as this is just the pixel width of the last character or image of the screen line. I think you should use the font->average_width, and if that is zero, then font->space_width. Here 'font' is the default face's font. This is because I believe we want to display the indicator at the X coordinate that corresponds to fill-column in units of the default font width, right? I mean, did you try to see what happens when the text has faces which make some of the characters appear smaller or larger than the default face? > + struct font *font = face->font ? face->font : FRAME_FONT (f); I think this uses the wrong face: you should be using default_face instead. The above is the face of the last character of the line. > + it->char_to_display = '|'; This character should be customizable. Perhaps make display-fill-column-indicator serve double duty: if non-nil, it should be the character to use as the indicator. > + it->face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID); You didn't really mean to use the line-number face here, did you? This should probably be a separate distinct face. > + DEFVAR_LISP ("display-fill-column-indicator", Vdisplay_fill_column_indicator, > + doc: /* Non-nil means display the fill column indicator line. */); The "line" part of the doc string doesn't belong there, I think. Just "indicator" is enough. > + DEFVAR_LISP ("display-fill-column-indicator-column", Vdisplay_fill_column_indicator_column, > + doc: /* Column where to draw the column indicator line when display-column-indicator > +is non-nil . */); The first line of a doc string should be a complete sentence, and it should fit the 80column display line. In this case, I'd shorten it like this: Column to draw the indicator when `display-fill-column-indicator' is non-nil. Please also say in the doc string what are the possible values and in what column units they are measured. Thanks for working on this.