Commit daddb897 authored by Ryan C. Gordon's avatar Ryan C. Gordon

Significantly improved XIM support.

   Fixes Bugzilla #429.


Selected notes from the patch's README:

= FIXES =

This patch fixes the above issues as follows.

== X11 events ==

Moved XFilterEvent just after XNextEvent so that all events are passed
to it.  Also, XFilterEvent will receive masks indicated by IM through
XNFilterEvents IC value as well as masks surpplied by SDL.

X11_KeyRepeat is called between XNextEvent and XFilterEvent, after
testing an event is a KeyRelease.  I'm not 100% comfortable to do so,
but I couldn't find a better timing to call it, and use of the
function is inevitable.

== Xutf8LookupString ==

Used a longer buffer to receive UTF-8 string.  If it is insufficient,
a dynamic storage of the requested size will be allocated.  The
initial size of the buffer is set to 32, because the Japanese text
converted from the most widely used benchmark key sequence for
Japanese IM, "WATASHINONAMAEHANAKANODESU." has ten Japanese characters
in it, that occupies 30 bytes when encoded in UTF-8.

== SDL_keysym.unicode ==

On Windows version of SDL implementation, SDL_keysym.unicode stores
UTF-16 encoded unicode characters, one UTF-16 encoding unit per an SDL
event.  A Unicode supplementary characters are sent to an application
as two events.  (One with a high surrogate and another with a low
surrogate.)  The behavior seems reasonable since it is upward
compatible with existing handling of BMP characters.

I wrote a UTF-8 to UTF-16 conversion function for the purpose.  It is
designed with the execution speed in mind, having a minimum set of
features that my patch requires.

