Sometimes no focus on new window #37

Closed
williex opened this Issue Aug 15, 2015 · 30 comments

Comments

Projects
None yet
6 participants
@williex

williex commented Aug 15, 2015

When a new program is started, icewm sometimes fails to properly focus it. A way for me to reproduce 100%:

  1. Start audacious (music player) on desktop 1
  2. Press Ctrl+J, an audacious dialog window appears
  3. Switch to desktop 2, by keyboard or mouse, doesn't matter.
  4. I press the key Ctrl+Alt+a which I have defined as:
    key "Ctrl+Alt+a" xfce4-terminal
    Or I start a terminal from the menu or start a terminal from the quicklaunch, it doesn't matter.
  5. The new terminal window does not get focus, it does have the urgency bit set and starts flashing in the taskbar.

The same problem happens with firefox or chromium and seems to happen when there is already a firefox or chromium window running somewhere and a new window is opened.

This problem does not happen in icewm 1.3.8. I've tracked it down to commit a57ec4f. The following patch fixes the problem for me. It's a terrible patch because it completely ignores the new timing code but my X knowledge is too limited to completely understand the code in updateNetWMUserTime

--- icewm-1.3.10.bak/src/wmframe.cc 2015-07-03 03:38:11.000000000 +0200
+++ icewm-1.3.10/src/wmframe.cc 2015-08-15 12:15:08.763321027 +0200
@@ -1851,9 +1851,9 @@
             doActivate = false;
     }

