bug#35452: Line number faces should check for remapping of the default face

bug#35452: Line number faces should check for remapping of the default face

[Dario Gjorgjevski]

Currently, the line number faces do not check for remapping of the
default face and use its attributes directly.  In the default
configuration, this has no adverse effects since the `line-number' face
inherits from `default' explicitly, so any remapping is considered there.

However, there is no need to have `line-number' inherit from `default'
explicitly since it already merges DEFAULT_FACE_ID.  Instead, we can
check for remapping of DEFAULT_FACE_ID prior to merging the faces.

The patch shown below accomplishes that.

diff --git a/src/xdisp.c b/src/xdisp.c
index d52d1333a0..1e7e31fb8a 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -21176,9 +21176,10 @@ maybe_produce_line_number (struct it *it)
   char lnum_buf[INT_STRLEN_BOUND (ptrdiff_t) + 1];
   bool beyond_zv = IT_BYTEPOS (*it) >= ZV_BYTE ? true : false;
   ptrdiff_t lnum_offset = -1; /* to produce 1-based line numbers */
-  int lnum_face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);
+  int base_face_id = lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);
+  int lnum_face_id = merge_faces (it->w, Qline_number, 0, base_face_id);
   int current_lnum_face_id
-    = merge_faces (it->w, Qline_number_current_line, 0, DEFAULT_FACE_ID);
+    = merge_faces (it->w, Qline_number_current_line, 0, base_face_id);
   /* Compute point's line number if needed.  */
   if ((EQ (Vdisplay_line_numbers, Qrelative)
        || EQ (Vdisplay_line_numbers, Qvisual)

-- 
Dario Gjorgjevski :: +389 (0)70 784 142

bug#35452: Line number faces should check for remapping of the default face

[Eli Zaretskii]

> From: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>
> Date: Sat, 27 Apr 2019 14:52:10 +0200
> 
> Currently, the line number faces do not check for remapping of the
> default face and use its attributes directly.  In the default
> configuration, this has no adverse effects since the `line-number' face
> inherits from `default' explicitly, so any remapping is considered there.
> 
> However, there is no need to have `line-number' inherit from `default'
> explicitly since it already merges DEFAULT_FACE_ID.  Instead, we can
> check for remapping of DEFAULT_FACE_ID prior to merging the faces.
> 
> The patch shown below accomplishes that.

Thanks, but I don't think I understand the advantages of this approach
vs the current one.  Concretely, why would we want not to inherit
from the 'default' face?

Also, doesn't your change force the line-number face to change
together with 'default', even if the user defines the face to not
inherit from 'default'?  With the current code, users are free to
define the face without inheritance, and that will stop update the
line-number face together with 'default', e.g. when the user enlarges
the default face's font or makes it smaller.  With your proposal, the
size changes in 'default' will always be propagated to line-number,
right?

And finally, if we do make the proposed change, shouldn't we stop
inheriting from 'default' at the same time?

bug#35452: Fwd: bug#35452: Line number faces should check for remapping of the default face

[Dario Gjorgjevski]

> Thanks, but I don't think I understand the advantages of this approach
> vs the current one.  Concretely, why would we want not to inherit
> from the 'default' face?

Indeed, there is no reason not to inherit from the default face.  In
fact, what I suggested was making the inheritance _always hold_, i.e.,
despite of the user's definitions of the line number faces.  Which
brings us to the next point you brought up...

> Also, doesn't your change force the line-number face to change
> together with 'default', even if the user defines the face to not
> inherit from 'default'?  With the current code, users are free to
> define the face without inheritance, and that will stop update the
> line-number face together with 'default', e.g. when the user enlarges
> the default face's font or makes it smaller.  With your proposal, the
> size changes in 'default' will always be propagated to line-number,
> right?

I agree with you.  In fact, this is exactly the reason why I had
suggested the change -- I was using a theme were the line number faces
were not set to inherit from default, and realized that
text-scale-adjust does not affect them.

> And finally, if we do make the proposed change, shouldn't we stop
> inheriting from 'default' at the same time?

Absolutely.

With all this being said, I agree with you that it is best to let the
user choose whether he or she wants the line number faces to inherit
from the default one.

--
Dario Gjorgjevski :: dario.gjorgjevski@gmail.com :: +389 (0)70 784 142

bug#35452: Line number faces should check for remapping of the default face

[Eli Zaretskii]

[Forwarding to the bug tracker; please use Reply All in the future.]

> From: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>
> Date: Thu, 16 May 2019 11:00:50 +0200
> 
> > Thanks, but I don't think I understand the advantages of this approach
> > vs the current one.  Concretely, why would we want not to inherit
> > from the 'default' face?
> 
> Indeed, there is no reason not to inherit from the default face.  In
> fact, what I suggested was making the inheritance _always hold_, i.e.,
> despite of the user's definitions of the line number faces.  Which
> brings us to the next point you brought up...
> 
> > Also, doesn't your change force the line-number face to change
> > together with 'default', even if the user defines the face to not
> > inherit from 'default'?  With the current code, users are free to
> > define the face without inheritance, and that will stop update the
> > line-number face together with 'default', e.g. when the user enlarges
> > the default face's font or makes it smaller.  With your proposal, the
> > size changes in 'default' will always be propagated to line-number,
> > right?
> 
> I agree with you.  In fact, this is exactly the reason why I had
> suggested the change -- I was using a theme were the line number faces
> were not set to inherit from default, and realized that
> text-scale-adjust does not affect them.
> 
> > And finally, if we do make the proposed change, shouldn't we stop
> > inheriting from 'default' at the same time?
> 
> Absolutely.
> 
> With all this being said, I agree with you that it is best to let the
> user choose whether he or she wants the line number faces to inherit
> from the default one.

So you agree that this bug should be closed without changing the
current code?

Thanks.

bug#35452: Line number faces should check for remapping of the default face

[Dario Gjorgjevski]

> [Forwarding to the bug tracker; please use Reply All in the future.]

Sorry for that.

> So you agree that this bug should be closed without changing the
> current code?
>
> Thanks.

Yes.  Thank you likewise.

-- 
Dario Gjorgjevski :: dario.gjorgjevski@gmail.com :: +389 (0)70 784 142

bug#35452: Line number faces should check for remapping of the default face

[Eli Zaretskii]

> From: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>
> Cc: 35452@debbugs.gnu.org
> Date: Thu, 16 May 2019 16:07:25 +0200
> 
> > So you agree that this bug should be closed without changing the
> > current code?
> >
> > Thanks.
> 
> Yes.  Thank you likewise.

Thanks, done.