--HG--
branch : SDL-1.2
extra : convert_revision : svn%3Ac70aab31-4412-0410-b14c-859654838e24/branches/SDL-1.2%402378
parent 0f38978e
......@@ -98,6 +98,7 @@ static void X11_GetSym(const char *fnname, int *rc, void **fn)
/* Annoying varargs entry point... */
#ifdef X_HAVE_UTF8_STRING
XIC (*pXCreateIC)(XIM,...) = NULL;
char *(*pXGetICValues)(XIC, ...) = NULL;
#endif
/* These SDL_X11_HAVE_* flags are here whether you have dynamic X11 or not. */
......@@ -127,6 +128,7 @@ void SDL_X11_UnloadSymbols(void)
#ifdef X_HAVE_UTF8_STRING
pXCreateIC = NULL;
pXGetICValues = NULL;
#endif
for (i = 0; i < SDL_TABLESIZE(x11libs); i++) {
......@@ -163,6 +165,7 @@ int SDL_X11_LoadSymbols(void)
#ifdef X_HAVE_UTF8_STRING
X11_GetSym("XCreateIC",&SDL_X11_HAVE_UTF8,(void **)&pXCreateIC);
X11_GetSym("XGetICValues",&SDL_X11_HAVE_UTF8,(void **)&pXGetICValues);
#endif
if (SDL_X11_HAVE_BASEXLIB) { /* all required symbols loaded. */
......@@ -178,6 +181,7 @@ int SDL_X11_LoadSymbols(void)
#endif
#ifdef X_HAVE_UTF8_STRING
pXCreateIC = XCreateIC;
pXGetICValues = XGetICValues;
#endif
#endif
......
......@@ -68,6 +68,7 @@ void SDL_X11_UnloadSymbols(void);
/* That's really annoying...make this a function pointer no matter what. */
#ifdef X_HAVE_UTF8_STRING
extern XIC (*pXCreateIC)(XIM,...);
extern char *(*pXGetICValues)(XIC, ...);
#endif
/* These SDL_X11_HAVE_* flags are here whether you have dynamic X11 or not. */
......
......@@ -176,6 +176,124 @@ Uint32 Utf8ToUcs4(const Uint8 *utf8)
}
return c;
}
/* Given a UTF-8 encoded string pointed to by utf8 of length length in
bytes, returns the corresponding UTF-16 encoded string in the
buffer pointed to by utf16. The maximum number of UTF-16 encoding
units (i.e., Unit16s) allowed in the buffer is specified in
utf16_max_length. The return value is the number of UTF-16
encoding units placed in the output buffer pointed to by utf16.
In case of an error, -1 is returned, leaving some unusable partial
results in the output buffer.
The caller must estimate the size of utf16 buffer by itself before
calling this function. Insufficient output buffer is considered as
an error, and once an error occured, this function doesn't give any
clue how large the result will be.
The error cases include following:
- Invalid byte sequences were in the input UTF-8 bytes. The caller
has no way to know what point in the input buffer was the
errornous byte.
- The input contained a character (a valid UTF-8 byte sequence)
whose scalar value exceeded the range that UTF-16 can represent
(i.e., characters whose Unicode scalar value above 0x110000).
- The output buffer has no enough space to hold entire utf16 data.
Please note:
- '\0'-termination is not assumed both on the input UTF-8 string
and on the output UTF-16 string; any legal zero byte in the input
UTF-8 string will be converted to a 16-bit zero in output. As a
side effect, the last UTF-16 encoding unit stored in the output
buffer will have a non-zero value if the input UTF-8 was not
'\0'-terminated.
- UTF-8 aliases are *not* considered as an error. They are
converted to UTF-16. For example, 0xC0 0xA0, 0xE0 0x80 0xA0,
and 0xF0 0x80 0x80 0xA0 are all mapped to a single UTF-16
encoding unit 0x0020.
- Three byte UTF-8 sequences whose value corresponds to a surrogate
code or other reserved scalar value are not considered as an
error either. They may cause an invalid UTF-16 data (e.g., those
containing unpaired surrogates).
*/
static int Utf8ToUtf16(const Uint8 *utf8, const int utf8_length, Uint16 *utf16, const int utf16_max_length) {
/* p moves over the output buffer. max_ptr points to the next to the last slot of the buffer. */
Uint16 *p = utf16;
Uint16 const *const max_ptr = utf16 + utf16_max_length;
/* end_of_input points to the last byte of input as opposed to the next to the last byte. */
Uint8 const *const end_of_input = utf8 + utf8_length - 1;
while (utf8 <= end_of_input) {
if (p >= max_ptr) {
/* No more output space. */
return -1;
}
Uint8 const c = *utf8;
if (c < 0x80) {
/* One byte ASCII. */
*p++ = c;
utf8 += 1;
} else if (c < 0xC0) {
/* Follower byte without preceeding leader bytes. */
return -1;
} else if (c < 0xE0) {
/* Two byte sequence. We need one follower byte. */
if (end_of_input - utf8 < 1 || (((utf8[1] ^ 0x80)) & 0xC0)) {
return -1;
}
*p++ = (Uint16)(0xCF80 + (c << 6) + utf8[1]);
utf8 += 2;
} else if (c < 0xF0) {
/* Three byte sequence. We need two follower byte. */
if (end_of_input - utf8 < 2 || (((utf8[1] ^ 0x80) | (utf8[2] ^ 0x80)) & 0xC0)) {
return -1;
}
*p++ = (Uint16)(0xDF80 + (c << 12) + (utf8[1] << 6) + utf8[2]);
utf8 += 3;
} else if (c < 0xF8) {
int plane;
/* Four byte sequence. We need three follower bytes. */
if (end_of_input - utf8 < 3 || (((utf8[1] ^ 0x80) | (utf8[2] ^0x80) | (utf8[3] ^ 0x80)) & 0xC0)) {
return -1;
}
plane = (-0xC8 + (c << 2) + (utf8[1] >> 4));
if (plane == 0) {
/* This four byte sequence is an alias that
corresponds to a Unicode scalar value in BMP.
It fits in an UTF-16 encoding unit. */
*p++ = (Uint16)(0xDF80 + (utf8[1] << 12) + (utf8[2] << 6) + utf8[3]);
} else if (plane <= 16) {
/* This is a legal four byte sequence that corresponds to a surrogate pair. */
if (p + 1 >= max_ptr) {
/* No enough space on the output buffer for the pair. */
return -1;
}
*p++ = (Uint16)(0xE5B8 + (c << 8) + (utf8[1] << 2) + (utf8[2] >> 4));
*p++ = (Uint16)(0xDB80 + ((utf8[2] & 0x0F) << 6) + utf8[3]);
} else {
/* This four byte sequence is out of UTF-16 code space. */
return -1;
}
utf8 += 4;
} else {
/* Longer sequence or unused byte. */
return -1;
}
}
return p - utf16;
}
#endif
/* Check to see if this is a repeated key.
......@@ -275,6 +393,24 @@ static int X11_DispatchEvent(_THIS)
SDL_memset(&xevent, '\0', sizeof (XEvent)); /* valgrind fix. --ryan. */
XNextEvent(SDL_Display, &xevent);
/* Discard KeyRelease and KeyPress events generated by auto-repeat.
We need to do it before passing event to XFilterEvent. Otherwise,
KeyRelease aware IMs are confused... */
if ( xevent.type == KeyRelease
&& X11_KeyRepeat(SDL_Display, &xevent) ) {
return 0;
}
#ifdef X_HAVE_UTF8_STRING
/* If we are translating with IM, we need to pass all events
to XFilterEvent, and discard those filtered events immediately. */
if ( SDL_TranslateUNICODE
&& SDL_IM != NULL
&& XFilterEvent(&xevent, None) ) {
return 0;
}
#endif
posted = 0;
switch (xevent.type) {
......@@ -358,6 +494,13 @@ printf("FocusOut!\n");
}
break;
/* Some IM requires MappingNotify to be passed to
XRefreshKeyboardMapping by the app. */
case MappingNotify: {
XRefreshKeyboardMapping(&xevent.xmapping);
}
break;
/* Generated upon EnterWindow and FocusIn */
case KeymapNotify: {
#ifdef DEBUG_XEVENTS
......@@ -409,50 +552,168 @@ printf("KeymapNotify!\n");
/* Key press? */
case KeyPress: {
static SDL_keysym saved_keysym;
SDL_keysym keysym;
KeyCode keycode = xevent.xkey.keycode;
#ifdef DEBUG_XEVENTS
printf("KeyPress (X11 keycode = 0x%X)\n", xevent.xkey.keycode);
#endif
/* Get the translated SDL virtual keysym */
if ( keycode ) {
/* If we're not doing translation, we're done! */
if ( !SDL_TranslateUNICODE ) {
/* Get the translated SDL virtual keysym and put it on the queue.*/
keysym.scancode = keycode;
keysym.sym = X11_TranslateKeycode(SDL_Display, keycode);
keysym.mod = KMOD_NONE;
keysym.unicode = 0;
} else {
keysym = saved_keysym;
}
/* If we're not doing translation, we're done! */
if ( !SDL_TranslateUNICODE ) {
posted = SDL_PrivateKeyboard(SDL_PRESSED, &keysym);
break;
}
if ( XFilterEvent(&xevent, None) ) {
if ( xevent.xkey.keycode ) {
posted = SDL_PrivateKeyboard(SDL_PRESSED, &keysym);
} else {
/* Save event to be associated with IM text
In 1.3 we'll have a text event instead.. */
saved_keysym = keysym;
}
break;
}
/* Look up the translated value for the key event */
#ifdef X_HAVE_UTF8_STRING
if ( SDL_IC != NULL ) {
static Status state;
Status status;
KeySym xkeysym;
int i;
/* A UTF-8 character can be at most 6 bytes */
char keybuf[6];
if ( Xutf8LookupString(SDL_IC, &xevent.xkey,
keybuf, sizeof(keybuf),
NULL, &state) ) {
keysym.unicode = Utf8ToUcs4((Uint8*)keybuf);
/* ... It's true, but Xutf8LookupString can
return more than one characters. Moreover,
the spec. put no upper bound, so we should
be ready for longer strings. */
char keybuf[32];
char *keydata = keybuf;
int count;
Uint16 utf16buf[32];
Uint16 *utf16data = utf16buf;
int utf16size;
int utf16length;
count = Xutf8LookupString(SDL_IC, &xevent.xkey, keydata, sizeof(keybuf), &xkeysym, &status);
if (XBufferOverflow == status) {
/* The IM has just generated somewhat long
string. We need a longer buffer in this
case. */
keydata = SDL_malloc(count);
if ( keydata == NULL ) {
SDL_OutOfMemory();
break;
}
count = Xutf8LookupString(SDL_IC, &xevent.xkey, keydata, count, &xkeysym, &status);
}
switch (status) {
case XBufferOverflow: {
/* Oops! We have allocated the bytes as
requested by Xutf8LookupString, so the
length of the buffer must be
sufficient. This case should never
happen! */
SDL_SetError("Xutf8LookupString indicated a double buffer overflow!");
break;
}
case XLookupChars:
case XLookupBoth: {
if (0 == count) {
break;
}
/* We got a converted string from IM. Make
sure to deliver all characters to the
application as SDL events. Note that
an SDL event can only carry one UTF-16
encoding unit, and a surrogate pair is
delivered as two SDL events. I guess
this behaviour is probably _imported_
from Windows or MacOS. To do so, we need
to convert the UTF-8 data into UTF-16
data (not UCS4/UTF-32!). We need an
estimate of the number of UTF-16 encoding
units here. The worst case is pure ASCII
string. Assume so. */
/* In 1.3 SDL may have a text event instead, that
carries the whole UTF-8 string with it. */
utf16size = count * sizeof(Uint16);
if (utf16size > sizeof(utf16buf)) {
utf16data = (Uint16 *) SDL_malloc(utf16size);
if (utf16data == NULL) {
SDL_OutOfMemory();
break;
}
}
utf16length = Utf8ToUtf16((Uint8 *)keydata, count, utf16data, utf16size);
if (utf16length < 0) {
/* The keydata contained an invalid byte
sequence. It should be a bug of the IM
or Xlib... */
SDL_SetError("Oops! Xutf8LookupString returned an invalid UTF-8 sequence!");
break;
}
/* Deliver all UTF-16 encoding units. At
this moment, SDL event queue has a
fixed size (128 events), and an SDL
event can hold just one UTF-16 encoding
unit. So, if we receive more than 128
UTF-16 encoding units from a commit,
exceeded characters will be lost. */
for (i = 0; i < utf16length - 1; i++) {
keysym.scancode = 0;
keysym.sym = SDLK_UNKNOWN;
keysym.mod = KMOD_NONE;
keysym.unicode = utf16data[i];
posted = SDL_PrivateKeyboard(SDL_PRESSED, &keysym);
}
/* The keysym for the last character carries the
scancode and symbol that corresponds to the X11
keycode. */
if (utf16length > 0) {
keysym.scancode = keycode;
keysym.sym = (keycode ? X11_TranslateKeycode(SDL_Display, keycode) : 0);
keysym.mod = KMOD_NONE;
keysym.unicode = utf16data[utf16length - 1];
posted = SDL_PrivateKeyboard(SDL_PRESSED, &keysym);
}
break;
}
case XLookupKeySym: {
/* I'm not sure whether it is possible that
a zero keycode makes XLookupKeySym
status. What I'm sure is that a
combination of a zero scan code and a non
zero sym makes SDL_PrivateKeyboard
strange state... So, just discard it.
If this doesn't work, I'm receiving bug
reports, and I can know under what
condition this case happens. */
if (keycode) {
keysym.scancode = keycode;
keysym.sym = X11_TranslateKeycode(SDL_Display, keycode);
keysym.mod = KMOD_NONE;
keysym.unicode = 0;
posted = SDL_PrivateKeyboard(SDL_PRESSED, &keysym);
}
break;
}
case XLookupNone: {
/* IM has eaten the event. */
break;
}
default:
/* An unknown status from Xutf8LookupString. */
SDL_SetError("Oops! Xutf8LookupStringreturned an unknown status");
}
/* Release dynamic buffers if allocated. */
if (keydata != NULL && keybuf != keydata) {
SDL_free(keydata);
}
if (utf16data != NULL && utf16buf != utf16data) {
SDL_free(utf16data);
}
}
else
......@@ -472,8 +733,9 @@ printf("KeyPress (X11 keycode = 0x%X)\n", xevent.xkey.keycode);
*/
keysym.unicode = (Uint8)keybuf[0];
}
posted = SDL_PrivateKeyboard(SDL_PRESSED, &keysym);
}
posted = SDL_PrivateKeyboard(SDL_PRESSED, &keysym);
}
break;
......@@ -482,13 +744,17 @@ printf("KeyPress (X11 keycode = 0x%X)\n", xevent.xkey.keycode);
SDL_keysym keysym;
KeyCode keycode = xevent.xkey.keycode;
if (keycode == 0) {
/* There should be no KeyRelease for keycode == 0,
since it is a notification from IM but a real
keystroke. */
/* We need to emit some diagnostic message here. */
break;
}
#ifdef DEBUG_XEVENTS
printf("KeyRelease (X11 keycode = 0x%X)\n", xevent.xkey.keycode);
#endif
/* Check to see if this is a repeated key */
if ( X11_KeyRepeat(SDL_Display, &xevent) ) {
break;
}
/* Get the translated SDL virtual keysym */
keysym.scancode = keycode;
......
......@@ -143,8 +143,10 @@ SDL_X11_SYM(int,Xutf8LookupString,(XIC a,XKeyPressedEvent* b,char* c,int d,KeySy
SDL_X11_SYM(void,XDestroyIC,(XIC a),(a),)
SDL_X11_SYM(void,XSetICFocus,(XIC a),(a),)
SDL_X11_SYM(void,XUnsetICFocus,(XIC a),(a),)
/*SDL_X11_SYM(char*,XGetICValues,(XIC a, ...),return)*/
SDL_X11_SYM(XIM,XOpenIM,(Display* a,struct _XrmHashBucketRec* b,char* c,char* d),(a,b,c,d),return)
SDL_X11_SYM(Status,XCloseIM,(XIM a),(a),return)
SDL_X11_SYM(char*,XSetLocaleModifiers,(_Xconst char* a),(a),return)
#endif
#ifndef NO_SHARED_MEMORY
......
......@@ -54,6 +54,10 @@
#include "SDL_x11gamma_c.h"
#include "../blank_cursor.h"
#ifdef X_HAVE_UTF8_STRING
#include <locale.h>
#endif
/* Initialization/Query functions */
static int X11_VideoInit(_THIS, SDL_PixelFormat *vformat);
static SDL_Surface *X11_SetVideoMode(_THIS, SDL_Surface *current, int width, int height, int bpp, Uint32 flags);
......@@ -105,8 +109,10 @@ static SDL_VideoDevice *X11_CreateDevice(int devindex)
SDL_memset(device, 0, (sizeof *device));
device->hidden = (struct SDL_PrivateVideoData *)
SDL_malloc((sizeof *device->hidden));
SDL_memset(device->hidden, 0, (sizeof *device->hidden));
device->gl_data = (struct SDL_PrivateGLData *)
SDL_malloc((sizeof *device->gl_data));
SDL_memset(device->gl_data, 0, (sizeof *device->gl_data));
}
if ( (device == NULL) || (device->hidden == NULL) ||
(device->gl_data == NULL) ) {
......@@ -314,6 +320,7 @@ static void create_aux_windows(_THIS)
char classname[1024];
XSetWindowAttributes xattr;
XWMHints *hints;
unsigned long app_event_mask;
int def_vis = (SDL_Visual == DefaultVisual(SDL_Display, SDL_Screen));
/* Look up some useful Atoms */
......@@ -391,9 +398,9 @@ static void create_aux_windows(_THIS)
XFree(hints);
X11_SetCaptionNoLock(this, this->wm_title, this->wm_icon);
XSelectInput(SDL_Display, WMwindow,
FocusChangeMask | KeyPressMask | KeyReleaseMask
| PropertyChangeMask | StructureNotifyMask | KeymapStateMask);
app_event_mask = FocusChangeMask | KeyPressMask | KeyReleaseMask
| PropertyChangeMask | StructureNotifyMask | KeymapStateMask;
XSelectInput(SDL_Display, WMwindow, app_event_mask);
/* Set the class hints so we can get an icon (AfterStep) */
get_classname(classname, sizeof(classname));
......@@ -409,19 +416,111 @@ static void create_aux_windows(_THIS)
}
/* Setup the communication with the IM server */
SDL_IM = NULL;
SDL_IC = NULL;
/* create_aux_windows may be called several times against the same
Display. We should reuse the SDL_IM if one has been opened for
the Display, so we should not simply reset SDL_IM here. */
#ifdef X_HAVE_UTF8_STRING
if (SDL_X11_HAVE_UTF8) {
/* Discard obsolete resources if any. */
if (SDL_IM != NULL && SDL_Display != XDisplayOfIM(SDL_IM)) {
/* Just a double check. I don't think this
code is ever executed. */
SDL_SetError("display has changed while an IM is kept");
if (SDL_IC) {
XUnsetICFocus(SDL_IC);
XDestroyIC(SDL_IC);
SDL_IC = NULL;
}
XCloseIM(SDL_IM);
SDL_IM = NULL;
}
/* Open an input method. */
if (SDL_IM == NULL) {
char *old_locale, *old_modifiers;
/* I'm not comfortable to do locale setup
here. However, we need C library locale
(and xlib modifiers) to be set based on the
user's preference to use XIM, and many
existing game programs doesn't take care of
users' locale preferences, so someone other
than the game program should do it.
Moreover, ones say that some game programs
heavily rely on the C locale behaviour,
e.g., strcol()'s, and we can't change the C
library locale. Given the situation, I
couldn't find better place to do the
job... */
/* Save the current (application program's)
locale settings. */
old_locale = setlocale(LC_ALL, NULL);
old_modifiers = XSetLocaleModifiers(NULL);
if (old_locale == NULL || old_modifiers == NULL) {
/* The specs guarantee that the query
calls to above functions never
fail, so we should never come
here. */
SDL_SetError("failed to retreive current locale settings");
old_locale = NULL;
old_modifiers = NULL;
} else {
/* Save retreived values in our own
storage, since they may be
overwritten by the successive calls
to
setlocale/XSetLocaleModifiers. */
char const *p;
p = old_locale;
old_locale = SDL_malloc(strlen(p) + 1);
strcpy(old_locale, p);
p = old_modifiers;
old_modifiers = SDL_malloc(strlen(p) + 1);
strcpy(old_modifiers, p);
}
/* Fetch the user's preferences and open the
input method with them. */
setlocale(LC_ALL, "");
XSetLocaleModifiers("");
SDL_IM = XOpenIM(SDL_Display, NULL, classname, classname);
/* Restore the application's locale settings
so that we don't break the application's
expected behaviour. */
if (old_locale != NULL && old_modifiers != NULL) {
/* We need to restore the C library
locale first, since the
interpretation of the X modifier
may depend on it. */
setlocale(LC_ALL, old_locale);
SDL_free(old_locale);
XSetLocaleModifiers(old_modifiers);
SDL_free(old_modifiers);
}
}
/* Create a new input context for the new window just created. */
SDL_IM = XOpenIM(SDL_Display, NULL, classname, classname);
if (SDL_IM == NULL) {
SDL_SetError("no input method could be opened");
} else {
if (SDL_IC != NULL) {
/* Discard the old IC before creating new one. */
XUnsetICFocus(SDL_IC);
XDestroyIC(SDL_IC);
}
/* Theoretically we should check the current IM supports
PreeditNothing+StatusNothing style (i.e., root window method)
before creating the IC. However, it is the bottom line method,
and we supports any other options. If the IM didn't support
root window method, the following call fails, and SDL falls
back to pre-XIM keyboard handling. */
SDL_IC = pXCreateIC(SDL_IM,
XNClientWindow, WMwindow,
XNFocusWindow, WMwindow,
XNInputStyle, XIMPreeditNothing | XIMStatusNothing,
XNInputStyle, XIMPreeditNothing | XIMStatusNothing,
XNResourceName, classname,
XNResourceClass, classname,
NULL);
......@@ -430,6 +529,19 @@ static void create_aux_windows(_THIS)
SDL_SetError("no input context could be created");
XCloseIM(SDL_IM);
SDL_IM = NULL;
} else {
/* We need to receive X events that an IM wants and to pass
them to the IM through XFilterEvent. The set of events may
vary depending on the IM implementation and the options
specified through various routes. Although unlikely, the
xlib specification allows IM to change the event requirement
with its own circumstances, it is safe to call SelectInput
whenever we re-create an IC. */
unsigned long mask = 0;
char *ret = pXGetICValues(SDL_IC, XNFilterEvents, &mask, NULL);
&ic_event_mask, NULL);
XSelectInput(SDL_Display, WMwindow, app_event_mask | mask);
XSetICFocus(SDL_IC);
}
}
}
......@@ -1350,6 +1462,7 @@ void X11_VideoQuit(_THIS)
/* Close the connection with the IM server */
#ifdef X_HAVE_UTF8_STRING
if (SDL_IC != NULL) {
XUnsetICFocus(SDL_IC);
XDestroyIC(SDL_IC);
SDL_IC = NULL;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment