On 28/08/07, Trent Waddington trent.waddington@gmail.com wrote:
On 8/28/07, H. Verbeet hverbeet@gmail.com wrote:
You should use the timing and frame sequence info from the .ani. Also, Xcursor actually supports proper animated cursors, so it would be better to handle the animation in winex11.drv
Can you please direct me to some example code of defining and using an animated cursor with Xcursor?
http://repo.or.cz/w/wine/hacks.git?a=blob;f=dlls/winex11.drv/mouse.c;h=b3443... should be a decent start.
As explained in the comment at the top of cursoricon.c, you can't do that, because it will break win16 applications.
Umm.. what comment would that be?
This one:
- Cursors and icons are stored in a global heap block, with the
- following layout:
- CURSORICONINFO info;
- BYTE[] ANDbits;
- BYTE[] XORbits;
- The bits structures are in the format of a device-dependent bitmap.
- This layout is very sub-optimal, as the bitmap bits are stored in
- the X client instead of in the server like other bitmaps; however,
- some programs (notably Paint Brush) expect to be able to manipulate
- the bits directly :-(
Should I replace hNext with a CURSORICONINFO *? This is what I had initially.. or should I be using something else? Or should I just make it an BYTE bFrames and append multiple data blocks to the end of the CURSORICONINFO struct?
Well, what it comes down to is that you can't change the cursor memory layout for win16 applications at all, which complicates things a bit.
On 8/28/07, H. Verbeet hverbeet@gmail.com wrote:
http://repo.or.cz/w/wine/hacks.git?a=blob;f=dlls/winex11.drv/mouse.c;h=b3443... should be a decent start.
heh, making me think this is a well beaten path.
Well, what it comes down to is that you can't change the cursor memory layout for win16 applications at all, which complicates things a bit.
Oh I see, fun. Well, I suppose putting the info after the bitmaps will do it.
Trent
I've fixed the style issues indicated.
I'm now using Xcursor's animated cursor support.
The delay information in the ANI is honored.
The changes to struct tagCURSORICONINFO are removed and, instead, I'm appending data after the bitmap info.. so Paint Brush can continue manipulating those bits directly.
Trent
I noticed LoadCursorFromFile() wasn't working, so here's a patch to fix that.
And a patch to fix some memory I was leaking.
Have tested 150+ animated cursors so far, all appear to display correctly.
Trent
For ease of review, you should send one cumulative patch, rather than patches that apply on top of one another. (I use separate git trees - one for development, one for merging - to make this easier.) The style fixes at least look good though.
--Juan
On 8/29/07, Juan Lang juan.lang@gmail.com wrote:
For ease of review, you should send one cumulative patch, rather than patches that apply on top of one another. (I use separate git trees - one for development, one for merging - to make this easier.) The style fixes at least look good though.
Ok, attached, and fixed an issue with setting delay on the last frame.
Trent
Ok, attached, and fixed an issue with setting delay on the last frame.
+static ANIHEADER *anih; +static LPBYTE *ani_frames; +static DWORD ani_frame_idx; + +static void decodeRIFF(LPBYTE bytes, DWORD size) +{ + CK *chunk = (CK*)bytes; + CKSIZE sizeWithPad = chunk->ckSize % 2 ? chunk->ckSize + 1 : chunk->ckSize; + switch(chunk->ckID) + { + case 0x46464952: /* RIFF */ + decodeRIFF(chunk->ckData + 4, chunk->ckSize - 4);
Like Henri already said, you really can't use globals for anih, ani_frames, and ani_frame_idx. These should be in a structure, and decodeRIFF should write into that structure, not into globals. The structure should then be associated with the icon.
+ HICON hNextIcon = CreateIconFromResourceEx( &bits[entry->dwDIBOffset], entry->dwDIBSize, TRUE, 0x00030000, 0, 0, 0 );
This may be obvious to other, but certainly not to me: what does 0x00030000 mean? You need to use symbolic constants here.
+ /*if (CURSORICONINFO_NEXT(info) && !animate_timer_started) { + animate_timer_started = TRUE; + SetTimer(NULL, 0, 55, animate_cursor); + }*/
Please don't include dead code. --Juan
On 28/08/07, Juan Lang juan.lang@gmail.com wrote:
This may be obvious to other, but certainly not to me: what does 0x00030000 mean? You need to use symbolic constants here.
It's a version number.
On 8/29/07, Juan Lang juan.lang@gmail.com wrote:
Like Henri already said, you really can't use globals for anih, ani_frames, and ani_frame_idx. These should be in a structure, and decodeRIFF should write into that structure, not into globals. The structure should then be associated with the icon.
They're temps.. I've added a comment to make that clear.
This may be obvious to other, but certainly not to me: what does 0x00030000 mean? You need to use symbolic constants here.
It's the version non-sense that Microsoft's api requires. You'll find that every call to this api and it's non-Ex variant have it.
Please don't include dead code.
Doh, sorry. Removed.
Trent
Like Henri already said, you really can't use globals for anih, ani_frames, and ani_frame_idx. These should be in a structure, and decodeRIFF should write into that structure, not into globals. The structure should then be associated with the icon.
They're temps.. I've added a comment to make that clear.
That doesn't make it better, it's not thread-safe. Pass them as parameters to decodeRIFF.
This may be obvious to other, but certainly not to me: what does 0x00030000 mean? You need to use symbolic constants here.
It's the version non-sense that Microsoft's api requires. You'll find that every call to this api and it's non-Ex variant have it.
I still say a symbolic constant is better, or at the least, a comment. A test case that shows that it's needed is even better. --Juan
On 8/29/07, Juan Lang juan.lang@gmail.com wrote:
That doesn't make it better, it's not thread-safe. Pass them as parameters to decodeRIFF.
Ahh, I see. Yes, fixed.
I still say a symbolic constant is better, or at the least, a comment. A test case that shows that it's needed is even better.
http://msdn2.microsoft.com/en-us/library/ms648061.aspx
dwVersion [in] Specifies the version number of the icon or cursor format for the resource bits pointed to by the pbIconBits parameter. This parameter can be 0x00030000.
and if you pop into the wine/dlls/user32 dir and do a grep 0x00030000 * you'll see I'm not the first to follow the documentation. It's cryptic, but hey, it's Microsoft.
Trent
--- a/dlls/imm32/imm.c +++ b/dlls/imm32/imm.c @@ -1505,7 +1505,8 @@ BOOL WINAPI ImmRegisterWordW( */ BOOL WINAPI ImmReleaseContext(HWND hWnd, HIMC hIMC) { - FIXME("(%p, %p): stub\n", hWnd, hIMC); + if (hIMC == (HIMC)root_context) + root_context->IMC.hWnd = NULL;
This doesn't belong with this patch.
if you pop into the wine/dlls/user32 dir and do a grep 0x00030000
- you'll see I'm not the first to follow the documentation.
Fair enough, objection withdrawn.
One more random piece of advice: this has gotten enough commentary that it may very well have entered Alexandre's "ignore for a while" queue. So if you don't have success with this, wait some time, and try again.
--Juan
On 8/29/07, Juan Lang juan.lang@gmail.com wrote:
This doesn't belong with this patch.
Yes, sorry.
One more random piece of advice: this has gotten enough commentary that it may very well have entered Alexandre's "ignore for a while" queue. So if you don't have success with this, wait some time, and try again.
Ok, no problem. Final(?) patch attached.
Trent
-
H. Verbeet -
Juan Lang -
Trent Waddington