Page MenuHomePhabricator

e_grabinput: do not pass a faulty time
ClosedPublic

Authored by bu5hm4n on Dec 19 2015, 2:19 AM.

Details

Summary

currently there is ecore_x_current_time passed, which is the time of the
last event. if this is passed to ecore_x_window_focus_at_time the request can be
ignored because the last event can be in the past. Instead using
ecore_x_window_focus fixes this, because current time is passed, which
means that x is just using this event at the time it is called.

@fix T2948

Test Plan

Try to run spotify and try to trigger the bug, I cannot anymore.

Diff Detail

Repository
rE core/enlightenment
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n updated this revision to Diff 7903.Dec 19 2015, 2:19 AM
bu5hm4n retitled this revision from to e_grabinput: do not pass a faulty time.
bu5hm4n updated this object.
bu5hm4n edited the test plan for this revision. (Show Details)
bu5hm4n added a reviewer: zmike.
bu5hm4n added a subscriber: billiob.

This patch even fixes more. Intelij is not locking anymore for me if a popup in the code shows up. In eclipse the autocompletion window is getting keyboard input.
(I switched again to Chromium, maybe the tooltip deadlock there is also gone)

abyomi0 added a subscriber: abyomi0.EditedDec 20 2015, 6:09 AM

This patch even fixes more. Intelij is not locking anymore for me if a popup in the code shows up. In eclipse the autocompletion window is getting keyboard input.
(I switched again to Chromium, maybe the tooltip deadlock there is also gone)

I've noticed that the tooltip deadlock hasn't showed up since Chromium 47. However, it is possible that I just haven't been able to trigger it.

Edit: it still exists and can be triggered (Latest comments) https://code.google.com/p/chromium/issues/detail?id=381732

zmike added a subscriber: raster.Dec 20 2015, 6:22 AM

rE0149e51aaacc36aa29ce5da9ecdc4104ab9b2012 from @raster explicitly made the opposite change that you're proposing. I'd be interested in hearing the reasoning for that if possible.

Hi raster, can you check if you are happy with this patch ?

Can we make some progress here ? I know at least two people where this patch is fixing Intelij and spotify bugs. And it would be cool to see it in a enlightenment-20 release. Are there some objections against it ? I ran it now since I pushed this revision without any problem.

raster edited edge metadata.Jan 15 2016, 4:32 PM

agh. so many patches.

so ecore_x_current_time_get() returns the timestamp of the last timestamp of an event like a mouse down, mouse move, key down, mouse in, mouse out, focus in, focus out, property change. these can trigger actions. the reason to do that change is to ensure focus timestamp matches (probably) the event that triggered it like the mouse click for click-to-focus, or the mouse in for pointer focus, or the key press with alt-tabbing around etc. that's why.

past timestamps are ok - as long as they dont pre-date the previous time focus was changed. we listen for focus changes so this should not be the case UNLESS there is a race condition - we haven't gotten a focus in/out event yet due to someone changing focus (e or some other client) and thus the action has already happened but we don't know yet. the manual section:

The XSetInputFocus() function changes the input focus and the last-focus-change time. It has no effect if the specified time is earlier than the current last-focus-change time or is later than the current X server time. Otherwise, the last-focus-change time is set to the specified time (CurrentTime is replaced by the current X server time)

That's why I did that. If this fixes things - then put it in. I can only imagine we are hitting a race condition.

Ok, thx for the details.

I will put it in, if there are new focus issues we can revert this patch.

This revision was automatically updated to reflect the committed changes.