[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Fill column indicator functionality

From: Ergus
Subject: Re: Fill column indicator functionality
Date: Thu, 14 Mar 2019 17:51:49 +0100
User-agent: NeoMutt/20180716

On Thu, Mar 14, 2019 at 08:40:27AM +0200, Eli Zaretskii wrote:

3) Actually there are 3 possible configuration options the column
number, the character, and the font. Should we provide some other

See above: since you already provide a special face, the font
customization is already covered and doesn't need a separate knob.

Sorry, I wanted to put face not fonts in fact

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.
A couple of comments to the patch you posted:

+         int local_default_face_id = lookup_basic_face (it->w, it->f, 

Please keep the source lines shorter than 78 columns; longer lines
should be broken into several shorter lines (here and elsewhere in
your patch).

That's why I needed to implement this functionality ;).

-                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 = 

Please leave one space between the macro name and the opening
parenthesis that follows it.

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?

In the original code there was a PRODUCE_GLYPHS (it); just before the
while loop, so at least one was always produced, that's why I switched
to a do loop. Checking the patch, a potential issue may come from the
fact that I changes the <= for a < and actually there was a missing
GLYPH at the very end. I fixed that now.

+  /* 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

 /* Names of the faces used to display fill column indicator character.  */


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?

More questions:

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??

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.

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.

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.


Attachment: display-fill-column-indicator.patch
Description: Text document

reply via email to

[Prev in Thread] Current Thread [Next in Thread]