[OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
 

Before this patch, the timestamps recorded for DragDrop events in the win32 implementation were not comparable to the timestamps recorded for regular mouse or keyboard events. This was the case because it used ioMicroMSecs() which returns the milliseconds elapsed since the VM was started (see sqWin32Heartbeat.c). However, for other events, the timestamp is retrieved from the MSG structure where the time is counted since the system start.

In the DnD module (see sqWin32DragDrop.c), no MSG instances are retrieved because it is implemented based on the IDropTarget interface from the ole2.h header instead of Winuser.h. Nevertheless, we can get the required timestamp using the GetTickCount() function from sysinfoapi.h which returns the milliseconds elapsed since the system start as well. By the way, it is already used for recording window events (see recordWindowEvent(), however, as far as I see, its use would not even have been necessary in this case because the single caller of that function could also have passed messageTouse->time to it).

Closes #509.

How can I test this?

For observing the consequences of this PR, please install this changeset into both an unpatched and patched VM and watch the outputs in the Transcript window while moving your mouse and dragging files from your Win32 host system over/into the image (works best when disabling the 'TranscriptStream forceUpdate' preference).

Merger notes

Please review!
And please squash this PR when merging. :-)


You can view, comment on, or merge this pull request online at:

  https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518

Commit Summary

  • Early WIP
  • Merge branch 'Cog' into fix-win-evt-timestamps
  • Fix timestamps for DragDrop events on Windows
  • Use GetTickCount() instead of GetMessageTime().
  • [skip-ci] Improve comment

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
 

Ok, this is funny. On non-windows the io.. ticks are used nearly exclusively for eventing. But if that's not helful ond Win32, lets go with the flow. @dtlewis290 , Do you know any reasons, why not to use the Windows time stamps?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690597585", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690597585", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

@krono It's just a theory, but an advantage of using msg->time over ioMSecs etc. could be the fact that even if the event queue got blocked for some time, the VM is able to deliver the correct timestamps to the image. See https://docs.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-msg#members:~:text=member.-,time,The%20time%20at%20which%20the%20message%20was%20posted.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690610599", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690610599", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

Makes sense; however, I think the Image expects io ticks… so I paged dave


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690616864", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690616864", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

The image simply expects an integer timeStamp relative to some (any) consistent start point. I expect no issues on the image side as long as events are milliseconds relative to the same basis. I am not a Windows expert, but the the Windows VM uses event timestamps from Windows events, and if those values are relative to system start time, then that is the way it should. Proposed patch as described above seems reasonable and good. As an aside, it might also be worth taking a look at ioMsecs() in the Windows VM to see if it could be changed to use the same time basis as Windows events (system startup time rather than VM startup time). If that could be done, then fixes like this would probably not be necessary. But don't let that suggestion stand in the way of integrating a good patch.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690796194", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690796194", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

On a hunch I looked at ioMSecs() in the SVN repository, see line 1426 in the file at http://squeakvm.org/cgi-bin/viewvc.cgi/squeak/trunk/platforms/win32/vm/sqWin32Window.c?revision=2921&view=markup.

This implements ioMSecs() as GetTickCount() &0x3FFFFFFF which is relative to system startup time. Thus the Windows VM originally had an ioMSecs() that was consistent with the time scale in the win32 MSG struct. Later changes to the platforms code may have changed this such that ioMSecs() no longer matches the time basis of the Windows events.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690832397", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690832397", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

Sorry, I was citing ioMSec(), should have referenced ioMicroMSecs(). But in both cases, the time basis is system start time rather than VM start time, which is consistent with the time scale of a timestamps in a MSG struct. So in the earlier VMs we had:

/* Note: ioMicroMSecs returns milliseconds /
int ioMicroMSecs(void)
{
/
Make sure the value fits into Squeak SmallIntegers */
return timeGetTime() &0x3FFFFFFF;
}


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690840933", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690840933", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

Ok, I'll merge.

@LinqLover, care to look at Daves proposal re ioMicroMSecs? I think the Mask is depending on Image bittines (32 vs 64) but the rest looks sane.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690901334", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-690901334", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

Merged #518 into Cog.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#event-3754531253", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#event-3754531253", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

Thank you for the discussion and, first of all, for merging a working patch. :-)

But it's true, Time eventMillisecondClock and ActiveHand lastEvent timeStamp are not comparable at the moment on Windows, which is suboptimal. Do you propose simply to restore the old versions from http://squeakvm.org/cgi-bin/viewvc.cgi/squeak/trunk/platforms/win32/vm/sqWin32Window.c?revision=3784&view=markup#l1420? I would like to read some changelogs documenting the changes or to run some regression tests before reverting, but I could not find any of both ...


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691022783", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691022783", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

WinCE is dead, but yes, I'd say use that version but make sure the Mask is sensible. It is currently 32bit, isn't it?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691054734", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691054734", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

To be clear - I do not propose reverting to the old version. But maybe there is a way to update the current version so that the time basis would match that of Windows events (time delta between system start time and VM start time is constant). It is just an idea, and I have not looked in detail.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691074887", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691074887", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

David, this sounds interesting. Either we have to change the timestamps of all recorded events (possibly losing accuracy), or we change the reference value provided by Time eventMillisecondClock (primitive 135/primitiveMillisecondClock). Is this what you meant? Is there any contract saying that if you start the VM, Time eventMillisecondClock should return a very small value? If not, could we just modify the Win32 implementation of ioMSecs() to return GetTickCount(), or could there be any other component depending on the current implementation?

So many questions! 😅 And here is just another one: Why does the Stack VM use a different header for the timing stuff (sqWin32Heartbeat.c vs sqWin32Time.c)?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691107144", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691107144", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

@LinqLover on closer review I believe that the fix that you provided is the best solution. I see no other cases of event creation where this would be a problem. Good fix, no further action required.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691134232", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691134232", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

So you mean there is no need to sync Time eventMillisecondClock with the timestamp provided by VM events, despite the comment in #eventMillisecondClock?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691170419", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691170419", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

I overlooked that. Let's take the question to squeak-dev and maybe loop tpr into the discussion. I do not know the answer.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691178962", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691178962", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fix timestamps for DragDrop events on Windows (#518)

David T Lewis
In reply to this post by David T Lewis
 

A bit more history - For sqWin32Window.c, I compared the squeakvm.org versions versus initial checkins here, confirming that the timer changes were done by Andreas in work prior to first public release of Cog. Files attached for reference. I am completely confident that Andreas knew exactly what he was doing WRT event timestamps, so it seems to me the right thing to do is eliminate in the image any assumption that event timestamps match ioMSec(). I expect no that further changes should be necessary in the VM.
win32Window-squeakvmorg-versus-oscog.zip


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691536698", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/518#issuecomment-691536698", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>