From 0bdc5020687d2a9b05bc1f70a0742a48071bc28d Mon Sep 17 00:00:00 2001 From: hujianwei Date: Sat, 12 Nov 2022 03:51:21 +0000 Subject: [PATCH] Fix text input bugs with grabbed popups and bugs found by new tests * 12to11-test.xml (test_manager) : Add invalid_user_time. New request and event. (test_seat_controller) : New request. * compositor.h (struct _TextInputFuncs): Make `filter_input' return keycode. * data_device.c (DestroyReference): Check if reference device is detached before unlinking reference. * seat.c (LookupKeysym): Delete function. (DispatchKey): Use keycodes instead. (XLSeatExplicitlyGrabSurface): Avoid using owner-events keyboard grab. * select.c (struct _SelectionOwnerInfo, HandleSelectionRequest): Allow CurrentTime in selection requests. Compensate for wraparound as well. (OwnSelection, DisownSelection): Use Timestamp instead of Time. * test.c (GetSerial): New function. (test_manager_impl): Add new function. * test_seat.c (SetLastUserTime): New function. (seat_controller_impl): Add new function. * tests/Imakefile (LOCAL_LIBRARIES): Remove GBM and DRM. (SRCS10, OBJS10, SRCS11, OBJS11): New variables. (dmabuf_test): Only link this program with GBM and DRM. (PROGRAMS): Add select_test, select_helper. (select_test, select_helper): New programs. * tests/README: Document that select_test needs to be run under vfb. * tests/run_tests.sh: Add TODO note. * tests/svnignore.txt: Add select_test and select_helper. * tests/test_harness.c (handle_test_manager_serial): New function. (test_manager_listener): Add new function. (open_test_display): Clear display->seat. (test_get_serial): New function. * tests/test_harness.h (struct test_display): New function `serial'. * text_input.c (CreateIC): Improve debugging code. (CalculateKeycodeForEvent): Move earlier. (FilterInputCallback): Handle keycodes here as well. (XLTextInputDispatchCoreEvent): Add more debugging code. * xdata.c (SelectSelectionInput): Obtain server time here. (XLOwnDragSelection, NoteLocalSelectionFooter): Convert times to Timestamp. --- 12to11-test.xml | 31 ++++++++++++++++ compositor.h | 8 ++-- data_device.c | 10 ++++- seat.c | 52 +++++--------------------- select.c | 19 +++++----- test.c | 11 ++++++ test_seat.c | 28 ++++++++++++++ tests/Imakefile | 15 ++++++-- tests/README | 6 ++- tests/run_tests.sh | 2 + tests/svnignore.txt | 2 + tests/test_harness.c | 21 +++++++++++ tests/test_harness.h | 4 ++ text_input.c | 87 +++++++++++++++++++++++++++----------------- xdata.c | 17 +++++---- 15 files changed, 207 insertions(+), 106 deletions(-) diff --git a/12to11-test.xml b/12to11-test.xml index fab139b..99fe3fd 100644 --- a/12to11-test.xml +++ b/12to11-test.xml @@ -53,6 +53,8 @@ summary="the specified artifical device already exists"/> + @@ -102,6 +104,12 @@ + + + Send a serial event with the next serial of the display. + + + The display_string event sends the name of the X display to @@ -110,6 +118,14 @@ + + + + The serial event is sent in reply to the get_serial request. + It provides the next serial that will be used by the display. + + + @@ -581,6 +597,21 @@ + + + Set the user time of the specified seat to the specified time. + If the last user time is greater than the specified time, + raise an invalid_user_time error. + + The month describes the number of times the millisecond + counter has wrapped around, and milliseconds specifies the X + server time. (This representation is also internally used in + the X sample server.) + + + + + This event is sent immediately after the test_seat_controller diff --git a/compositor.h b/compositor.h index a0eff35..10919dd 100644 --- a/compositor.h +++ b/compositor.h @@ -1544,8 +1544,8 @@ struct _TextInputFuncs void (*focus_in) (Seat *, Surface *); void (*focus_out) (Seat *); - /* The last argument is actually an XIDeviceEvent *. */ - Bool (*filter_input) (Seat *, Surface *, void *, KeySym *); + /* The second-last argument is actually an XIDeviceEvent *. */ + Bool (*filter_input) (Seat *, Surface *, void *, KeyCode *); }; extern void XLTextInputDispatchCoreEvent (Surface *, XEvent *); @@ -1654,8 +1654,8 @@ extern unsigned char *ReadChunk (ReadTransfer *, int, ptrdiff_t *, extern void SkipChunk (ReadTransfer *); extern void CompleteDelayedTransfer (ReadTransfer *); -extern Bool OwnSelection (Time, Atom, GetDataFunc (*) (WriteTransfer *, - Atom, Atom *), +extern Bool OwnSelection (Timestamp, Atom, GetDataFunc (*) (WriteTransfer *, + Atom, Atom *), Atom *, int); extern void DisownSelection (Atom); diff --git a/data_device.c b/data_device.c index f86a9b2..9f65dc7 100644 --- a/data_device.c +++ b/data_device.c @@ -858,8 +858,14 @@ SendDataOffers (void) static void DestroyReference (DataDeviceReference *reference) { - reference->next->last = reference->last; - reference->last->next = reference->next; + /* If reference->device is NULL, then the data device itself has + been destroyed. */ + + if (reference->device) + { + reference->next->last = reference->last; + reference->last->next = reference->next; + } XLFree (reference); } diff --git a/seat.c b/seat.c index a60da7d..76cb088 100644 --- a/seat.c +++ b/seat.c @@ -4308,30 +4308,10 @@ DispatchButton (Subcompositor *subcompositor, XIDeviceEvent *xev) xev->event_x, xev->event_y); } -static KeySym -LookupKeysym (XIDeviceEvent *xev, KeyCode keycode) -{ - unsigned int state, mods_return; - KeySym keysym; - - /* Convert the state to a core state mask. */ - state = ((xev->mods.effective & ~(1 << 13 | 1 << 14)) - | (xev->group.effective << 13)); - keysym = 0; - - /* Look up the keysym. */ - XkbLookupKeySym (compositor.display, keycode, - state, &mods_return, &keysym); - - /* Return the keysym. */ - return keysym; -} - static void DispatchKey (XIDeviceEvent *xev) { Seat *seat; - KeySym keysym; KeyCode keycode; seat = XLLookUpAssoc (seats, xev->deviceid); @@ -4345,12 +4325,12 @@ DispatchKey (XIDeviceEvent *xev) if (seat->focus_surface) { - keysym = 0; + keycode = 0; if (input_funcs && seat->flags & IsTextInputSeat && input_funcs->filter_input (seat, seat->focus_surface, - xev, &keysym)) + xev, &keycode)) /* The input method decided to filter the key. */ return; @@ -4358,25 +4338,9 @@ DispatchKey (XIDeviceEvent *xev) if (xev->flags & XIKeyRepeat) return; - keycode = xev->detail; - - /* If the input method specified a keysym to use, use it. */ - - if (keysym) - { - /* If looking up the event keycode also results in the - keysym, then just use the keycode specified in the - event. */ - if (LookupKeysym (xev, keycode) == keysym) - keycode = xev->detail; - else - keycode = XKeysymToKeycode (compositor.display, keysym); - - /* But if no corresponding keycode could be found, use the - keycode provided in the event. */ - if (!keycode) - keycode = xev->detail; - } + /* If the input method specified a keycode, use it. */ + if (!keycode) + keycode = xev->detail; if (xev->evtype == XI_KeyPress) SendKeyboardKey (seat, seat->focus_surface, @@ -5586,11 +5550,13 @@ XLSeatExplicitlyGrabSurface (Seat *seat, Surface *surface, uint32_t serial) /* Now, grab the keyboard. Note that we just grab the keyboard so that keyboard focus cannot be changed, which is not very crucial, - so it is allowed to fail. */ + so it is allowed to fail. The keyboard grab is not owner_events, + which is important for input method events to be filtered + correctly. */ state = XIGrabDevice (compositor.display, seat->master_keyboard, window, time, None, XIGrabModeAsync, - XIGrabModeAsync, True, &mask); + XIGrabModeAsync, False, &mask); /* Cancel any external grab that might be applied if the keyboard grab succeeded. */ diff --git a/select.c b/select.c index 1268a53..37e8b4b 100644 --- a/select.c +++ b/select.c @@ -56,7 +56,7 @@ struct _PropertyAtom struct _SelectionOwnerInfo { /* When this selection was last owned. */ - Time time; + Timestamp time; /* The targets of this atom. */ Atom *targets; @@ -1421,7 +1421,10 @@ HandleSelectionRequest (XEvent *event) if (!info || !info->get_transfer_function - || event->xselectionrequest.time < info->time + /* The ICCCM prohibits clients from using CurrentTime, but some + do anyway. */ + || (event->xselectionrequest.time != CurrentTime + && TimeIs (event->xselectionrequest.time, Earlier, info->time)) || !CanConvertTarget (info, event->xselectionrequest.target)) { DebugPrint ("Couldn't convert selection due to simple reason\n"); @@ -1805,7 +1808,7 @@ CompleteDelayedTransfer (ReadTransfer *transfer) } Bool -OwnSelection (Time time, Atom selection, +OwnSelection (Timestamp time, Atom selection, GetDataFunc (*hook) (WriteTransfer *transfer, Atom, Atom *), Atom *targets, int ntargets) @@ -1815,15 +1818,13 @@ OwnSelection (Time time, Atom selection, info = XLLookUpAssoc (selection_owner_info, selection); - if (time == CurrentTime) - time = XLGetServerTimeRoundtrip (); - - if (info && time < info->time) + if (info && TimestampIs (time, Earlier, info->time)) /* The timestamp is out of date. */ return False; XSetSelectionOwner (compositor.display, selection, - selection_transfer_window, time); + selection_transfer_window, + time.milliseconds); /* Check if selection ownership was actually set. */ owner = XGetSelectionOwner (compositor.display, selection); @@ -1863,7 +1864,7 @@ DisownSelection (Atom selection) if (info && info->get_transfer_function) { XSetSelectionOwner (compositor.display, selection, - None, info->time); + None, info->time.milliseconds); /* Also free info->targets. */ XLFree (info->targets); diff --git a/test.c b/test.c index 0dc33dd..c27f669 100644 --- a/test.c +++ b/test.c @@ -552,11 +552,22 @@ GetTestSeat (struct wl_client *client, struct wl_resource *resource, XLGetTestSeat (client, resource, id); } +static void +GetSerial (struct wl_client *client, struct wl_resource *resource) +{ + uint32_t serial; + + /* Send the display's next serial to the client. */ + serial = wl_display_next_serial (compositor.wl_display); + test_manager_send_serial (resource, serial); +} + static const struct test_manager_interface test_manager_impl = { .get_test_surface = GetTestSurface, .get_scale_lock = GetScaleLock, .get_test_seat = GetTestSeat, + .get_serial = GetSerial, }; diff --git a/test_seat.c b/test_seat.c index 6d2f99a..c8f37c7 100644 --- a/test_seat.c +++ b/test_seat.c @@ -1102,6 +1102,33 @@ GetDeviceController (struct wl_client *client, struct wl_resource *resource, HandleTestDeviceControllerDestroy); } +static void +SetLastUserTime (struct wl_client *client, struct wl_resource *resource, + uint32_t months, uint32_t milliseconds) +{ + Timestamp timestamp; + TestSeatController *controller; + + timestamp.months = months; + timestamp.milliseconds = milliseconds; + controller = wl_resource_get_user_data (resource); + + if (TimestampIs (timestamp, Earlier, + controller->seat->last_user_time)) + { + wl_resource_post_error (resource, TEST_MANAGER_ERROR_INVALID_USER_TIME, + "the specified user time (%"PRIu32":%"PRIu32 + ") lies in the past. the current time is %u:%u", + months, milliseconds, + controller->seat->last_user_time.months, + controller->seat->last_user_time.milliseconds); + return; + } + + controller->seat->last_user_time.months = months; + controller->seat->last_user_time.milliseconds = milliseconds; +} + static const struct test_seat_controller_interface seat_controller_impl = { .destroy = DestroySeatController, @@ -1115,6 +1142,7 @@ static const struct test_seat_controller_interface seat_controller_impl = .dispatch_XI_ButtonPress = DispatchXIButtonPress, .dispatch_XI_ButtonRelease = DispatchXIButtonRelease, .get_device_controller = GetDeviceController, + .set_last_user_time = SetLastUserTime, }; static void diff --git a/tests/Imakefile b/tests/Imakefile index 868d945..432b74c 100644 --- a/tests/Imakefile +++ b/tests/Imakefile @@ -3,7 +3,7 @@ 12TO11ROOT = .. DEPLIBS = $(DEPXLIB) SYS_LIBRARIES = MathLibrary -LOCAL_LIBRARIES = $(WAYLAND_CLIENT) $(XLIB) $(PNG) $(GBM) $(DRM) +LOCAL_LIBRARIES = $(WAYLAND_CLIENT) $(XLIB) $(PNG) COMMONOBJS = test_harness.o COMMONSRCS = test_harness.c HEADER = test_harness.h @@ -46,7 +46,11 @@ ScannerTarget(linux-dmabuf-unstable-v1) OBJS8 = $(COMMONSRCS) seat_test.o SRCS9 = $(COMMONSRCS) dmabuf_test.c OBJS9 = $(COMMONSRCS) dmabuf_test.o - PROGRAMS = imgview simple_test damage_test transform_test viewporter_test subsurface_test scale_test seat_test dmabuf_test + SRCS10 = $(COMMONSRCS) select_test.c + OBJS10 = $(COMMONSRCS) select_test.o + SRCS11 = select_helper.c + OBJS11 = select_helper.o + PROGRAMS = imgview simple_test damage_test transform_test viewporter_test subsurface_test scale_test seat_test dmabuf_test select_test select_helper /* Make all objects depend on HEADER. */ $(OBJS1): $(HEADER) @@ -62,10 +66,13 @@ NormalProgramTarget(viewporter_test,$(OBJS5),NullParameter,$(LOCAL_LIBRARIES),Nu NormalProgramTarget(subsurface_test,$(OBJS6),NullParameter,$(LOCAL_LIBRARIES),NullParameter) NormalProgramTarget(scale_test,$(OBJS7),NullParameter,$(LOCAL_LIBRARIES),NullParameter) NormalProgramTarget(seat_test,$(OBJS8),NullParameter,$(LOCAL_LIBRARIES),NullParameter) -NormalProgramTarget(dmabuf_test,$(OBJS9),NullParameter,$(LOCAL_LIBRARIES),NullParameter); +NormalProgramTarget(dmabuf_test,$(OBJS9),NullParameter,$(LOCAL_LIBRARIES) $(GBM) $(DRM),NullParameter); +NormalProgramTarget(select_test,$(OBJS10),NullParameter,$(LOCAL_LIBRARIES) ThreadsLibraries,NullParameter); +NormalProgramTarget(select_helper,$(OBJS11),NullParameter,$(XLIB),NullParameter); DependTarget3($(SRCS1),$(SRCS2),$(SRCS3)) DependTarget3($(SRCS4),$(SRCS5),$(SRCS6)) -DependTarget3($(SRCS7),$(SRCS8),NullParameter) +DependTarget3($(SRCS7),$(SRCS8),$(SRCS9)) +DependTarget3($(SRCS10),$(SRCS11),NullParameter) all:: $(PROGRAMS) diff --git a/tests/README b/tests/README index ade44f3..205ef1e 100644 --- a/tests/README +++ b/tests/README @@ -20,5 +20,7 @@ would be ``integration tests''. Most likely, you do not want to run these tests manually, as the `run_tests.sh' script does all the setup for you. -Please note that the EGL renderer currently does not pass the -viewporter test, which is expected behavior. +Please note that the EGL renderer currently does not pass some +graphics tests, which is expected behavior, and that `select_test' +must be run with no clipboard manager (or any other clients, for that +matter) running. diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 32d668b..88c026f 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -45,4 +45,6 @@ do fi done +# TODO: figure out how to run some tests under Xvfb. + popd diff --git a/tests/svnignore.txt b/tests/svnignore.txt index a121b13..38fdf26 100644 --- a/tests/svnignore.txt +++ b/tests/svnignore.txt @@ -9,6 +9,8 @@ subsurface_test scale_test seat_test dmabuf_test +select_test +select_helper imgview reject.dump Makefile diff --git a/tests/test_harness.c b/tests/test_harness.c index 6aaaec0..56a206c 100644 --- a/tests/test_harness.c +++ b/tests/test_harness.c @@ -61,9 +61,20 @@ handle_test_manager_display_string (void *data, struct test_manager *manager, &display->num_pixmap_formats); } +static void +handle_test_manager_serial (void *data, struct test_manager *manager, + uint32_t serial) +{ + struct test_display *display; + + display = data; + display->serial = serial; +} + static const struct test_manager_listener test_manager_listener = { handle_test_manager_display_string, + handle_test_manager_serial, }; static bool @@ -220,6 +231,7 @@ open_test_display (struct test_interface *interfaces, int num_interfaces) if (!display->scale_lock) goto error_4; + display->seat = NULL; return display; error_4: @@ -901,3 +913,12 @@ test_complete (void) test_log ("test ran successfully"); exit_with_code (0); } + +uint32_t +test_get_serial (struct test_display *display) +{ + test_manager_get_serial (display->test_manager); + wl_display_roundtrip (display->display); + + return display->serial; +} diff --git a/tests/test_harness.h b/tests/test_harness.h index a17972d..46298a9 100644 --- a/tests/test_harness.h +++ b/tests/test_harness.h @@ -87,6 +87,9 @@ struct test_display /* The number of such interfaces. */ int num_test_interfaces; + + /* Internal field used by test_get_serial. */ + uint32_t serial; }; struct test_interface @@ -152,6 +155,7 @@ extern void test_init (void); extern void test_complete (void) __attribute__ ((noreturn)); extern void test_set_scale (struct test_display *, int); extern void test_init_seat (struct test_display *); +extern uint32_t test_get_serial (struct test_display *); #define ARRAYELTS(arr) (sizeof (arr) / sizeof (arr)[0]) diff --git a/text_input.c b/text_input.c index 272ff3e..6abb781 100644 --- a/text_input.c +++ b/text_input.c @@ -2488,7 +2488,8 @@ CreateIC (TextInput *input) XLAssert (!input->xic); - DebugPrint ("creating XIC for text input %p", input); + DebugPrint ("creating XIC for text input %p and window 0x%lx", input, + window); status_attr = NULL; preedit_attr = NULL; @@ -3301,16 +3302,47 @@ LookupString (TextInput *input, XEvent *event, KeySym *keysym_return) return True; } +static KeyCode +CalculateKeycodeForEvent (XEvent *event, KeySym keysym) +{ + KeySym sym_return; + unsigned int mods_return; + + /* Calculate the keycode to actually send clients along with an + event, given the keysym specified by the input method. Return 0 + if no special treatment is required. */ + + if (!keysym) + return 0; + + /* If looking up the event keycode also results in the keysym, + then just use the keycode specified in the event. This is + because French keyboard layouts have multiple keycodes that + decode to the same keysym, which causes problems later on + when Wayland clients keep repeating the "a" key, as a keysym + was looked up for the key press but not for the corresponding + key release. */ + if (XkbLookupKeySym (compositor.display, event->xkey.keycode, + event->xkey.state, &mods_return, &sym_return) + && keysym == sym_return) + return 0; + + /* Otherwise, convert the keysym to a keycode and use that. */ + return XLKeysymToKeycode (keysym, event); +} + static Bool FilterInputCallback (Seat *seat, Surface *surface, void *event, - KeySym *keysym) + KeyCode *keycode) { XIDeviceEvent *xev; XEvent xkey; TextInputClientInfo *info; TextInput *input; + KeySym keysym; xev = event; + keysym = 0; DebugPrint ("seat %p, surface %p, detail: %d, event: %lx", seat, surface, xev->detail, xev->event); @@ -3342,7 +3374,19 @@ FilterInputCallback (Seat *seat, Surface *surface, void *event, /* Otherwise, call XmbLookupString. If a keysym is returned, return False. Otherwise, commit the string looked up and return True. */ - return LookupString (input, &xkey, keysym); + if (LookupString (input, &xkey, &keysym)) + return True; + + /* Look up the right keycode for the event. */ + + if (!keysym && xev->type == XI_KeyRelease) + *keycode = GetKeycode (&input->keysym_map, xev->detail); + else + *keycode = CalculateKeycodeForEvent (&xkey, keysym); + + if (xev->type == XI_KeyRelease) + /* Remove the keycode from the keycode-keysym map. */ + RemoveKeysym (&input->keysym_map, xev->detail); } } @@ -3360,35 +3404,6 @@ static TextInputFuncs input_funcs = .filter_input = FilterInputCallback, }; -static KeyCode -CalculateKeycodeForEvent (XEvent *event, KeySym keysym) -{ - KeySym sym_return; - unsigned int mods_return; - - /* Calculate the keycode to actually send clients along with an - event, given the keysym specified by the input method. Return 0 - if no special treatment is required. */ - - if (!keysym) - return 0; - - /* If looking up the event keycode also results in the keysym, - then just use the keycode specified in the event. This is - because French keyboard layouts have multiple keycodes that - decode to the same keysym, which causes problems later on - when Wayland clients keep repeating the "a" key, as a keysym - was looked up for the key press but not for the corresponding - key release. */ - if (XkbLookupKeySym (compositor.display, event->xkey.keycode, - event->xkey.state, &mods_return, &sym_return) - && keysym == sym_return) - return 0; - - /* Otherwise, convert the keysym to a keycode and use that. */ - return XLKeysymToKeycode (keysym, event); -} - void XLTextInputDispatchCoreEvent (Surface *surface, XEvent *event) { @@ -3436,8 +3451,12 @@ XLTextInputDispatchCoreEvent (Surface *surface, XEvent *event) return; if (info->focus_surface != surface) - /* The surface is no longer focused. */ - return; + { + /* The surface is no longer focused. */ + DebugPrint ("incorrect focus surface (focus surface is %p, " + "surface is %p)", info->focus_surface, surface); + return; + } if (info) { diff --git a/xdata.c b/xdata.c index 4570a30..3102ead 100644 --- a/xdata.c +++ b/xdata.c @@ -1201,20 +1201,20 @@ static void SelectSelectionInput (Atom selection) { int mask; + Time time; /* If the selection already exists, announce it to Wayland clients - as well. Use CurrentTime (even though the ICCCM says this is a - bad idea); any XFixesSelectionNotify event that arrives later - will clear up our (incorrect) view of the selection change - time. */ + as well. Use the current time. */ + + time = XLGetServerTimeRoundtrip (); if (selection == CLIPBOARD && XGetSelectionOwner (compositor.display, CLIPBOARD) != None) - NoticeClipboardChanged (CurrentTime); + NoticeClipboardChanged (time); if (selection == XA_PRIMARY && XGetSelectionOwner (compositor.display, XA_PRIMARY) != None) - NoticePrimaryChanged (CurrentTime); + NoticePrimaryChanged (time); mask = XFixesSetSelectionOwnerNotifyMask; mask |= XFixesSelectionWindowDestroyNotifyMask; @@ -1995,7 +1995,8 @@ XLOwnDragSelection (Time time, DataSource *source) /* Own the selection. */ drag_data_source = source; - rc = OwnSelection (time, XdndSelection, GetDragCallback, + rc = OwnSelection (TimestampFromClientTime (time), + XdndSelection, GetDragCallback, targets, ntargets); XLFree (targets); @@ -2135,7 +2136,7 @@ FindTargetInArray (Atom *targets, int ntargets, Atom atom) /* And own the selection. */ \ field = source; \ \ - rc = OwnSelection (time.milliseconds, atom, callback, targets, ntargets); \ + rc = OwnSelection (time, atom, callback, targets, ntargets); \ XLFree (targets); \ return rc \