Stefan Leichter Stefan.Leichter@camline.com wrote:
- BYTE buffer[MAX_PATH];
This should be WCHAR if you decided to test unicode version of the API (although testing ANSI variant would be much easier), that will help to avoid all those casts, and protect from insufficient buffer size. Also please use a font that exists in Wine, such as sserife.fon, but be prepared that it could be not found since font file name is locale dependent.
Monday 17 December 2012 Dmitry Timoshkov dmitry@baikal.ru
Stefan Leichter Stefan.Leichter@camline.com wrote:
- BYTE buffer[MAX_PATH];
This should be WCHAR if you decided to test unicode version of the API (although testing ANSI variant would be much easier), that will help to avoid all those casts, and protect from insufficient buffer size. Also please use a font that exists in Wine, such as sserife.fon, but be prepared that it could be not found since font file name is locale dependent.
First of all: The issues your are pointing out have been in the previous version of the patch too. Why didn't you complain in your first review. To my understanding this is a wast of my time complaining about two issues and keeping the rest secret for the next version of the patch.
Now to the technical stuff: - i agree to change the data type to WCHAR - what you mean with "protect from insufficient buffer size" it totaly unclear to me - the tests are written for a font in %windor%\fonts. This directory is empty after a clean .wine directory was created. Changing the font does not help at all.
Stefan Leichter Stefan.Leichter@camline.com wrote:
First of all: The issues your are pointing out have been in the previous version of the patch too. Why didn't you complain in your first review. To my understanding this is a wast of my time complaining about two issues and keeping the rest secret for the next version of the patch.
It's often happens with subsequent reviews, and there is nothing magic or secret about it: first review was about the whole approach while next one is more about the actual details.
Now to the technical stuff:
- i agree to change the data type to WCHAR
- what you mean with "protect from insufficient buffer size" it totaly unclear to
me
I intentionally left 'BYTE buffer[MAX_PATH];' in the quote, think about it.
- the tests are written for a font in %windor%\fonts. This directory is empty
after a clean .wine directory was created. Changing the font does not help at all.
But it will once Wine starts to copy its own fonts there at some point.
Saturday 29 December 2012 Dmitry Timoshkov dmitry@baikal.ru
Stefan Leichter Stefan.Leichter@camline.com wrote:
First of all: The issues your are pointing out have been in the previous version of the patch too. Why didn't you complain in your first review. To my understanding this is a wast of my time complaining about two issues and keeping the rest secret for the next version of the patch.
It's often happens with subsequent reviews, and there is nothing magic or secret about it: first review was about the whole approach while next one is more about the actual details.
But the "subsequent review" is burning my time without need.
- the tests are written for a font in %windor%\fonts. This directory is
empty after a clean .wine directory was created. Changing the font does not help at all.
But it will once Wine starts to copy its own fonts there at some point.
To me this sounds more like an excuse than an argument. Maybe we will have an Arial font before this happens!
I will drop the patch with the tests an see what happens first. You may know the old idiom "A bird in the hand is worth two in the bush."
-- Stefan
Stefan Leichter Stefan.Leichter@camline.com wrote:
First of all: The issues your are pointing out have been in the previous version of the patch too. Why didn't you complain in your first review. To my understanding this is a wast of my time complaining about two issues and keeping the rest secret for the next version of the patch.
It's often happens with subsequent reviews, and there is nothing magic or secret about it: first review was about the whole approach while next one is more about the actual details.
But the "subsequent review" is burning my time without need.
I certainly can stop burning my time with reviewing your patches, but somehow I doubt that it will help with improving and accepting then. Besides I don't see that much of reviewers of font related patches.
- the tests are written for a font in %windor%\fonts. This directory is
empty after a clean .wine directory was created. Changing the font does not help at all.
But it will once Wine starts to copy its own fonts there at some point.
To me this sounds more like an excuse than an argument. Maybe we will have an Arial font before this happens!
Consider what is easier: 'cp ~/wine/fonts/sserife.fon ~/.wine/drive_c/windows/fonts' or find an installer for Arial, download and install it before running 'make test'. Even worse, installing Arial or any other external font may actually break the tests.
I will drop the patch with the tests an see what happens first. You may know the old idiom "A bird in the hand is worth two in the bush."
There always is a hope to get a real thing somewhere in the future instead of a broken thing right now.
-
Dmitry Timoshkov -
Stefan Leichter