On Sun, 8 Aug 2010, Michael Jennings wrote: > On Wednesday, 04 August 2010, at 22:37:21 (+0200), > Kim Woelders wrote: > >> A few comments... >> >> The "else if (rq->target == XA_STRING)" case is redundant - same as final >> "else" case. > > You're right. Yes, I know, in my first version I wrote it exactly as now the patch is. After, I thought that adding both the XA_STRING block, and the non-conditional XA_STRING forcing block, was redundant but better for readability, but well, ain't a big issue.
>> I think that in both cases where XmbTextListToTextProperty() is called >> XFree(xtextp.value) should be added after XChangeProperty(). > > Right again. Wow, that would be one heck of a resource leak.... Yess yes, thanks Kim. > Kim/Paolo, please comment on the patch I committed. Most importantly, > if/how to condense or eliminate the two XmbText...() cases. > > Thanks! > Michael here's an updated patch. macros mess, possible to condense more, but at the expense of readability I think. I've put XA_UTF8_STRING instead of X_HAVE_UTF8_STRING, because I've read somewhere [ http://www.mail-archive.com/maintain...@lists.opencsw.org/msg04089.html ] a case in which a Solaris OS does have X_HAVE_UTF8_STRING, but without XmbTextListToTextProperty and XUTF8StringStyle, while the presence of XA_UTF8_STRING should mean availability of the libXmu functions. Portability nightmares. And I hope the lines below will not break. But of course they will. --- src/screen.c.bak0 2010-08-09 03:19:15.000000000 +0200 +++ src/screen.c 2010-08-09 04:45:09.000000000 +0200 @@ -3329,6 +3329,13 @@ XEvent ev; long target_list[2]; +#if defined (MULTI_CHARSET) && (defined(XA_UTF8_STRING) || defined(HAVE_X11_XMU_ATOMS_H)) + XTextProperty xtextp; + char *l[1]; + *l = selection.text; + xtextp.value = NULL; + xtextp.nitems = 0; +#endif ev.xselection.type = SelectionNotify; ev.xselection.property = None; ev.xselection.display = rq->display; @@ -3345,14 +3352,8 @@ (sizeof(target_list) / sizeof(target_list[0]))); ev.xselection.property = rq->property; #ifdef MULTI_CHARSET -# ifdef X_HAVE_UTF8_STRING + #ifdef XA_UTF8_STRING } else if (rq->target == XA_UTF8_STRING(Xdisplay)) { - XTextProperty xtextp; - char *l[1]; - - *l = selection.text; - xtextp.value = NULL; - xtextp.nitems = 0; if (XmbTextListToTextProperty(Xdisplay, l, 1, XUTF8StringStyle, &xtextp) == Success) { if (xtextp.nitems > 0 && xtextp.value != NULL) { XChangeProperty(Xdisplay, rq->requestor, rq->property, @@ -3361,15 +3362,9 @@ XFree(xtextp.value); } } -# endif /* X_HAVE_UTF8_STRING */ -# ifdef HAVE_X11_XMU_ATOMS_H - } else if (rq->target == XA_TEXT(Xdisplay) || rq->target == XA_COMPOUND_TEXT(Xdisplay)) { - XTextProperty xtextp; - char *l[1]; - - *l = selection.text; - xtextp.value = NULL; - xtextp.nitems = 0; + #endif + #ifdef HAVE_X11_XMU_ATOMS_H + } else if (rq->target == XA_TEXT(Xdisplay) || rq->target == XA_COMPOUND_TEXT(Xdisplay)) { if (XmbTextListToTextProperty(Xdisplay, l, 1, XCompoundTextStyle, &xtextp) == Success) { if (xtextp.nitems > 0 && xtextp.value != NULL) { XChangeProperty(Xdisplay, rq->requestor, rq->property, XA_COMPOUND_TEXT(Xdisplay), @@ -3378,7 +3373,7 @@ XFree(xtextp.value); } } -# endif /* HAVE_X11_XMU_ATOMS_H */ + #endif #endif /* MULTI_CHARSET */ } else { XChangeProperty(Xdisplay, rq->requestor, rq->property, XA_STRING, 8, PropModeReplace, selection.text, selection.len); ------------------------------------------------------------------------------ This SF.net email is sponsored by Make an app they can't live without Enter the BlackBerry Developer Challenge http://p.sf.net/sfu/RIM-dev2dev _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
