-
Notifications
You must be signed in to change notification settings - Fork 183
Prepare TransferData for GTK4 support #2540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare TransferData for GTK4 support #2540
Conversation
Test Results 118 files ±0 118 suites ±0 12m 7s ⏱️ -34s For more details on these errors, see this check. Results for commit 0a5f620. ± Comparison against base commit 34becc0. ♻️ This comment has been updated with latest results. |
2559e0f
to
6b16cfb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not ready for review. Despite TransferData.type being commented:
The type is a unique identifier of a system format or user defined format. (Warning: This field is platform dependent)
The type field is actually accessed from non-Platform dependent code and I missed that before submitting this.
Its just for debugging AFAICT, but since type
is available on all platforms today perhaps I should just leave that outside of the GTK3/GTK4 structures. More thought required.
...es/org.eclipse.swt/Eclipse SWT Drag and Drop/common/org/eclipse/swt/dnd/DragSourceEvent.java
Outdated
Show resolved
Hide resolved
...es/org.eclipse.swt/Eclipse SWT Drag and Drop/common/org/eclipse/swt/dnd/DropTargetEvent.java
Outdated
Show resolved
Hide resolved
...es/org.eclipse.swt/Eclipse SWT Drag and Drop/common/org/eclipse/swt/dnd/DropTargetEvent.java
Outdated
Show resolved
Hide resolved
6b16cfb
to
60bd3dd
Compare
This is what I did for now - but I think it would be nicer if TransferData had a toString that the DropTargetEvent.toString called instead of looking into the private data. |
@akurtakov Please let me know if such a refactor is acceptable or if I need to find a different way for GTK4 and GTK3 to co-exist in TransferData |
For "Compiler — 2 new issues, 19 tota"l check I see this (I assume the top two are the new ones). But this seems incorrect as we are using ![]() The continuous-integration/jenkins/pr-head and Jenkins failed too, but AFAICT that is the same issue as above. Any help gratefully received. |
The fields in TransferData are platform specific - but GTK3 and GTK4 share the same class. The fields needed for GTK4 are quite different than for GTK3. Therefore this change refactors TransferData's internals to make it ready for GTK4 better. This is achieved by splitting out all existing fields in a new GTK3 specific subtype, and including a starting point for GTK4 fields. Some type checking is done to make sure that the correct object is used on GTK3 vs GTK4 respectively and this is done via the newly introduced gtk3() and gtk4() methods. For now the GTK4 has no fields, but it will eventually have the internal fields needed for GTK4 serializing and deserializing of objects from the native side. Part of #2126 Split out of #2538
60bd3dd
to
0a5f620
Compare
@jonahgraham The new compiler warnings are because there are unused imports. You have TransferData in the same package so there is no need to import it, importing it's inner classes is not needed either as you don't refer to their types at all in the respective classes. Long story short - just delete the 2 unused imports (jdt even gives warnings for them) and warnings are gone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite an unusual way to do it for SWT.
What I am missing here is what will TransferData on Gtk 4 contain? Will it be that different ?
The usual approach is to keep the fields that are there, add the newly needed ones and set them when needed. If needed the classes using it can have if (GTK4) setPropertyA else setPropertyB. That would keep it more in line with the rest of the codebase.
Maybe the differences are really that big that make such approach not feasible but I am not convinced of that for now.
* @since 3.132 | ||
*/ | ||
public int length; | ||
public static class TransferDataGTK3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we don't introduce new classes/methods/fields that are accessible by endusers and are only hidden by PDE apitools as not every SWT user/developer is developing with PDE apitools and this has created some churn in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - I will keep that in mind.
Oops - I went to look again, I wasn't looking in the correct place. If I had it would have been obvious what I was doing wrong. The reason I looked at wrong thing is that the link in the output doesn't actually go to the correct place: ![]() I suspect there is supposed to be some annotation in my PR that isn't being created properly, so the link didn't go to the correct place and then I was acting blind 🤦 |
In GTK4 it will be quite different. For GTK3 the TransferData contains pointers to a block of memory For GTK it will be a handle to GdkContentSerializer or GdkContentDeserializer depending on whether we are doing java to native or the reverse.
OK. For now I am going to abandon doing it this way and we can look at applying this refactor when the Clipboard is done if we find the contents of TransferData too complicated. |
The fields in TransferData are platform specific - but GTK3 and GTK4 share the same class. The fields needed for GTK4 are quite different than for GTK3. Therefore this change refactors TransferData's internals to make it ready for GTK4 better.
This is achieved by splitting out all existing fields in a new GTK3 specific subtype, and including a starting point for GTK4 fields.
Some type checking is done to make sure that the correct object is used on GTK3 vs GTK4 respectively and this is done via the newly introduced gtk3() and gtk4() methods.
For now the GTK4 has no fields, but it will eventually have the internal fields needed for GTK4 serializing and deserializing of objects from the native side.
Part of #2126
Split out of #2538