-    if (fUserTime != -1UL)
+/*    if (fUserTime != -1UL)
         if (fUserTime == 0 || fUserTime != manager->lastUserTime())
-            doActivate = false;
+            doActivate = false;*/
 }

 void YFrameWindow::wmShow() {

@bbidulock bbidulock self-assigned this Aug 15, 2015

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Aug 15, 2015

Owner

Well, all that does is shut off _NET_WM_USER_TIME support.

_NET_WM_USER_TIME, as far as I understand it, is supposed to work like so:

  1. An application that supports _NET_WM_USER_TIME sets the property of the same name on its top-level windows whenever there is user input (key or button) within the application (i.e. it has the focus).
  2. When an application that supports _NET_WM_USER_TIME launches, it sets the property on the top-level window to the time of the user interaction that caused the launch. This time is sometimes difficult for the application to determine. Some toolkits will set it to the first X11 protocol timestamp that it obtains during startup, others will use the xdg startup notification id (which contains the timestamp of the event that launched the application). I think I put standard code in icewm to take the timestamp from startup notification and use it if the time is earlier than the timestamp set on _NET_WM_USER_TIME for the corresponding application, or, when _NET_WM_USER_TIME is zero (CurrentTime) or when the application does not support _NET_WM_USER_TIME at all.
  3. When the application launches and maps a top-level window, and after reparenting, the WM that supports _NET_WM_USER_TIME will check to see if the initial launch time associated with the new client is older than the latest update to _NET_WM_USER_TIME for existing clients. If it is older, it does not give the window the focus. As a special case, if the application set _NET_WM_USER_TIME to zero (CurrentTime), the window manager should not give the window the focus regardless of the setting of the _NET_WM_USER_TIME property on other windows.

Basically the idea here is that if I type at an xterm:

$> libreoffice &

and then start typing some more, when the main window appears for libreoffice, it should not steal the focus from the xterm while I am actively typing the in the xterm.

Chances are the last != should be a < (in the modulo sense) in the conditional you have commented out.

You can find the specification of _NET_WM_USER_TIME in the EWMH/NetWM specification.

Owner

bbidulock commented Aug 15, 2015

Well, all that does is shut off _NET_WM_USER_TIME support.

_NET_WM_USER_TIME, as far as I understand it, is supposed to work like so:

  1. An application that supports _NET_WM_USER_TIME sets the property of the same name on its top-level windows whenever there is user input (key or button) within the application (i.e. it has the focus).
  2. When an application that supports _NET_WM_USER_TIME launches, it sets the property on the top-level window to the time of the user interaction that caused the launch. This time is sometimes difficult for the application to determine. Some toolkits will set it to the first X11 protocol timestamp that it obtains during startup, others will use the xdg startup notification id (which contains the timestamp of the event that launched the application). I think I put standard code in icewm to take the timestamp from startup notification and use it if the time is earlier than the timestamp set on _NET_WM_USER_TIME for the corresponding application, or, when _NET_WM_USER_TIME is zero (CurrentTime) or when the application does not support _NET_WM_USER_TIME at all.
  3. When the application launches and maps a top-level window, and after reparenting, the WM that supports _NET_WM_USER_TIME will check to see if the initial launch time associated with the new client is older than the latest update to _NET_WM_USER_TIME for existing clients. If it is older, it does not give the window the focus. As a special case, if the application set _NET_WM_USER_TIME to zero (CurrentTime), the window manager should not give the window the focus regardless of the setting of the _NET_WM_USER_TIME property on other windows.

Basically the idea here is that if I type at an xterm:

$> libreoffice &

and then start typing some more, when the main window appears for libreoffice, it should not steal the focus from the xterm while I am actively typing the in the xterm.

Chances are the last != should be a < (in the modulo sense) in the conditional you have commented out.

You can find the specification of _NET_WM_USER_TIME in the EWMH/NetWM specification.

@williex

This comment has been minimized.

Show comment
Hide comment
@williex

williex Aug 16, 2015

Thanks for the thorough explanation. I've tried to run libreoffice in the background like you suggested and with the unpatched/vanilla 1.3.10 release (with _NET_WM_USER_TIME included) the libreoffice window takes focus, even when I keep on typing. So for me the code doesn't seem to be working as intended. Changing != to < did not make a difference.

I also noticed the problem happens when there is already a window of the same program open. So, if I have a firefox window open, sometimes a new firefox window doesn't get focus. But if it's the first firefox window to open, it always gets focus.

I wonder does the net_wm code work for you? If so, it might be something different on my system causing this issue and not something in the icewm code itself.

williex commented Aug 16, 2015

Thanks for the thorough explanation. I've tried to run libreoffice in the background like you suggested and with the unpatched/vanilla 1.3.10 release (with _NET_WM_USER_TIME included) the libreoffice window takes focus, even when I keep on typing. So for me the code doesn't seem to be working as intended. Changing != to < did not make a difference.

I also noticed the problem happens when there is already a window of the same program open. So, if I have a firefox window open, sometimes a new firefox window doesn't get focus. But if it's the first firefox window to open, it always gets focus.

I wonder does the net_wm code work for you? If so, it might be something different on my system causing this issue and not something in the icewm code itself.

@sebastianopilla

This comment has been minimized.

Show comment
Hide comment
@sebastianopilla

sebastianopilla Mar 29, 2016

I have the same kind of issue on my system (CentOS 7.2 and icewm 1.3.12): when I double-click on a PDF file in my file manager, the PDF reader opens but the title bar is grayed out (the program has no focus) and the taskbar entry flashes.

This happens also for windows for the same program, like in the comment above: for example, in SpaceFM I hit F2 to rename a file and the rename dialog doesn't have focus and the taskbar entry is flashing.

I have the same kind of issue on my system (CentOS 7.2 and icewm 1.3.12): when I double-click on a PDF file in my file manager, the PDF reader opens but the title bar is grayed out (the program has no focus) and the taskbar entry flashes.

This happens also for windows for the same program, like in the comment above: for example, in SpaceFM I hit F2 to rename a file and the rename dialog doesn't have focus and the taskbar entry is flashing.

@bbidulock bbidulock added the bug label Mar 29, 2016

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Mar 29, 2016

Owner

Looks like icewm has a lot of focus problems. It is not conforming to ICCCM 2.0. Because focus-setting is strewn around the code under all manner of conditionals, it will take some time and effort to fix.

Owner

bbidulock commented Mar 29, 2016

Looks like icewm has a lot of focus problems. It is not conforming to ICCCM 2.0. Because focus-setting is strewn around the code under all manner of conditionals, it will take some time and effort to fix.

@bedna-KU

This comment has been minimized.

Show comment
Hide comment
@bedna-KU

bedna-KU May 1, 2016

Contributor

After I upgrade on IceWM 1.3.12.34 (I dont know previous version) and open sub window aplication, has this subwindow no focus.
Affected applications FileZilla, SpaceFM, Geany, Textadept, Pidgin, HexChat, Medit ...

Roxterm not gained focus at startup in all versions.

I test preferences from mainline and no change.

Contributor

bedna-KU commented May 1, 2016

After I upgrade on IceWM 1.3.12.34 (I dont know previous version) and open sub window aplication, has this subwindow no focus.
Affected applications FileZilla, SpaceFM, Geany, Textadept, Pidgin, HexChat, Medit ...

Roxterm not gained focus at startup in all versions.

I test preferences from mainline and no change.

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock May 2, 2016

Owner

Yes, IceWM is not transfering focus properly, nor does it handle application-modal dialogs properly. Lots of work to fix it (time that I don't have right now).

Owner

bbidulock commented May 2, 2016

Yes, IceWM is not transfering focus properly, nor does it handle application-modal dialogs properly. Lots of work to fix it (time that I don't have right now).

@bedna-KU

This comment has been minimized.

Show comment
Hide comment
@bedna-KU

bedna-KU May 3, 2016

Contributor

I have on other computer IceWM 1.3.12.32 and focus work fine. (only roxterm not gained focus)
https://vimeo.com/165105955

Contributor

bedna-KU commented May 3, 2016

I have on other computer IceWM 1.3.12.32 and focus work fine. (only roxterm not gained focus)
https://vimeo.com/165105955

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock May 3, 2016

Owner

The issue might be timing related. IceWM is setting focus to window before issuing WM_TAKE_FOCUS command, which is incorrect per ICCCM 2.0 and might be confusing some toolkits but depends on race condition. Unfortunately focus transfer is about 5 levels deep in inheritance and ti is difficult to even establish which code is running.

Owner

bbidulock commented May 3, 2016

The issue might be timing related. IceWM is setting focus to window before issuing WM_TAKE_FOCUS command, which is incorrect per ICCCM 2.0 and might be confusing some toolkits but depends on race condition. Unfortunately focus transfer is about 5 levels deep in inheritance and ti is difficult to even establish which code is running.

PetteriAimonen added a commit to PetteriAimonen/icewm that referenced this issue Mar 1, 2017

Add missing initialization of fLastUserTime.
This seemed to cause the fLastUserTime to be some garbage
value such as "0x654b20230a202023", thus causing comparison failure
in wmframe.cc line 1858. Possible cause of issue #37, but I'll have
to test it longer to be sure.
@PetteriAimonen

This comment has been minimized.

Show comment
Hide comment
@PetteriAimonen

PetteriAimonen Mar 1, 2017

Contributor

Hmm, is there a missing initialization of fLastUserTime in wmmgr.c? I was printing the timestamps for debugging and was getting garbage values like 0x654b20230a202023, this seems to fix that part:
PetteriAimonen@a6ac9e2

Not sure if it actually fixes the whole problem. Need more time to know, because it starts occurring only approximately once a month for me and usually disappears when I start debugging.

Contributor

PetteriAimonen commented Mar 1, 2017

Hmm, is there a missing initialization of fLastUserTime in wmmgr.c? I was printing the timestamps for debugging and was getting garbage values like 0x654b20230a202023, this seems to fix that part:
PetteriAimonen@a6ac9e2

Not sure if it actually fixes the whole problem. Need more time to know, because it starts occurring only approximately once a month for me and usually disappears when I start debugging.

bbidulock added a commit that referenced this issue Mar 3, 2017

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Mar 3, 2017

Owner

Thanks for the patch, @PetteriAimonen

Owner

bbidulock commented Mar 3, 2017

Thanks for the patch, @PetteriAimonen

@bedna-KU

This comment has been minimized.

Show comment
Hide comment
@bedna-KU

bedna-KU Mar 4, 2017

Contributor

Yes this works OK, thanks @PetteriAimonen.

Contributor

bedna-KU commented Mar 4, 2017

Yes this works OK, thanks @PetteriAimonen.

@PetteriAimonen

This comment has been minimized.

Show comment
Hide comment
@PetteriAimonen

PetteriAimonen Apr 11, 2017

Contributor

Doesn't appear to be full fix though, issue reoccurred for me just now.

I have this debug print in updateFocusOnMap:

fprintf(stderr, "time %llx, %llx\n", (long long unsigned)fUserTime, (long long unsigned)manager->lastUserTime());

and I'm getting this in my logs now:

time ffffffff82721cf3, 7ffd6dda
time ffffffff82724e24, 7ffd6dda
time ffffffff827301fd, 7ffd6dda

Last lines before problem and first lines of the problem:

time 7ffb46fb, 7ffb46fb
time 7ffba7b9, 7ffba7b9
time 7ffd6dda, 7ffd6dda
time ffffffff800a1081, 7ffd6dda
time ffffffff800a1081, 7ffd6dda
time ffffffff800b1372, 7ffd6dda

Seems like some kind of signed vs. unsigned extension to 64 bits problem.

Contributor

PetteriAimonen commented Apr 11, 2017

Doesn't appear to be full fix though, issue reoccurred for me just now.

I have this debug print in updateFocusOnMap:

fprintf(stderr, "time %llx, %llx\n", (long long unsigned)fUserTime, (long long unsigned)manager->lastUserTime());

and I'm getting this in my logs now:

time ffffffff82721cf3, 7ffd6dda
time ffffffff82724e24, 7ffd6dda
time ffffffff827301fd, 7ffd6dda

Last lines before problem and first lines of the problem:

time 7ffb46fb, 7ffb46fb
time 7ffba7b9, 7ffba7b9
time 7ffd6dda, 7ffd6dda
time ffffffff800a1081, 7ffd6dda
time ffffffff800a1081, 7ffd6dda
time ffffffff800b1372, 7ffd6dda

Seems like some kind of signed vs. unsigned extension to 64 bits problem.

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Apr 12, 2017

Owner

You are printing a 32-bit unsigned number as a 64-bit signed number. Don't do that. Try:

fprintf(stderr, "time %lx, %lx\n", fUserTime, manager->lastUserTime());

They are, after all, both (unsigned long) not (long long unsigned).

Owner

bbidulock commented Apr 12, 2017

You are printing a 32-bit unsigned number as a 64-bit signed number. Don't do that. Try:

fprintf(stderr, "time %lx, %lx\n", fUserTime, manager->lastUserTime());

They are, after all, both (unsigned long) not (long long unsigned).

@PetteriAimonen

This comment has been minimized.

Show comment
Hide comment
@PetteriAimonen

PetteriAimonen Apr 12, 2017

Contributor

I'm casting them up to long long unsigned, AFAIK that way any smaller type should be printed accurately.

Contributor

PetteriAimonen commented Apr 12, 2017

I'm casting them up to long long unsigned, AFAIK that way any smaller type should be printed accurately.

@PetteriAimonen

This comment has been minimized.

Show comment
Hide comment
@PetteriAimonen

PetteriAimonen Apr 12, 2017

Contributor

Hmm, I think I found atleast part of the problem:

  1. On Linux x86_64, sizeof(long) == 8
  2. X11 protocol was designed with assumption that sizeof(long) == 4, and was then updated so that the API uses longs, but only lower 32 bits are used (https://groups.google.com/forum/#!topic/fltkcoredev/xe3m8d9P8ic)
  3. In wmclient.cc getNetWMUserTime() the 32-bit time value is read as long, which X11 has presumably sign-extended to 64 bits. This gives 0xffffffff800a1081 once the X server has been running for more than 25 days.
  4. In wmmgr.cc updateUserTime() there are a few comparisons that attempt to handle the value overflowing after 50 days. These however do not work when the variable is 64 bits in size.

Not sure what is the best way to fix this..

Contributor

PetteriAimonen commented Apr 12, 2017

Hmm, I think I found atleast part of the problem:

  1. On Linux x86_64, sizeof(long) == 8
  2. X11 protocol was designed with assumption that sizeof(long) == 4, and was then updated so that the API uses longs, but only lower 32 bits are used (https://groups.google.com/forum/#!topic/fltkcoredev/xe3m8d9P8ic)
  3. In wmclient.cc getNetWMUserTime() the 32-bit time value is read as long, which X11 has presumably sign-extended to 64 bits. This gives 0xffffffff800a1081 once the X server has been running for more than 25 days.
  4. In wmmgr.cc updateUserTime() there are a few comparisons that attempt to handle the value overflowing after 50 days. These however do not work when the variable is 64 bits in size.

Not sure what is the best way to fix this..

PetteriAimonen added a commit to PetteriAimonen/icewm that referenced this issue Apr 12, 2017

Truncate time values to 32 bits in wmclient.
X11 protocol passes time values as 32 bit signed integers, stored in 64 bit longs.
The icewm code internally uses unsigned longs for time values.
Direct casting would result 0xffffffff in the upper bytes, which messes up
comparisons.

This commit adds a mask that will force the upper bytes to 0.

Partial fix for issue #37, might still have some problems on the 50 day overflow.

bbidulock added a commit that referenced this issue Apr 13, 2017

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Apr 13, 2017

Owner

I think that will fix it.

Owner

bbidulock commented Apr 13, 2017

I think that will fix it.

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Apr 28, 2017

Owner

Did this change fix the overall issue?

Owner

bbidulock commented Apr 28, 2017

Did this change fix the overall issue?

@gijsbers

This comment has been minimized.

Show comment
Hide comment
@gijsbers

gijsbers May 2, 2017

Collaborator

For me it doesn't.
It may actually have made things worse?
I have the following settings:

ClickToFocus=0
FocusOnMap=1

but often I need to move the mouse into a new window in order to activate it.

On an empty desktop if I hotkey a new terminal it never gets focus, not if it is under the mouse and not if it is somewhere else. Only if I have a terminal with focus and hotkey a new terminal over it then the new terminal starts with focus.

To me the icewm from about two months ago had better focus behavior than the current one.

Collaborator

gijsbers commented May 2, 2017

For me it doesn't.
It may actually have made things worse?
I have the following settings:

ClickToFocus=0
FocusOnMap=1

but often I need to move the mouse into a new window in order to activate it.

On an empty desktop if I hotkey a new terminal it never gets focus, not if it is under the mouse and not if it is somewhere else. Only if I have a terminal with focus and hotkey a new terminal over it then the new terminal starts with focus.

To me the icewm from about two months ago had better focus behavior than the current one.

@gijsbers

This comment has been minimized.

Show comment
Hide comment
@gijsbers

gijsbers Jun 2, 2017

Collaborator

Commit 931c814 fixes the FocusOnMap problem for me. I also tried to reproduce the audacious scenario, but that works fine for me as well.

Collaborator

gijsbers commented Jun 2, 2017

Commit 931c814 fixes the FocusOnMap problem for me. I also tried to reproduce the audacious scenario, but that works fine for me as well.

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Jun 16, 2017

Owner

Does this fix the issue for others as well?

Owner

bbidulock commented Jun 16, 2017

Does this fix the issue for others as well?

@PetteriAimonen

This comment has been minimized.

Show comment
Hide comment
@PetteriAimonen

PetteriAimonen Jul 15, 2017

Contributor

Hmm, after 36 days of uptime, I'm getting this problem yet again. So I guess the fix is still not perfect, will try to debug more later.

Contributor

PetteriAimonen commented Jul 15, 2017

Hmm, after 36 days of uptime, I'm getting this problem yet again. So I guess the fix is still not perfect, will try to debug more later.

@gijsbers

This comment has been minimized.

Show comment
Hide comment
@gijsbers

gijsbers Jul 16, 2017

Collaborator

Counting milliseconds in a 32-bit number is bound to set us up for problems like this.
We may wish to extend the X11 Time value with a time_t time(2) seconds,
so we can take into account the X server time wrapping around.
Or use a struct timeval or a proper 64-bit counter.
Would be interesting to learn how other WMs deal with this.

Collaborator

gijsbers commented Jul 16, 2017

Counting milliseconds in a 32-bit number is bound to set us up for problems like this.
We may wish to extend the X11 Time value with a time_t time(2) seconds,
so we can take into account the X server time wrapping around.
Or use a struct timeval or a proper 64-bit counter.
Would be interesting to learn how other WMs deal with this.

@gijsbers

This comment has been minimized.

Show comment
Hide comment
@gijsbers

gijsbers Jul 17, 2017

Collaborator

To resolve this issue structurally and support runtimes of many months, could we take into account the time at which we come to know about a timestamp?

A timestamp then consists of two parts. The user time / startup time as given by the X server time stamp, which is a 32-bit count of milliseconds, and a time in seconds when this value was received by the window manager. We are only interested in comparing two such combined timestamps, not in their absolute differences. A comparison then is a two-step process. First we compare the two time-in-seconds values. If their absolute difference is greater than the length of a predefined interval then we use only their values to decide the outcome of the comparison. Otherwise we look at the 32-bit X server timestamps in milliseconds for the comparison outcome, taking into account wrapping. We assume that this is safe to do, because we assume that they are always less than 31-bits apart in time given an appropriate length of the came-to-know-about-it seconds-interval.

How large should the seconds interval be? There must be some slack between the creation of the 32-bit X millitime and its reception by the window manager. Let's say 1 million seconds is a safe bet. This is 1 billion milliseconds, or less than 30 bits of milliseconds, or 11.57 days. We could round this upwards to a nice number like 2 or 3 weeks or so, no prob.

Then we can have uptimes of years, leave computers unattended for months, and still have everything working as expected. Comments? Too good to be true?

Collaborator

gijsbers commented Jul 17, 2017

To resolve this issue structurally and support runtimes of many months, could we take into account the time at which we come to know about a timestamp?

A timestamp then consists of two parts. The user time / startup time as given by the X server time stamp, which is a 32-bit count of milliseconds, and a time in seconds when this value was received by the window manager. We are only interested in comparing two such combined timestamps, not in their absolute differences. A comparison then is a two-step process. First we compare the two time-in-seconds values. If their absolute difference is greater than the length of a predefined interval then we use only their values to decide the outcome of the comparison. Otherwise we look at the 32-bit X server timestamps in milliseconds for the comparison outcome, taking into account wrapping. We assume that this is safe to do, because we assume that they are always less than 31-bits apart in time given an appropriate length of the came-to-know-about-it seconds-interval.

How large should the seconds interval be? There must be some slack between the creation of the 32-bit X millitime and its reception by the window manager. Let's say 1 million seconds is a safe bet. This is 1 billion milliseconds, or less than 30 bits of milliseconds, or 11.57 days. We could round this upwards to a nice number like 2 or 3 weeks or so, no prob.

Then we can have uptimes of years, leave computers unattended for months, and still have everything working as expected. Comments? Too good to be true?

@PetteriAimonen

This comment has been minimized.

Show comment
Hide comment
@PetteriAimonen

PetteriAimonen Jul 17, 2017

Contributor

@gijsbers Seems reasonable, though that might require quite a lot of rework because the Time values are used in many places throughout the code, and the type comes from X11 headers and is only 32 bit on 32 bit machines.

I think the remaining problem is in YWindowManager::updateUserTime() which compares against 0x7fffffff. It seems that the purpose of the function is to update fLastUserTime if the parameter time is newer than old value.

The current code (https://github.com/bbidulock/icewm/blob/icewm-1-3-BRANCH/src/wmmgr.cc#L3009) does this by checking (time - fLastUserTime) < 0x7fffffff. Currently for my system, time = 0xc04b79df and fLastUserTime = 0x56c46a6, so the check fails because it considers that time value is from before a wraparound and fLastUserTime is from after it, and doesn't update it.

How did it arrive in this situation? I don't know exactly, but looks like at some point I activated a very old window, got a very old time event and it thought that it had wrapped around:

time=b0697f04, fLastUserTime=b0697f04
time=56c46a6, fLastUserTime=56c46a6
time=b069b781, fLastUserTime=56c46a6

So I think the remaining bug now only appears if one activates a window that has been untouched for atleast 24 days - not that rare a thing for me, as I tend to use virtual desktops extensively and leave apps running there.

But not sure how to fix this, because X11 only stores the 32 bit timestamp, there is no easy way to know whether the stored timestamp is from this wrap period or not. I guess one way would be if there is a way to get the current timestamp separately from the UserTime (which is window dependent) - then the check could be e.g. abs(time - current_time) < 10000.

The good thing is that the remaining issue seems to be fixed by just restarting icewm from the menu, no need to close applications.

Contributor

PetteriAimonen commented Jul 17, 2017

@gijsbers Seems reasonable, though that might require quite a lot of rework because the Time values are used in many places throughout the code, and the type comes from X11 headers and is only 32 bit on 32 bit machines.

I think the remaining problem is in YWindowManager::updateUserTime() which compares against 0x7fffffff. It seems that the purpose of the function is to update fLastUserTime if the parameter time is newer than old value.

The current code (https://github.com/bbidulock/icewm/blob/icewm-1-3-BRANCH/src/wmmgr.cc#L3009) does this by checking (time - fLastUserTime) < 0x7fffffff. Currently for my system, time = 0xc04b79df and fLastUserTime = 0x56c46a6, so the check fails because it considers that time value is from before a wraparound and fLastUserTime is from after it, and doesn't update it.

How did it arrive in this situation? I don't know exactly, but looks like at some point I activated a very old window, got a very old time event and it thought that it had wrapped around:

time=b0697f04, fLastUserTime=b0697f04
time=56c46a6, fLastUserTime=56c46a6
time=b069b781, fLastUserTime=56c46a6

So I think the remaining bug now only appears if one activates a window that has been untouched for atleast 24 days - not that rare a thing for me, as I tend to use virtual desktops extensively and leave apps running there.

But not sure how to fix this, because X11 only stores the 32 bit timestamp, there is no easy way to know whether the stored timestamp is from this wrap period or not. I guess one way would be if there is a way to get the current timestamp separately from the UserTime (which is window dependent) - then the check could be e.g. abs(time - current_time) < 10000.

The good thing is that the remaining issue seems to be fixed by just restarting icewm from the menu, no need to close applications.

@gijsbers

This comment has been minimized.

Show comment
Hide comment
@gijsbers

gijsbers Jul 17, 2017

Collaborator

@PetteriAimonen I think you point out the right line of code.
For your second observation there is a way to get the current timestamp from xapp->getEventTime(0).

Collaborator

gijsbers commented Jul 17, 2017

@PetteriAimonen I think you point out the right line of code.
For your second observation there is a way to get the current timestamp from xapp->getEventTime(0).

@gijsbers

This comment has been minimized.

Show comment
Hide comment
@gijsbers

gijsbers Jul 24, 2017

Collaborator

I think line 3009 is a solution for 32-bit Time, but not for 64-bit:

if (fLastUserTime == 0 || (time - fLastUserTime) < 0x7fffffff)

But by enforcing 32-bit we may get by:

 if (fLastUserTime == 0 || ((time - fLastUserTime) & 0xFFFFFFFF) < 0x7fffffff)

Your idea of considering current_time is very interesting.

Collaborator

gijsbers commented Jul 24, 2017

I think line 3009 is a solution for 32-bit Time, but not for 64-bit:

if (fLastUserTime == 0 || (time - fLastUserTime) < 0x7fffffff)

But by enforcing 32-bit we may get by:

 if (fLastUserTime == 0 || ((time - fLastUserTime) & 0xFFFFFFFF) < 0x7fffffff)

Your idea of considering current_time is very interesting.

gijsbers added a commit that referenced this issue Jul 24, 2017

Enforce 32-bit unsigned comparison of X11 time stamps in updateUserTime.
This improves availability of focused windows in issue #37.
@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Aug 1, 2017

Owner

Has this issue been resolved?

Owner

bbidulock commented Aug 1, 2017

Has this issue been resolved?

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Oct 16, 2017

Owner

No response, I will consider it closed.

Owner

bbidulock commented Oct 16, 2017

No response, I will consider it closed.

@bbidulock bbidulock closed this Oct 16, 2017

@PetteriAimonen

This comment has been minimized.

Show comment
Hide comment
@PetteriAimonen

PetteriAimonen Oct 16, 2017

Contributor

Yeah, not perfect if there exist very old windows but at least it is better than before. Not sure if there is some trick to improve it further, but works for me.

Contributor

PetteriAimonen commented Oct 16, 2017

Yeah, not perfect if there exist very old windows but at least it is better than before. Not sure if there is some trick to improve it further, but works for me.

@bbidulock

This comment has been minimized.

Show comment
Hide comment
@bbidulock

bbidulock Oct 16, 2017

Owner

Reopen if and when necessary.

Owner

bbidulock commented Oct 16, 2017

Reopen if and when necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment