2008/11/1 Frank Schmitt <[EMAIL PROTECTED]>:
> Hello,
>
> I tried to fix all issues you mentioned. Some comments below:
>
> Marc Lehmann <[EMAIL PROTECTED]> writes:
>
>> I just had basically a four hour talk about your patch, and here is the deal:
>>
>> - the code seems to be broken - _NET_WM_ICON expects cardinals, and thus
>>   an array of longs, but you seem to pass in two int's followed by bytes?
>>   you would need to fix this.
>> - similarly, are you sure there isn't any endianness issue in the conversion?
>>   does it really work on big-endian systems?
>> - fix the coding style to be the same as the rest of the code (especially
>>   the indentation which is weird, but also curly braces and
>>   whitespace).
>
> All this should be OK now.
>
>> - make sure you set the property before urxvt maps the window (this seems
>>   to be the case already).
>
> Should be fine. My code is in create_windows(), this is called in the
> middle of init() while the windows are mapped at the end of init().
>
>> - you don't seem to free the created asvisual anywhere, bug?
>
> I removed the call to create_asvisual and used the object already
> created in init_resources instead. However it seems, the one created
> init_resources isn't freed, too.
>
>> - make it dependent on ENABLE_FRILLS
>
> Done.
>
>> - add the neccessary documentation (rxvt.1.pod)
>
> I have a terse documentation for the resource and a slightly longer for
> the option as this seems to be the standard, I hope this is ok.
>
> If there are more issues with the reworked patch, just tell me.

+Compile I<afterimage>: Specify image file for the applications
+icon. Many window managers will show the icon in some sort of taskbar

should be application

+      ASImage *im = file2ASImage(rs [Rs_iconfile], 0xFFFFFFFF,
SCREEN_GAMMA, 0, NULL);
+              ARGB32* asbuf=result->alt.argb32;

probably want to be more consistent with spaces around = and (
urxvt uses a space before ( in function calls, and spaces around =

+              Atom net_wm_icon = XInternAtom(dpy, "_NET_WM_ICON", False);
+              Atom cardinal = XInternAtom(dpy, "CARDINAL", False);

_NET_WM_ICON should be added as XA_NET_WM_ICON in rxvttoolkit.C and .h
(in ENABLE_EWMH, which i guess means this code should be under that
ifdef as well).
XA_CARDINAL is already defined and used in other places.
-- 
Mikael Magnusson

_______________________________________________
rxvt-unicode mailing list
[email protected]
http://lists.schmorp.de/cgi-bin/mailman/listinfo/rxvt-unicode

Reply via email to