Re: x-display-pixel-width/height inconsistency

[Eli Zaretskii]

> Date: Fri, 10 May 2013 15:00:33 +0900
> From: YAMAMOTO Mitsuharu <address@hidden>
> Cc: address@hidden
> 
> > It makes me nervous to have consing inside a Win32 API callback.  Are
> > we even sure the callback gets called in the context of our main
> > tread?  If not, and if consing causes GC, we could blow up the stack.
> 
> If the callback can be called from the thread/context other than the
> one for EnumDisplayMonitors and that can cause memory synchronization
> issue, then API documentation should clearly say so, because such kind
> of problem can happen even with other data structures.

Well, we can sue MS, but that won't help us, I think.

Seriously: while debugging Emacs on Windows, I frequently see messages
from GDB about new threads being launched, which I know for sure Emacs
didn't do directly.  I have no idea why they are started.  For
example, calling GetOpenFileName certainly starts a new thread, but
the MSDN documentation says nothing about that.  (We've had problems
with the stack size of that thread, see bug #13065.)  We define a
callback for that API, and I suspect that callback is run in the
context of that additional thread.  I don't know if the API you are
using is different in this regard, but I prefer not to risk that, if
we can avoid that risk.

> And it is not Fcons but Feval that can cause GC.

Thank you for your lecture.

> > And actually, I don't see why you need the list of monitor HMONITOR
> > handles, you get them from MonitorFromWindow anyway.  All you need is
> > the count of the monitors, and then you can create the vector in
> > monitor_frames by simply inserting every monitor you get from
> > MonitorFromWindow, and leave the rest at nil.  Or am I missing
> > something?
> 
> > So could we please get rid of the consing inside the callback, and
> > just count them instead?
> 
> The display-monitor-function lists all the monitors regardless of the
> existence of Emacs frames in it.

I know that, but you end up with just nil in the list of frames for
those monitors that don't have frames, right?  So just use the
callback to count the monitors; then you don't need to cons a list
inside the callback, you only need to increment a counter.  The cells
for the monitors that don't have frames just need to be filled with
nil.

> >> +       if (FRAME_W32_P (f) && FRAME_W32_DISPLAY_INFO (f) == dpyinfo
> >> +    && !EQ (frame, tip_frame))
> 
> > The test against dpyinfo is redundant, it will always be true: there's
> > only one dpyinfo on Windows.  I would like to remove it, because it's
> > confusing.  (There's one more such test in another place in the
> > patch.)
> 
> I'm aware of that.  It's just for resemblance to X11 code.  I'd rather
> leave it to make it easier to compare with the code for X11 if it were
> for the Mac port, but I don't have a strong opinion for W32.

The w32 display doesn't support multiple display terminals, and
probably never will.  Even if it does some day support that, I doubt
that the infrastructure for that will be similar to X.

So please remove that.

> >> +       name = make_unibyte_string (mi.szDevice, strlen (mi.szDevice));
> 
> > Why make_unibyte_string?  Is szDevice documented to be an ASCII-only
> > string?
> 
> make_unibyte_string does not assume that the passed bytes are only of
> ASCII characters.

Any other string will get users in trouble, because this will produce
a unibyte string, something Lisp programs generally don't expect to
get when the string is supposed to be human-readable text.

> Of course, you can decode the string if its encoding is known (I'm not
> sure how it is encoded because I'm not familiar with W32).

You want DECODE_SYSTEM.  It knows about the encoding of strings in the
current session.

> But you should be careful because code conversions can involve GC in
> general unlike Fcons.

Then use GCPRO (although I don't think it's needed in this case).
It's not a big deal.

> Alternatively, as the `name' attribute is optional, you can remove
> it entirely if you don't like make_unibyte_string.

I see no reason to remove it due to such a minor and easily solvable
issue.

Thanks.