From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Ergus Newsgroups: gmane.emacs.devel Subject: Re: Fill column indicator functionality Date: Wed, 13 Mar 2019 21:02:25 +0100 Message-ID: <20190313200225.dpqrw7xthkj47fqw@Ergus> References: <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> <83imwm3fxf.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="64743"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: NeoMutt/20180716 Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Mar 13 21:20:16 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 1h4AMJ-000GgZ-NJ for ged-emacs-devel@m.gmane.org; Wed, 13 Mar 2019 21:20:16 +0100 Original-Received: from localhost ([127.0.0.1]:50301 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4AMI-0002BH-Mo for ged-emacs-devel@m.gmane.org; Wed, 13 Mar 2019 16:20:14 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:43738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4AJu-0000dk-Tt for emacs-devel@gnu.org; Wed, 13 Mar 2019 16:17:53 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1h4A5A-00016M-Q1 for emacs-devel@gnu.org; Wed, 13 Mar 2019 16:02:34 -0400 Original-Received: from sonic311-31.consmr.mail.ir2.yahoo.com ([77.238.176.163]:39248) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4A5A-00015g-3f for emacs-devel@gnu.org; Wed, 13 Mar 2019 16:02:32 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aol.com; s=a2048; t=1552507349; bh=Cx9KbS4q30mjwtUpXXTiyg67ZJHsBRatjiBvdt45hjM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From:Subject; b=L7/J1RCl/jxR1e1woCg+tE3NflhS6Gig6c4tiikyDVj833znGcLfvSupoEFvNhYl8a+InWok5doHsXgT92/J/KVrl0qjB38cc2mdfAeUaw7aqF+7vuWiXOtpn9EDWVS0XdFtR5DFyxjrWsZFgHoL4Z2RJgvdmYcMQVGgg1chPIksOCZo2Ee4dXugSbj7K2VGIKsF/RCphsaQK4w1f8IIpfTzcU48p69TDmy/JPSgh8sKNBDNQzJ82s22dtjhKzbfXqH0fpykJdk2L8wXC05xuwGj/1A8X01LzcIzafCP5UxiIPgF9Y0gDRBDgdJ1G5D6mnFZD4xEyU0w0Ya8mowdiQ== X-YMail-OSG: C9h9xCMVM1kfwavcgWFckPMEC9eltsmNPGZTTmX3Zxlofk7U3kEAnWtbXQrgB2v gJ28bCu1OwkTmtE4o6veoMJ.wpodRMgWYKSPGtLdxrHKYRlX_XlcDRwCYskSK8AKkRCcgCLiHyQC wpeWkt9Vqn_VWDbkgKxbXnz6HE0PBuOnD.K_L_g34XmsB9an41f.LyHZevOZTGyRp5YoIMMmorrP ETC.YVbIMw1KIDFgRm0Bc7KBHUq8xm90HXXiY5SuSrZd3T6TT7Jn7.JMvjNEMKWsUohU6AKd7UWm UFhtipdxXqvdVLCAjPwdYWr_r4QYBPLUlq52tbKNJOrqXDieoTrSpaKf58ZLDBGPnySw7Zi0WEj2 WflO7hAtJOKcJ_iNJ8noIuCunuX4y9xmc2idXnAcTyuCV9ui.QOzj0srHYteoC_A1uLF91Uiy95E VH7WZGhchopmzwrvHYHydgckZluO5lbfPywsX_hlK8yOCQ4zncQAQAZJvek5auxUZOvOZueDBID. p476cuyUW3FXAtKV0JierLsSWU727gB6Flw.ldGCWPYDDuJyOgVHgFH5XGWGscGjgeuWAKMZpi.C Q0z2OsC_2e9xTH4ivKW2hZyQil2sm7kcc_onaaGcb0jf5ninNakRpsrr8_C4LKpNstEppXMDJ8qs L7r1ZovLLRMD24NvFAFEyc9fcNNH7yjqKcg6H62yLR5Aodi7Af0Wla1Luuo4dAd.aJzdlzL5tKHg jVUiMnjlCWWKbCsiGkze_YEUim2T0BuR63pC8_75CdQprrh40bw.JDzfHeg4tPTlhDG51D5H9USC 27WemujnBUG1pifFHEv9BzBuotOWGBFUvKtPuMo9t3 Original-Received: from sonic.gate.mail.ne1.yahoo.com by sonic311.consmr.mail.ir2.yahoo.com with HTTP; Wed, 13 Mar 2019 20:02:29 +0000 Original-Received: from 2.152.205.184.dyn.user.ono.com (EHLO Ergus) ([2.152.205.184]) by smtp422.mail.ir2.yahoo.com (Oath Hermes SMTP Server) with ESMTPA ID c472fff997a7fa954a96e5a2cb4456d6; Wed, 13 Mar 2019 20:02:27 +0000 (UTC) Content-Disposition: inline In-Reply-To: <83imwm3fxf.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 77.238.176.163 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:234136 Archived-At: Hi Eli: On Wed, Mar 13, 2019 at 06:19:40PM +0200, Eli Zaretskii wrote: >> 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. > I thought that append_stretch_glyph was putting the it->object to Now I see it didn't. >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. > Yes I was referring to cursor as point. My bad. >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. > The -1 is related with the extra ' ' added by the extend_face function, I will try to avoid it, in the final patch, but now this is just a prove of concept. >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; > Ok, I'll prevent this. >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); > You just saved me a lot of time of reading the font and face structs definitions (I will but not now). >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 is customizable actually in my current version, I just sent you the basic changes to make it work that exposed the issues with minimal code changes. In fact the default value will be ?\u2402 (and not ?| as in the example) if the font supports it because it will look like a contiguous line. >> + 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. > Yes, I did, this was a prove of concept. I created a fill_column 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. > Ok, perfect >Thanks for working on this. 1E6 Thanks. Ergus