[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: face-remapping patch
From: |
Stefan Monnier |
Subject: |
Re: face-remapping patch |
Date: |
Tue, 27 May 2008 22:54:55 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.0.60 (gnu/linux) |
> I've attached my face-remapping patch to this message (I needed to
> untangle it from some other changes).
> I've used regularly in my personal emacs for many years, and in light
> testing the patch seems to work fine against the current trunk.
It looks pretty good. Is there any reason why you added redundant tests
like !NILP (Vface_remapping_alist)? I'd expect at least in my case that
face-remapping-alist would almost never be nil so this extra test would
end up redundant.
Stefan
> +int
> +lookup_named_face (f, symbol, signal_p)
> + struct frame *f;
> + Lisp_Object symbol;
> + int signal_p;
> +{
> + return lookup_named_face_1 (f, symbol, 0);
> +}
Why have a `signal_p' arg that's not used?
Also, the ChangeLog entry doesn't make much sense:
(lookup_named_face_1): Renamed from `lookup_named_face'. Add
`signal_p' argument. Pass new last arg to `get_lface_attributes', and
return -1 if it fails.
(lookup_named_face): Redefined in terms of `lookup_named_face_1'.
The signal_p arg was there before, it's not added. Am I missing something?
> + switch (face_id)
> + {
> + case DEFAULT_FACE_ID: name = Qdefault; break;
> + case MODE_LINE_FACE_ID: name = Qmode_line; break;
> + case MODE_LINE_INACTIVE_FACE_ID: name = Qmode_line_inactive; break;
> + case HEADER_LINE_FACE_ID: name = Qheader_line;
> break;
> + case TOOL_BAR_FACE_ID: name = Qtool_bar; break;
> + case FRINGE_FACE_ID: name = Qfringe; break;
> + case SCROLL_BAR_FACE_ID: name = Qscroll_bar; break;
> + case BORDER_FACE_ID: name = Qborder; break;
> + case CURSOR_FACE_ID: name = Qcursor; break;
> + case MOUSE_FACE_ID: name = Qmouse;
> break;
> + case MENU_FACE_ID: name = Qmenu;
> break;
> +
> + default:
> + return face_id; /* Give up. */
Does this default case correspond to a bug? if so shouldn't it `abort'?
If not, shouldn't it obey Vface_remapping_alist?
> + mapping = assq_no_quit (name, Vface_remapping_alist);
> + if (NILP (mapping))
> + return face_id; /* Give up. */
> +
> + remapped_face_id = lookup_named_face_1 (f, name, 0);
This looks odd: we look up Vface_remapping_alist and then ignore the
actual result (other than its being null or not). Is it just an
optimization to avoid calling lookup_named_face_1? If so, a comment is
in order.
- face-remapping patch, Miles Bader, 2008/05/26
- Re: face-remapping patch, Florian Beck, 2008/05/27
- Re: face-remapping patch,
Stefan Monnier <=
- Re: face-remapping patch, Miles Bader, 2008/05/27
- Re: face-remapping patch, David Reitter, 2008/05/28
- Re: face-remapping patch, Miles Bader, 2008/05/28
- Re: face-remapping patch, Miles Bader, 2008/05/28
- Re: face-remapping patch, David Kastrup, 2008/05/28
- Re: face-remapping patch, Miles Bader, 2008/05/28
- Re: face-remapping patch, David Kastrup, 2008/05/28
- Re: face-remapping patch, David Reitter, 2008/05/28
- Re: face-remapping patch, Miles Bader, 2008/05/28
- Re: face-remapping patch, David Reitter, 2008/05/28