Hello everybody!
Attached is my current cursor icon patches based upon work by Henri Verbeet and Andrew Riedi for review and comment. I finished the actual code about 3 weeks ago, but it's been a lot of work for me to split it out into smaller pieces, especially being new to git. Here is a break-down of what these patches do and the theory.
* Adds real 32-bit handles (instead of converting 16-bit handles to 32 bit). * Cursors & icons stored in non-moveable memory block (a.k.a., "32 bit data") in addition to existing 16-bit moveable block. * A new 'struct cursor_header' (include/wine/winuser16.h) is pre-pended to the 32-bit cursor/icon data (16-bit data format is untouched). * A new 'struct cursor_map_entry' (dlls/user32/cursoricon.c) is used to manage a cursor's association with its handles and data. * Adds more complete .ani support (I think nearly 100% complete). * Adds themed icon support (from your X-windows theme). * Fixes a lot of bugs. * And finally, moves cursors to the server.
The server opaquely stores the raw 32-bit cursor/icon data. Since the client does all interaction with winex11.drv, the server doesn't really need to understand it. The client caches all objects locally, but attempts to retrieve from the server any handle it doesn't know about. Locally, the client uses a pair of rbtrees to associate cursor objects (actually struct cursor_map_entry) with their respective 16 & 32 bit handles. The 16 bit cursor data is automatically generated on the client and can still be directly manipulated from 16 bit programs; in such cases, the 32-bit cursor data will be updated from the modified 16 bit data. The CRITICAL_SECTION IconCrst is used to synchronize access to the rbtrees and ensure that data is unmodified once an operation on that data begins (in database terms, IconCrst is used to lock the entire "table" and there's no "row-level" locking).
There are several new functions to query and manipulate the rbtrees:
add_cursor_entry : (self-explanatory) remove_cursor_entry: (self-explanatory) get_cursor_handle16: retrieves a 16-bit handle from a 32-bit one get_cursor_handle : visa-versa
Layered on top of that are a basic set of functions for higher level management of cursor/icon objects:
alloc_cursor_data : allocate & initialize cursor data & header create_cursor : creates cursor and auto-generates 16-bit data destroy_cursor : destroys cursor and associated 16-bit data get_cursor_entry : will look both locally and on the server get_cursor_object : returns the cursor's data update_cursor_data16: creates or updates the 16-bit data from 32-bit data CURSORICON_refresh_cursor16: updates the 32-bit data from 16-bit data
The functions add_cursor_entry and get_cursor_entry directly return a pointer to the struct cursor_map_entry and therefore require locking IconCrst in advance of calling (or will fail an assertion). Having direct access to the cursor_map_entry reduces complexity in some circumstances.
One of the oddest fixes is the fake_cursor() function, which is used to accommodate deleting the active cursor, which windows doesn't mind. This isn't the only way to go about it, but the alternative was to make the cursor empty and I'm not certain that that is the visual behavior of windows when deleting the active cursor, so I'm more than open to feedback on that as well as anything else in these patches.
Thanks! Daniel
I haven't heard anything, so if anybody can find the time I would greatly appreciate it.
Thanks, Daniel
--- On Thu, 7/23/09, Daniel Santos javatroubadour@yahoo.com wrote:
From: Daniel Santos javatroubadour@yahoo.com Subject: Cursor & Icon patches To: "wine-devel" wine-devel@winehq.org Date: Thursday, July 23, 2009, 1:59 AM Hello everybody!
Attached is my current cursor icon patches based upon work by Henri Verbeet and Andrew Riedi for review and comment. I finished the actual code about 3 weeks ago, but it's been a lot of work for me to split it out into smaller pieces, especially being new to git. Here is a break-down of what these patches do and the theory.
- Adds real 32-bit handles (instead of converting 16-bit
handles to 32 bit).
- Cursors & icons stored in non-moveable memory block
(a.k.a., "32 bit data") in addition to existing 16-bit moveable block.
- A new 'struct cursor_header' (include/wine/winuser16.h)
is pre-pended to the 32-bit cursor/icon data (16-bit data format is untouched).
- A new 'struct cursor_map_entry'
(dlls/user32/cursoricon.c) is used to manage a cursor's association with its handles and data.
- Adds more complete .ani support (I think nearly 100%
complete).
- Adds themed icon support (from your X-windows theme).
- Fixes a lot of bugs.
- And finally, moves cursors to the server.
The server opaquely stores the raw 32-bit cursor/icon data. Since the client does all interaction with winex11.drv, the server doesn't really need to understand it. The client caches all objects locally, but attempts to retrieve from the server any handle it doesn't know about. Locally, the client uses a pair of rbtrees to associate cursor objects (actually struct cursor_map_entry) with their respective 16 & 32 bit handles. The 16 bit cursor data is automatically generated on the client and can still be directly manipulated from 16 bit programs; in such cases, the 32-bit cursor data will be updated from the modified 16 bit data. The CRITICAL_SECTION IconCrst is used to synchronize access to the rbtrees and ensure that data is unmodified once an operation on that data begins (in database terms, IconCrst is used to lock the entire "table" and there's no "row-level" locking).
There are several new functions to query and manipulate the rbtrees:
add_cursor_entry : (self-explanatory) remove_cursor_entry: (self-explanatory) get_cursor_handle16: retrieves a 16-bit handle from a 32-bit one get_cursor_handle : visa-versa
Layered on top of that are a basic set of functions for higher level management of cursor/icon objects:
alloc_cursor_data : allocate & initialize cursor data & header create_cursor : creates cursor and auto-generates 16-bit data destroy_cursor : destroys cursor and associated 16-bit data get_cursor_entry : will look both locally and on the server get_cursor_object : returns the cursor's data update_cursor_data16: creates or updates the 16-bit data from 32-bit data CURSORICON_refresh_cursor16: updates the 32-bit data from 16-bit data
The functions add_cursor_entry and get_cursor_entry directly return a pointer to the struct cursor_map_entry and therefore require locking IconCrst in advance of calling (or will fail an assertion). Having direct access to the cursor_map_entry reduces complexity in some circumstances.
One of the oddest fixes is the fake_cursor() function, which is used to accommodate deleting the active cursor, which windows doesn't mind. This isn't the only way to go about it, but the alternative was to make the cursor empty and I'm not certain that that is the visual behavior of windows when deleting the active cursor, so I'm more than open to feedback on that as well as anything else in these patches.
Thanks! Daniel
-----Inline Attachment Follows-----
On 25/07/09 12:46, Daniel Santos wrote:
I finished the actual code about 3 weeks ago,
but it's been a lot of work for me to split it out into smaller pieces, especially being new to git.
The intention is that is that you build and test smaller chunks, hence the term "commit often". It certainly makes it easier later on.
On Sat, Jul 25, 2009 at 8:04 AM, latsjeff_lats@hotmail.com wrote:
On 25/07/09 12:46, Daniel Santos wrote:
I finished the actual code about 3 weeks ago,
but it's been a lot of work for me to split it out into smaller pieces, especially being new to git.
The intention is that is that you build and test smaller chunks, hence the term "commit often". It certainly makes it easier later on.
Sure in general you should submit small chunks but in this case it is a very invasive change and I think Alexandre wants to see the overal work before he lets in small chunks (e.g. changes in winex11, wineserver and other parts are needed). Unfortunately I don't know anything about the cursor code, so can't comment on it. I think Henri, Alexandre and perhaps some other can comment on it.
Roderick
On 07/25/2009 02:06 PM, Roderick Colenbrander wrote:
On Sat, Jul 25, 2009 at 8:04 AM, latsjeff_lats@hotmail.com wrote:
On 25/07/09 12:46, Daniel Santos wrote:
I finished the actual code about 3 weeks ago,
but it's been a lot of work for me to split it out into smaller pieces, especially being new to git.
The intention is that is that you build and test smaller chunks, hence the term "commit often". It certainly makes it easier later on.
Sure in general you should submit small chunks but in this case it is a very invasive change and I think Alexandre wants to see the overal work before he lets in small chunks (e.g. changes in winex11, wineserver and other parts are needed). Unfortunately I don't know anything about the cursor code, so can't comment on it. I think Henri, Alexandre and perhaps some other can comment on it.
Roderick
Anyway Alexandre is on holiday next week, so you'll have to wait a bit to get his comments.
Anyway Alexandre is on holiday next week, so you'll have to wait a bit to get his comments.
Yea, that's what I was afraid of.
As far as the chunks, most of them do stand on their own with updated test code (one of them fails tests). I was considering submitting the patches in both the incremental form in addition to a single bulk patch. I guess it doesn't matter too much since you can do that pretty easily in git anyway.
Thank you for the responses.
Daniel
--- On Sat, 7/25/09, Frédéric Delanoy frederic.delanoy@gmail.com wrote:
From: Frédéric Delanoy frederic.delanoy@gmail.com Subject: Re: Request for Review: Cursor & Icon patches To: "Roderick Colenbrander" thunderbird2k@gmail.com Cc: "wine-devel" wine-devel@winehq.org, "lats" jeff_lats@hotmail.com Date: Saturday, July 25, 2009, 9:56 AM On 07/25/2009 02:06 PM, Roderick Colenbrander wrote:
On Sat, Jul 25, 2009 at 8:04 AM, latsjeff_lats@hotmail.com
wrote:
On 25/07/09 12:46, Daniel Santos wrote:
I finished the actual code about 3 weeks ago,
but it's been a lot of work for me to
split it out into
smaller pieces, especially being new to
git.
The intention is that is that you build and test
smaller chunks, hence the
term "commit often". It certainly makes it
easier later on.
Sure in general you should submit small chunks but in
this case it is
a very invasive change and I think Alexandre wants to
see the overal
work before he lets in small chunks (e.g. changes in
winex11,
wineserver and other parts are needed). Unfortunately
I don't know
anything about the cursor code, so can't comment on
it. I think Henri,
Alexandre and perhaps some other can comment on it.
Roderick
Anyway Alexandre is on holiday next week, so you'll have to wait a bit to get his comments.
On 07/25/2009 08:21 PM, Daniel Santos wrote:
Anyway Alexandre is on holiday next week, so you'll have to wait a bit to get his comments.
Yea, that's what I was afraid of.
As far as the chunks, most of them do stand on their own with updated test code (one of them fails tests).
You should probably ensure all of your tests succeed if you want your code to be included.
I was considering submitting the patches in both the incremental form in addition to a single bulk patch. I guess it doesn't matter too much since you can do that pretty easily in git anyway.
The purpose of separate patches is - to ease review - to ease regression testing
Submitting a bulk patch is unneeded since you can easily get diffs between arbitrary revisions in git.
I only looked at the first couple of patches, but this probably needs more work. E.g. in the first patch you white space changes and changes to apparently unrelated code. It also helps to be a bit more descriptive about what you're testing, instead of something like "Add more tests for SetCursor & DestroyCursor".
- Patch 2: I'm not sure we need the extra traces there, since +relay will show what's called, and you can always add more specific traces while debugging. Regardless, you also have white space changes in this patch.
- Patch 3 & 4: We very rarely care about LastError after successful calls. Pretty much the only exception is if a (real world) application depends on the behaviour. So you'll need at least tests and an application that depends on this for those patches to go in.
- Patch 5: Again, white space / formatting. - I think some of the comments as well as some of the asserts are a bit superfluous. - The forward declaration for get_bitmap_width_bytes() is redundant. - cursor_rb_functions and cursor16_rb_functions should probably be static. - CURSORICON_init() should fail if initializing the lookup trees fails (and as a result, process_attach() as well). - Although it will probably work, I think it's ugly to pass the key to wine_rb_get() as the pointer instead of passing a pointer to the key as intended. - wine_rb_put() might fail. - Using map_entry as handle in add_cursor_entry() is unsafe on 64-bit. I also think it's somewhat questionable that add_cursor_entry() creates the 32-bit handle in the first place. The way it should work is that create_cursor creates a cursor handle from the data, and an entry in the lookup tree is only created when no 16-bit cursor exists yet in get_cursor_handle16(). In general, it looks like you're abusing the 16 <=> 32 lookup trees to determine if a handle is valid. You really need a handle table for that, either in the server, or something local to user32.
-
Daniel Santos -
Frédéric Delanoy -
Henri Verbeet -
lats -
Roderick Colenbrander