Re: [1/2] winex11: Cache XRandR 1.0 display mode. (try 2)
On 05/10/2013 10:51 AM, Ruslan Kabatsayev wrote: Hi, Won't such code fail if the mode is changed outside wine? Regards, Ruslan Unfortunately it will, but the only way to fix that is to listen for an XRandR event indicating that the mode has changed. The screen_width/screen_height values currently don't update if the mode is changed outside Wine either, so AJ suggested that I could simply cache the XRandR mode without worrying about what happens if the mode changes outside of Wine.
Re: [1/3] winex11: Select for XRandR screen change events and implement an event handler.
On 05/10/2013 09:11 AM, Alexandre Julliard wrote: Sam Edwards writes: Yeah, I don't call it until patch 2 in the series. This patch just introduces it without calling it, which does cause a warning. (I hope this doesn't violate the "atomic patches" rule.) It does, you can't add dead code (or warnings for that matter). So, if I were to give this patch a try 2, I'd have to call the event handler somewhere (probably right after selecting for the resize event)? I'll keep that in mind for the future. :) But no, you can't select for events on the gdi display; actually I don't think you want to do it this way at all. So gdi_display should not receive events? I'm guessing this has something to do with the fact that Wine opens one Xlib Display per thread, so the XRandR resize events need to be pulled down on one of the thread-local X connections, rather than the master gdi_display. (I'm still not sure why Wine does it this way. Are certain versions of Xlib not thread-safe?) Most window events need to be handled in the thread that owns the window. Pulling them from a global connection in a different thread would only cause extra context switches and potential deadlocks. This is kind of a special case because RRScreenChangeNotify is a root window event, so there's no "thread that owns the window" there (unless Wine has some internal representation for the X11 root window, and a thread that owns that representation, that I'm not aware of). Handling external resizes should be done in the desktop process, and most likely involves the wineserver too. The event handler does call X11DRV_resize_desktop, which messages the desktop process with the new size. But, I can see how it would be cleaner to put the event checker in the same process as well, since that way the XRandR resize doesn't have to get processed once for every process on the system. How would this involve the wineserver, though? It looks like we already have the infrastructure for synchronizing screen_width and screen_height changes from one process to another. Not really, only the process that triggered the change gets properly updated at the moment. The Mac driver tries to do it by broadcasting a message, but that doesn't update processes that currently don't own a window. There are also race conditions, and various issues with window surfaces and DCE regions. Resizing the desktop is a tricky problem. Aha; this is why the wineserver is necessary: So that applications that haven't created any windows can receive the update. As far as XRandr is concerned, at this point you could probably simply cache the mode and don't bother to update except when the app triggers a change, since that's what we do for the rest of the desktop parameters anyway. Thanks for the suggestion. That sounds like it has a much smaller impact, so I think I'll use that approach instead. Until then, I'll withdraw this patch. Thanks, Sam
Re: [1/3] winex11: Select for XRandR screen change events and implement an event handler.
On 05/09/2013 12:36 PM, Alexandre Julliard wrote: It's okay only because you are not actually calling it ;-) Yeah, I don't call it until patch 2 in the series. This patch just introduces it without calling it, which does cause a warning. (I hope this doesn't violate the "atomic patches" rule.) But no, you can't select for events on the gdi display; actually I don't think you want to do it this way at all. So gdi_display should not receive events? I'm guessing this has something to do with the fact that Wine opens one Xlib Display per thread, so the XRandR resize events need to be pulled down on one of the thread-local X connections, rather than the master gdi_display. (I'm still not sure why Wine does it this way. Are certain versions of Xlib not thread-safe?) Handling external resizes should be done in the desktop process, and most likely involves the wineserver too. The event handler does call X11DRV_resize_desktop, which messages the desktop process with the new size. But, I can see how it would be cleaner to put the event checker in the same process as well, since that way the XRandR resize doesn't have to get processed once for every process on the system. How would this involve the wineserver, though? It looks like we already have the infrastructure for synchronizing screen_width and screen_height changes from one process to another. At any rate, it makes sense to keep winex11 and winemac in sync. If Ken's resize handling isn't proper, we should decide how to proceed now, so that both can be fixed in the same way. It certainly won't be an easy task. That's fine; this is the last issue that I need to resolve before Wine is pretty much "bug free" for me, so I'm more than happy to put in the extra effort. :)
Re: [PATCH] dlls/gdi32/freetype.c: A better divide by zero fix, report bad fonts.
On 05/06/2013 03:05 PM, Max TenEyck Woodbury wrote:
Just to make this clear, the most recent version of this patch is such
a graceful handling, right?
I haven't worked on gdi32/freetype.c much, so I wouldn't be the one to
say for sure (you should probably talk to Alexandre Julliard, Dmitry
Timoshkov, or Huw Davies), but I'll give you my thoughts anyway:
On 05/05/2013 12:48 PM, m...@mtew.isa-geek.net wrote:
+ /* HACKHACK: If a font has tmHeight=0, let us know. */
+ if (!font->potm->otmTextMetrics.tmHeight) {
+ ERR("Font named '%s' has tmHeight=0, aveWidth=%d!\n",
font->ft_face->family_name, font->aveWidth);
+ }
+
Since we've learned that tmHeight=0 is not actually an error, it doesn't
make sense to log it using ERR(...). I would suggest a TRACE(...) or
(preferably) nothing at all. Also, when I wrote the ERR message patch
for you, it was intended only to help you locate the problematic font
and wasn't meant to be included in upstream Wine. (Which is why I marked
it with HACKHACK and didn't bother to use debugstr_a.)
As for the actual check, it makes sense to consider what the math is
trying to do: If aveWidth is extremely large (relative to tmHeight),
then assume aveWidth is wrong and set it to 0. If tmHeight is 0, then it
seems reasonable to assume that aveWidth isn't valid either.
I'm not sure why the sanity check cares to do this:
(aveWidth+tmHeight-1) / tmHeight > 100
When it can just do this:
(aveWidth+tmHeight-1) / tmHeight > 100
= (aveWidth-1)/tmHeight + 1 > 100
= (aveWidth-1)/tmHeight > 99
= (aveWidth-1) > tmHeight*99
= aveWidth > tmHeight*99 + 1
≈ aveWidth > tmHeight*100
This allows us to write out tmHeight just once, and is also much easier
to understand at a glance.
So, I would just simplify the whole sanity check to something like:
/* Make sure that the font has sane width/height ratio */
if (font->aveWidth > font->potm->otmTextMetrics.tmHeight * 100)
{
WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth);
font->aveWidth = 0;
}
But even then, the sanity check itself sounds like a workaround for a
much deeper bug. Maybe add a "FIXME: Investigate why this is necessary"
comment while you're at it?
Best,
Sam
Re: [PATCH] dlls/gdi32/fretype.c: Avoid division by zero.
On 05/04/2013 10:38 PM, Max TenEyck Woodbury wrote:
$ grep ' has tmHeight=0, aveWidth=' 201305055-make-test-native.log
err:font:get_text_metrics Font named 'Emmentaler-Brace' has
tmHeight=0, aveWidth=3!
err:font:get_text_metrics Font named 'Emmentaler-Brace' has
tmHeight=0, aveWidth=3!
err:font:get_text_metrics Font named 'Emmentaler-Brace' has
tmHeight=0, aveWidth=3!
err:font:get_text_metrics Font named 'Emmentaler-Brace' has
tmHeight=0, aveWidth=3!
err:font:get_text_metrics Font named 'RiordonFancy' has tmHeight=0,
aveWidth=0!
The Emmentaler-Brace font is part of the LilyPond music engraving
program, so I was able to track it down without too much effort.
On both Windows (XP) and Wine, the .otf loads without any trouble. When
calling CreateFontIndirect with lfHeight of 1-9, the
tm{Height,Ascent,Descent} are, in fact, 0 on both platforms. lfHeight
values of 10 or greater produce nonzero values for
tm{Height,Ascent,Descent}.
So, Wine's font loader seems to be working appropriately and thus the
sane thing to do is to make sure tmHeight=0 is handled gracefully.
As for RiordonFancy, I don't know, but aveWidth=0 in that case, so that
doesn't really affect us here.
Re: [PATCH] dlls/gdi32/fretype.c: Avoid division by zero.
On 05/04/2013 08:27 PM, Max TenEyck Woodbury wrote:
OK. Let's summarize:
There are some fonts where tmHeight is in fact 0. If Hin-Tak Leang is
correct, these may be Open-Source fonts possibly with proprietary
equivalents. Since I have hundreds of fonts installed on my system, it
is almost certain that I have one or more. Identifying which without
support in wine is a large task, not to be undertaken lightly.
Let's try to fully understand this problem and identify a font that
causes the issue. Since you already have one installed in your font
library, you could just run the tests with the attached patch applied
and make quick work of identifying the troublesome font. Maybe
(hopefully) it's an easy font to obtain so all of us can get a better
look at what's going on.
I'm going to *guess* that tmHeight being 0 is the actual problem, but we
won't know until we try the same font on Windows and see what Windows
does. If Windows also produces tmHeight=0, then this patch makes perfect
sense.
If Windows gives a non-zero tmHeight under the same circumstances, then
we know the problem is in the font loader, and we'll fix that instead,
thus making this patch unnecessary because tmHeight will always be
nonzero anyway.
>From f5dc537a3799b9c4507cb0c8aefcc4ee73d2a0b5 Mon Sep 17 00:00:00 2001
From: Sam Edwards
Date: Sat, 4 May 2013 21:58:19 -0600
Subject: gdi32: [HACK] Print which font is causing division-by-zero problems.
---
dlls/gdi32/freetype.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c
index c30a358..2547092 100644
--- a/dlls/gdi32/freetype.c
+++ b/dlls/gdi32/freetype.c
@@ -6772,6 +6772,12 @@ static BOOL get_text_metrics(GdiFont *font, LPTEXTMETRICW ptm)
{
if (!get_outline_text_metrics(font) && !get_bitmap_text_metrics(font)) return FALSE;
+/* HACKHACK: If a font has tmHeight=0, let us know. */
+if (!font->potm->otmTextMetrics.tmHeight)
+{
+ERR("Font named '%s' has tmHeight=0, aveWidth=%d!\n", font->ft_face->family_name, font->aveWidth);
+}
+
/* Make sure that the font has sane width/height ratio */
if (font->aveWidth)
{
--
1.7.10.4
Re: [PATCH] dlls/gdi32/fretype.c: Avoid division by zero.
On 05/04/2013 05:13 PM, Max TenEyck Woodbury wrote: Having wine throw an exception where it did not do so before is another kind of problem indicator. One that hardly needs a special conformance test. Hmm. As I suspected, that is a single point pass only test. It does not explore any of the possible fail conditions. Thus, it is also definitely a possibility that Windows makes no such sanity check. I suggested the conformance test for removing the sanity check, not for the crash. We need to know for sure whether Windows makes no such sanity check before we act on that possibility. Almost certainly. Without this patch, the 'make test' suit throws a divide by zero exception and brings up a dialog box. With this patch, it does not. I believe that is sufficient to convict the unpatched code of having something wrong with it. The flaw in your logic is that you're assuming the code which generates the exception is necessarily the code that's written incorrectly. By the same logic, you could patch out the crash handler and say "Look, no more crash dialog! It's fixed!" I agree that the unpatched code is definitely dividing by zero in this case; but according to Dmitry, tmHeight is never supposed to be 0. If he's correct, it means that this code is correct (given the assumption), but something else is bugged (thereby violating that "contract of behavior") and that's what should be fixed instead. As Dmitry has said, tmHeight should never be 0, so it's probably valid to assume tmHeight!=0. But that assumption can be checked. Currently there is no such check. 'Should' in this context is a very bad word and has no place at the foundation of an argument about what actually happens. I agree that this assumption can/should be checked, but the appropriate place to do that is in unit tests. This also verifies that the assumption holds on Windows as well, ensuring that Wine's behavior doesn't diverge from that of Windows. And yeah, sorry for using "should" - it is kind of a weak word, in retrospect... I think "must" would have been a better choice, but I'm not certain enough for a word so strong: We need to see what tmHeight does on Windows (that means tests!) before we can argue it one way or another. :) The bug may instead be in allowing the font to load with tmHeight=0, and this is merely the first crash that the problem causes for you. But what about apps that divide by tmHeight under the same assumption? We can't change those, so it's better to fix tmHeight. If the APP does the division, that is the APP's problem. Wine, on the other hand should not throw an exception, and it did NOT do so until recently. Whoever wrote the new code (Dmitry?) should have put in the check or made the test work without doing a division. The fact that it fails is the problem being addressed. If the app, which was made for Windows, works on Windows, but not on Wine, then it is our problem, not the app's. The crash that you're seeing is a clear indicator of *something* being wrong, but we don't yet know specifically what. I have no objection to someone writing an alternative patch and backing this one out when that patch goes in, but until then, this patch, or something like it, needs to be applied. With wine throwing the exception, some Apps are going to fail that would not fail otherwise. That is definitely a 'Bad Thing' and should be fixed ASAP (like right now)! Crashes are definitely a Bad Thing(TM), but this patch may not fix all of them (e.g. with apps that assume tmHeight!=0) without actually fixing the underlying problem. In fact, this patch itself could be a Bad Thing(TM) -- if something is wrong with tmHeight calculation/scaling, the patch might only mask the real issue, which would make a proper fix take even longer. - I would like to reiterate: I simply don't know what tmHeight is supposed to do. According to Dmitry, the real bug is having tmHeight=0. If Windows allows tmHeight=0 under some circumstance, then your patch is the right thing to do, because we must prevent a division by zero in that case. If Windows absolutely disallows tmHeight=0, then the bug is actually that Wine allows tmHeight=0, and so we should be focusing on that instead. Hope this helps, Sam
Re: [PATCH] dlls/gdi32/fretype.c: Avoid division by zero.
On 05/04/2013 12:59 PM, Max TenEyck Woodbury wrote: You are trying to make this about me. It is not. Windows fairly obviously does not do this 'sanity' test. Wine is supposed to imitate windows. To do this absolutely correctly, the whole 'sanity' test should go away. This sounds like an excellent reason to write a conformance test. A test that succeeds on Windows but fails under Wine is a great way to convince everyone that the patch is necessary. It's also gives us a closer look at Windows's behavior under the same circumstances, so we can see whether Windows does this sanity check or not, and if not, how it reacts when aveWidth is wrong. I should note, the commit that introduces the sanity check (21589993826cdb1cb2f87ceb94c8a188bd4a3090) also includes a test (dlls/gdi32/tests/font.c:3908 as of this writing) that passes under Windows, which could mean that Windows actually does include this sanity check for the width vs. the height. I isolated the problem, and came up with a fix. Bug reports are for cases where you can not yourself identify the bad code. That this code is bad is obvious when you know that it can throw an exception. The only investigation absolutely needed is to report the occurrence of the exception. It happens in at least some circumstances. Anything additional is simply an invitation to delay. Are we sure that *this* code is the problem? As Dmitry has said, tmHeight should never be 0, so it's probably valid to assume tmHeight!=0. The bug may instead be in allowing the font to load with tmHeight=0, and this is merely the first crash that the problem causes for you. But what about apps that divide by tmHeight under the same assumption? We can't change those, so it's better to fix tmHeight. Are delays necessarily a bad thing? This bug doesn't have any security implications, and we aren't hurrying to catch the Wine 1.6 release window. We can afford to take the extra time to ensure the quality of the patch. :)
Re: winex11.drv
On 05/01/2013 05:38 AM, Christopher Cope wrote: On 05/01/2013 07:21 AM, Christopher Cope wrote: I am unsure how the "enum x11drv_atoms" in the file dlls/winex11.dev/x11drv.h works. My /usr/include/Xatom.h defines XA_LAST_PREDEFINED as 68. Presumably, therefore XATOM_Rel_X and XATOM_Rel_X should be 81 and 82 respectively. However, when I dump the values I get 195 and 196. I am attempting to add XATOM_Abs_X and XATOM_Abs_X with values of 480 and 481 for a patch I'm working on. I could simply define the values elsewhere, but I assumed I should try to make it blend with the other code as much as possible. Any help is appreciated thank you. I think I may have it figured out. Is it just a place holder until XInternAtoms returns the appropriate values? Looks like; x11drv_main.c contains the X11DRV_Atoms and atom_names arrays, and on line 569 it populates X11DRV_Atoms with the X server's correct atoms. So, the purpose of a value in enum x11drv_atoms is only to serve as a key into the X11DRV_Atoms array to look up the real atom.
Re: winex11.drv
On 05/01/2013 05:21 AM, Christopher Cope wrote:
I am unsure how the "enum x11drv_atoms" in the file
dlls/winex11.dev/x11drv.h works. My /usr/include/Xatom.h defines
XA_LAST_PREDEFINED as 68. Presumably, therefore XATOM_Rel_X and
XATOM_Rel_X should be 81 and 82 respectively. However, when I dump the
values I get 195 and 196. I am attempting to add XATOM_Abs_X and
XATOM_Abs_X with values of 480 and 481 for a patch I'm working on. I
could simply define the values elsewhere, but I assumed I should try
to make it blend with the other code as much as possible. Any help is
appreciated thank you.
I've written a quick C program to print those enums (it's attached;
compile with "gcc -o xatomdump xatomdump.c -I wine/dlls/winex11.drv -I
wine/include") and I get this output:
FIRST_XATOM=69
XATOM_Rel_X=81
XATOM_Rel_Y=82
So, your guess is right, the Rel_X and Rel_Y values are (supposed to be)
81/82. Maybe your compiler is doing something strange or looked at the
wrong values by mistake?
Hope this helps,
Sam
#include
#include "config.h"
#include "x11drv.h"
void _printatom(const char *name, enum x11drv_atoms value)
{
printf("%s=%d\n", name, value);
}
#define printatom(atom) _printatom(#atom, atom)
int main(int argc, char *argv[])
{
printatom(FIRST_XATOM);
printatom(XATOM_Rel_X);
printatom(XATOM_Rel_Y);
return 0;
}
Re: [PATCH 2/2] gdi32: Clip font glyphs to fit within text metrics. (try 3)
On 04/30/2013 11:05 AM, Alexandre Julliard wrote: It still crashes: Hmm... Guess I didn't fully understand the underlying problem. Fortunately, after enabling subpixel rendering on my system, the same crash now happens for me, so I can figure out exactly what's breaking. I'll try to get try 4 sent in either today or tomorrow, but it depends how long it takes me to understand this bug (and any other impact it may have that we're not seeing) completely. Thanks, Sam
Re: [PATCH 1/2] gdi32: Clip font glyphs to fit within text metrics. (try 2)
On 04/29/2013 08:20 AM, Alexandre Julliard wrote: Well, there was no backtrace since winedbg crashes. Fortunately gdb works: Big thanks! I found the problem (it only happened with subpixel rendering, which I don't have enabled) and am resending the patches now. :)
Re: [PATCH 1/2] gdi32: Clip font glyphs to fit within text metrics. (try 2)
On 04/26/2013 08:17 AM, Michael Stefaniuc wrote: Sam, look at the size of the address. It is the 64bit build that crashes. bye michael Michael: Thanks for the advice; I noticed that too and have been running both 64-bit and 32-bit tests. (Although, it's perplexing to me that a change like this would affect 64-bit and not 32-bit.) AJ: I can't reproduce the crash at all. I spoke with Marcus on #winehackers last night and he found no test failures with the patch either. Could you provide a backtrace on the crash, or at least run the failing test with WINETEST_REPORT_SUCCESS=1 to give me more information on where/how it's dying? Thanks, Sam
Re: [PATCH 1/2] gdi32: Clip font glyphs to fit within text metrics. (try 2)
On 04/25/2013 12:28 PM, Alexandre Julliard wrote: It doesn't work here: ../../../../wine/tools/runtest -q -P wine -M comctl32.dll -T ../../.. -p comctl32_test.exe.so ../../../../wine/dlls/comctl32/tests/listview.c && touch listview.ok wine: Unhandled page fault on write access to 0x400053b34 at address 0x2b4c3d69e6b7 (thread 0398), starting debugger... winedbg: Internal crash at 0x2b53f78066b7 make: *** [listview.ok] Error 84 Thanks for the heads-up, AJ. I can't reproduce this problem, and it's strange that the debugger itself suffered an internal crash as well. Can anyone else verify? Did something mess up during AJ's build? Thanks for the help, Sam
Re: [PATCH 1/2] gdi32: Clip font glyphs to fit within text metrics. (try 2)
This series has been waiting in the queue for a few days now. Does this need further discussion, or can it be committed? Thanks, Sam On 04/20/2013 08:08 AM, Sam Edwards wrote:
Re: [PATCH 2/2] gdi32: Clip font glyphs to fit within text metrics.
On 04/23/2013 02:04 AM, Ken Thomases wrote:
Hmm. I assumed that ascent+descent+leading equals the distance between
baselines. In a Mac-native text editor, the strokes of one line of
Zapfino can extend into the lines above or below, which is why I
figured they exceeded the ascent and descent values. Notepad doesn't
let the lines get that close, though. So, something else must be going
on. I guess this isn't relevant to the question at issue with your
patch. Never mind. ;) -Ken
Apple and Microsoft have different definitions of "ascent" and "descent"
- so you may very well be right about how Apple does it, but the OS/2
table contains "typo" ascent/descent and "win" ascent/descent... I think
"typo" ascent/descent is used similar to what you described, but "win"
ascent/descent (which is what is retrieved through GetTextMetrics on
Windows) is for maximum/minimum glyph size. I may be wrong, so if there
are any typography experts reading this, please speak up. :)
Regardless, we see this in freetype.c:
if(font->yMax) {
TM.tmAscent = font->yMax;
TM.tmDescent = -font->yMin;
TM.tmInternalLeading = (TM.tmAscent + TM.tmDescent) -
ft_face->size->metrics.y_ppem;
} else {
TM.tmAscent = SCALE_Y(ascent);
TM.tmDescent = SCALE_Y(descent);
TM.tmInternalLeading = SCALE_Y(ascent + descent -
ft_face->units_per_EM);
}
So it's clearly the intent of tmAscent and tmDescent to contain yMax and
-yMin (if available) and so I feel that it's safe to clip the fonts,
because it should only affect improperly-made fonts in the first place,
and it's Windows's native behavior to clip accordingly.
(My head is spinning from all of this typography work ...)
And yeah, Wine's Notepad does keep the lines spaced well apart. I don't
(and can't) know if that's the case on Windows or not, but it might be
worth looking into later.
Thanks,
Sam
Re: [PATCH 2/2] gdi32: Clip font glyphs to fit within text metrics.
On 04/22/2013 07:08 PM, Ken Thomases wrote: I can't speak to how Windows would render it, but one such font is Zapfino. It's an "exuberant" calligraphic font with lots of flourishes, some of which have strokes extending into the line above or below. http://en.wikipedia.org/wiki/Zapfino https://www.google.com/images?q=zapfino -Ken I have never known about this font -- very stylistic indeed. :) While there are plenty of strokes that go well above the cap height or below the baseline, the Win Ascent and Win Descent values (which are defined as yMax for all glyphs and -yMin for all glyphs, respectively) in the metric tables are valid - none of the glyphs violate these limits, so the patch doesn't affect this (correctly-formatted) font. I loaded it up in Wine's Notepad, both with and without my patch, and took screenshots of the output: http://cfsworks.com/files/images/screenshots/zapfino_wine_unpatched.png http://cfsworks.com/files/images/screenshots/zapfino_wine_patched.png I even did a pixel-by-pixel comparison: they render the same! Sadly I can't make any comparisons to how Windows would handle the font. My Win XP VM complains about the ttf being corrupt. Is there a similar font that will work on Windows that I could try? Thanks, Sam
Re: [PATCH 2/2] gdi32: Clip font glyphs to fit within text metrics.
On 04/19/2013 10:32 AM, Max TenEyck Woodbury wrote: As I understand it, some fonts deliberately have glyphs larger than their metrics bounding boxes. Clipping them is almost certainly not a good idea. Forgive my disbelief, but can you provide an example? It seems like Windows has the same clipping behavior (see my test http://source.winehq.org/patches/data/95792). From my understanding, the intent of the ascent metric is that it indicates the maximum ascender on any glyph in the font (and likewise for descent), so the only real reason for the ascent/descent metrics to be wrong is if the font designer made a mistake. (And some tools, like FontForge, will automatically set the ascent/descent metrics correctly for you on export.) I can't think of any reason why a font author would want to create a font with an invalid ascent/descent metric. P.S. This is a resend because I accidentally sent this to wine-patches the first time... Sorry! That was dumb of me. :)
Re: [PATCH 3/4] d3d9/tests: Verify window style after exiting fullscreen mode. (try 2)
On 04/01/2013 08:16 AM, Henri Verbeet wrote: I think it's ok for the tests to be todo_wine, but we do want ddraw coverage for this in the tests. Fine by me. But that will take a few days since I'll have to familiarize myself with ddraw first. :) In the meantime, should I resend this patchset (with the reset_device() change included) so that it can be reviewed and committed while I'm busy with ddraw (and send the ddraw tests in a separate patch), or is this patchset unacceptable until I include the ddraw tests? Thanks, Sam
Re: wined3d: Properly handle WS_VISIBLE and WS_EX_TOPMOST style flags on fullscreen windows.
On 03/29/2013 06:16 AM, Henri Verbeet wrote: I think the idea is basically ok, but I do have some comments: I think test_window_style() is a more appropriate place for the tests, and you should add them to the variants in ddraw as well. Ddraw applications in particular are very fragile in this regard. Note that the test_window_style() tests are fairly similar to what your tests do, except that they don't test restoring the window as such. That test also shows that we're actually not supposed to touch the styles at all, but that's hard to make work without calling into winex11. Ack, you're right! I saw the test_window_style() but second-guessed myself and put it into test_reset() instead... That was foolish of me. Thanks. -SetWindowPos(window, HWND_TOP, 0, 0, w, h, SWP_FRAMECHANGED | SWP_SHOWWINDOW | SWP_NOACTIVATE); +SetWindowPos(window, HWND_TOPMOST, 0, 0, w, h, SWP_FRAMECHANGED | SWP_SHOWWINDOW | SWP_NOACTIVATE); This is really a separate change. I'm a bit confused, then: Without this change, the exstyle test breaks, and the guidelines request that every patch be atomic. They also request that all tests be included in the same patch.What is the proper procedure for this when it comes to multipatch patchsets? Off the top of my head, I would guess it's one of these: 1. The "all tests in the same patch" rule is only appropriate for single-patch submissions, so in multipatch patchsets, it's acceptable to include a final patch with nothing but tests. 2. Only introduce the tests in the last patch of the patchset. (That is, add them all at once, but only when all of the necessary changes have been made to allow the tests to work first.) 3. Include each test in the first patch that will allow the test to pass. (That is, add them incrementally.) 4. Add the tests as todo_wine in the first patch, then remove the todo_wine in later patches as each test becomes functional. (That is, add them all at once as todo_wine, and mark them as non-todo incrementally.) +device->style ^= (device->style^style) & WS_VISIBLE; Spaces around the (second) ^, please. Done. :) Thank you, Sam
Re: [PATCH] d3d9/tests: Add test for IDirect3DDevice9_Reset with BackBufferWidth/Height = 0.
On Nov 6, 2012, at 3:45, Henri Verbeet wrote: > Yeah, we'll want a similar test for d3d8. Okay, I'll write that as soon as the d3d9 test is accepted. > You're probably looking for AdjustWindowRect() and SetWindowPos(). Thank you, I didn't know about AdjustWindowRect(), but why SetWindowPos()? Is this preferred over MoveWindow() for any reason? > Testing the viewport dimensions is useful, but it probably wouldn't > hurt to check the swapchain's presentation parameters and the > backbuffer's surface desc as well. Good catch, I forgot to copy the relevant code for that. Thanks, Sam
