[OpenSmalltalk/opensmalltalk-vm] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

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

[OpenSmalltalk/opensmalltalk-vm] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

Christoph Thiede-2
 

A bit of context: For Squeak, I am currently implementing full-fledged handling of DND events sent by the host system. Actually, Squeak only handles SQDragDrop at the moment.

However, when logging all received DND events on the image side, I discovered that whenever a file is dropped into the VM, after the last SQDragMove an SQDragLeave is received before the eventual SQDragDrop. This looks very suspicious to me. Neither does the same happen on any other platform (tested on Win32, too), nor does it appear logical to me in any way. If I release the mouse button over the image, the drag did not leave, so a "drag leave" should not be recorded.

This MR makes sure that in dndInDrop(), an SQDragLeave event is only recorded if the drop was not successful. (Please note that the SQDragDrop event itself will be sent in a deferred manner; dndInDrop() itself only calls XConvertSelection() from the X-Server which then will send a SelectionNotify which will trigger dndGetSelection() and eventually have generateSqueakDropEventIfDroppedFiles() called.)

Please review carefully, as I am a bloody VM newbie and this is my very first commit to this repo! :-) Mentioning @eliotmiranda who appears to be the code owner of this plugin, according to the file's history.

Todos:

  • Think about backward compatibility: Squeak did not respect any of the drag events but SQDragDrop until today, and I'm reworking that handling ATM, so there will be no problem. But are there other users of this plugin that could rely on the old behavior which I consider a bug? Or does the sq in the prefix unambiguously stand for Squeak?
  • Are there any tests I could run to make sure that this change cannot break anything else?

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

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

Commit Summary

  • Fix a typo
  • Comment out suspicious line
  • Merge branch 'Cog' into sqUnixXdnd
  • Refactor changes
  • Don't skip SQDragLeave if XGetSelectionOwner failed

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/508", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/508", "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] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

Christoph Thiede-2
 

@LinqLover pushed 1 commit.

  • e2be1e8 Don't skip SQDragLeave if XGetSelectionOwner failed


You are receiving this because you are subscribed to this thread.
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/508/files/cb852fe26b0fa1e363bf2e62961da6644ea9833b..e2be1e85adaa5da536f665cf4a05655c0a0658da", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/508/files/cb852fe26b0fa1e363bf2e62961da6644ea9833b..e2be1e85adaa5da536f665cf4a05655c0a0658da", "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] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

Christoph Thiede-2
In reply to this post by Christoph Thiede-2
 

Ehm, all the pharo builds are failing. Is this my fault?


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/508#issuecomment-643603279", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/508#issuecomment-643603279", "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] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

Christoph Thiede-2
In reply to this post by Christoph Thiede-2
 

@nicolas-cellier-aka-nice Would you mind to have a short look at this? :-)


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/508#issuecomment-650434309", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/508#issuecomment-650434309", "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] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

Christoph Thiede-2
In reply to this post by Christoph Thiede-2
 

Concerning Pharo, you did not break anything.
External libraries are precompiled on the CI in some cache.
When version number of those libraries are update, the cache is not automatically cleared.
Therefore, the failure is postponed until the cache is cleared, which happens on a regular schedule apparently.

Concerning sq, we did not want to change each and every prefix to osvm when we switched squeak vm to osvm.
It would have touched each and every file and the VMMaker and all plugin repositories, for not much added value.
Who knows how many regressions could have been un-noticed.
So do not consider sq* as Squeak only.

Concerning DND, it's beyond my skill, I've never studied its logic nor particular sequence of events.
What you say sounds logical, and consistent VM behavior is desirable though.
So if you say that you compiled, used the VM and saw no regression in an official release (say drop a .png in Squeak 5.3), I can do the merge (and squash).

Ideally we should do it on an old version of pharo too (7?), but at one moment, pharo people shall help us to help them, and this is not the trend. The INRIA team forked, and when I request community help on Opensmalltalk-vm-dev ML, I ear nothing but a big silence, I guess no one is interested. Newspeak sounds less like a problem, because they use their own form of FFI for windows system integration rather than plugin.


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/508#issuecomment-650611001", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/508#issuecomment-650611001", "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] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

Christoph Thiede-2
In reply to this post by Christoph Thiede-2
 

@LinqLover pushed 2 commits.

  • aafdc28 Only record SQDragLeave xor SQDragDrop
  • 4def9a6 Merge remote-tracking branch 'origin/Cog' into sqUnixXdnd


You are receiving this because you are subscribed to this thread.
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/508/files/e2be1e85adaa5da536f665cf4a05655c0a0658da..4def9a65347638fe002deb5cfb92d00fa532145c", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/508/files/e2be1e85adaa5da536f665cf4a05655c0a0658da..4def9a65347638fe002deb5cfb92d00fa532145c", "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] sqUnixXdnd: Don't record SQDragLeave when XdndDrop is handled (#508)

Christoph Thiede-2
In reply to this post by Christoph Thiede-2
 

Thank you for the detailed explanations, @nicolas-cellier-aka-nice!

you say that you compiled, used the VM and saw no regression in an official release (say drop a .png in Squeak 5.3)

Yes, I confirm that.

Newspeak sounds less like a problem, because they use their own form of FFI for windows system integration rather than plugin.

What is about Cuis?

consistent VM behavior is desirable though

Hm, please don't merge yet. I just found further small inconsistencies in this implementation I would like to fix within this PR. I will update the description when I am done and mention you again. :-)


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/508#issuecomment-650783530", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/508#issuecomment-650783530", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>