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 20:22:03 +0200 Message-ID: <83r2b91flg.fsf@gnu.org> References: <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> <835zsm2c2s.fsf@gnu.org> <20190314165147.gmtwgzqaibwbzhbm@Ergus> Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="235002"; 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 21:12: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 1h4Wi7-000z0H-QT for ged-emacs-devel@m.gmane.org; Thu, 14 Mar 2019 21:12:15 +0100 Original-Received: from localhost ([127.0.0.1]:44270 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4Wi6-0004V2-De for ged-emacs-devel@m.gmane.org; Thu, 14 Mar 2019 16:12:14 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:34965) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4WW5-0004j7-Ow for emacs-devel@gnu.org; Thu, 14 Mar 2019 15:59:51 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:44031) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h4WW3-0007Sy-PP; Thu, 14 Mar 2019 15:59:49 -0400 Original-Received: from [176.228.60.248] (port=2557 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1h4V02-00086C-0m; Thu, 14 Mar 2019 14:22:40 -0400 In-reply-to: <20190314165147.gmtwgzqaibwbzhbm@Ergus> (message from Ergus on Thu, 14 Mar 2019 17:51:49 +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:234147 Archived-At: > Date: Thu, 14 Mar 2019 17:51:49 +0100 > From: Ergus > Cc: emacs-devel@gnu.org > > >> 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. > > > Now I tried and it works, and passed my tests. As expected. > >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. > > Good to know, then why it is not in the gitignore as loaddefs.el? Because unlike loaddefs.el, ldefs-boot.el is a versioned file. It must be in the repository, otherwise people would be unable to build a fresh checkout. It just isn't maintained by hand, that's all. > There is DEFVAR_INT and DEFVAR_BOOL. Does it makes sense to use those to > define display-fill-column-indicator-column and > display-fill-column-indicator; instead of DEFVAR_LISP?? Using DEFVAR_INT and DEFVAR_BOOL makes it easier to reference the variable in C, since you have a normal C integer or boolean. So, for example, you can say if (emacs_scroll_step > 0) instead of if (XFIXNAT (Vemacs_scroll_step) > 0) and if (!inhibit_message) instead of if (!NILP (Vinhibit_message)) > Should we add some code in order to assert that if the user changes > fill-column and it is equal to display-fill-column-indicator-column; > then the second one should be updated too? Because > display-fill-column-indicator-column must be == fill-column except if the > users explicitly sets it to a different value. We could use a special value of t for display-fill-column-indicator-column to mean the current value of fill-column. > Finally, if you think that there is something else missing to consider > this as ready, please tell me, else, tell me how to do a pull request. I think the main part that is missing is your copyright assignment. Did you do your legal paperwork? If not, now is the time to start it. This patch is large enough to require a copyright assignment. The only other missing parts are documentation: NEWS and the user manual. Btw, did you test the code when there are variable-size characters in the buffer? Good test cases are Info files and also Web pages with images visited via EWW. > Maybe in the future I will consider the alternative to change the > background color, but I don't want to make this too complex or add more > overhead to the display engine. Indicating the fill-column with background can use almost the same code as the one you wrote, but we will need to decide what to do if the text that exceeds fill-column itself has non-default background. Also, I think the visual effect will be less pleasant when there are continued lines, because background color is more prominent than an indicator by a single character. Thanks.