On 13/04/15 01:51 AM, Pekka Paalanen wrote: > On Fri, 10 Apr 2015 11:13:22 -0500 > Derek Foreman <[email protected]> wrote: > >> On 10/04/15 09:41 AM, Derek Foreman wrote: >>> On 10/04/15 02:16 AM, Pekka Paalanen wrote: >>>> On Wed, 8 Apr 2015 15:18:41 -0500 >>>> Derek Foreman <[email protected]> wrote: >>>> >>>>> If we have no pointer focus and weston_compositor_pick_view() returns NULL >>>>> default_grab_pointer_focus() will test the unset sx, sy output parameters >>>>> from weston_compositor_pick_view(). >>>>> >>>>> Instead, assume that since both pointers are NULL focus hasn't changed and >>>>> we don't need to update pointer focus. >>>>> >>>>> Signed-off-by: Derek Foreman <[email protected]> >>>>> --- >>>>> >>>>> This fixes an error reported by valgrind. I think the patch is correct >>>>> but >>>>> it does stop pointer->focus_signal from being emitted when focus switches >>>>> from NULL to NULL. I think that shouldn't have happened anyway? >>>> >>>> If focus switches from NULL to NULL, weston_pointer_set_focus() would >>>> never be called because of the pointer->focus != view check, right? >>>> >>>> So the case you saw in Valgrind must be a case of !NULL -> NULL switch, >>>> which has to be processed but does not have valid sx, sy by definition. >>>> Therefore... >>>> >>>>> Initializing sx, sy to wl_fixed_from_int(0) would silence the warning too, >>>>> but I think this way is more correct... >>>> >>>> ...isn't initializing the sx, sy the right thing to do, and this patch >>>> actually fixes nothing? >>>> >>>> Did I miss something? >>> >>> As did I. >>> >>> As discussed on irc, the null to null case doesn't short circuit the || >>> so the patch does "something"... > > Ahaha, yeah, silly me, can't read logic operators right... sorry. > >>> However, we probably shouldn't be getting here for a null to null switch >>> anyway, so I need to investigate why that's happened in the first place. >>> >>> Initializing sx and sy would really have the same result, so I'm going >>> to avoid that for now as well. >> >> On further investigation this happens in two places: >> From udev_input_init() which is called very early, so device_added() >> gets called before the view list contains anything at all. This could >> be hidden by setting input->suspended for the duration of >> udev_input_init(). However, if the user is moving the mouse around >> during startup, it'll still happen. > > Sounds like that special-casing is not worth it, then. > >> During the first repaint when only the shell's fade surface exists. >> This can be hidden by giving the shell's fade surface an input region, I >> guess. This'll also hide the mouse cursor during fade in, which is a >> little weird/wrong. > > Yeah, not good. > >> I think we need to deal with this case with either what I've done, or >> initializing sx, sy. exposay appears to set a NULL focus with non-zero >> co-ordinates, so there may actually be a functional difference between >> these, though I'm having a hard time figuring out what it is, and if >> it's even intentional. > > Right... > > Let's take a step back. If weston_compositor_pick_view() returns NULL, > then there is no surface having input at that point. As there is no > surface, no value for sx,sy makes sense, because you don't even have a > coordinate system to begin with (a surface would define the coordinate > system). Therefore, sx,sy should not be used at all, they are invalid > in any case. > > Comparing invalid sx,sy to previous sx,sy makes no sense either. It > just should not be done. Comparing previous sx,sy to new sx,sy only > makes sense if the coordinate systems are the same, that means, the > previous and current picked surface are the same and not NULL. > > This brings us to what default_grab_pointer_focus() should do. Sx,sy > should be compared only if the previous and current surface are the > same non-NULL. In all other cases, the comparison is illegal. > > If the picked surface is different than the previous one, then we > obviously need to call weston_pointer_set_focus(). If previous and > current are NULL, then it should just do nothing, because there are no > sx,sy and going from no-target to no-target is not a change.
Ok, that's all I've handled in the previous patch. > This leaves the problem when previous surface was non-NULL, but the new > pick is NULL, because weston_pointer_set_focus() needs some sx,sy > arguments. In this case, I'd suggest we pick some arbitrary sx,sy that > is not easily confused with proper coordinates. Maybe something like > wl_fixed_from_int(-1000000). Ah, yes. I don't know if that ever happens right now, but we should prevent it. I'd like to be a little more aggressive in stamping this out - how about if view is NULL we expect 0s to be passed along with it to weston_pointer_set_focus() (and assert otherwise). Then internally in weston_pointer_set_focus() we set sx, sy to wl_fixed_from_int(-1000000). Can it be a normal integer 0 instead of a wl_fixed_from_int(0) since it's being ignored anyway? ;) exposay is the only caller that tries to use non 0 with a NULL view, and I haven't been able to figure out a reason why that's useful - we've just declared that case to be invalid. I'm planning to just make that 0 too. > If anything ends up actually computing with that value while the pick > is NULL, that's another bug to fix. > > Would this make sense? Hopefully anything doing a computation with -1000000 will stand out as wrong immediately. Valgrind's nice in that it can give me a line number, instead of 'something looks funny' some time further on... > The strange thing here I do not understand is why would you need to > call weston_pointer_set_focus() if the view stays the same but only > coordinates change? Is that something that shouldn't happen? Is that > about bumping the input serial? It would cause leave/enter, and... ooh! > > I know! We use an arbitrary leave/enter pair to signal pointer location > changes to clients when there is no physical input. That is, the > pointer device doesn't move, but the surface or view moves. We cannot > fake a wl_pointer.motion event, because it's not a user's physical > action. It happens when the surface/view moves under the pointer for any > reason. Ah yes, I can confirm that with two mice on separate seats. If I drag a window under the stationary pointer it generates events. > Thanks, > pq > >>>>> src/input.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/src/input.c b/src/input.c >>>>> index 142c670..18d1262 100644 >>>>> --- a/src/input.c >>>>> +++ b/src/input.c >>>>> @@ -157,6 +157,9 @@ default_grab_pointer_focus(struct weston_pointer_grab >>>>> *grab) >>>>> pointer->x, pointer->y, >>>>> &sx, &sy); >>>>> >>>>> + if (!pointer->focus && !view) >>>>> + return; >>>>> + >>>>> if (pointer->focus != view || pointer->sx != sx || pointer->sy != sy) >>>>> weston_pointer_set_focus(pointer, view, sx, sy); >>>>> } > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
