Skip to content
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

Change ImageLoader importing #105

Open
wants to merge 2 commits into
base: 4.7
Choose a base branch
from

Conversation

summer-alice
Copy link
Contributor

@summer-alice summer-alice commented Jun 27, 2021

This will add a .all for org.eclipse.swt.internal.image, which is then imported by the ImageLoader class.

As I've mentioned in #103 and #104, when attempting to add a new constructor to the Image class (specifically Image(Device, ImageDataProvider), I encountered a cyclic dependency between DPIUtil and JPEGFileFormat.

The main cause was in the FileFormat class which, in DWT, imports the JPEGFileFormat class, whereas SWT doesn't.

SWT uses Class.forName to dynamically create an instance of the file format. I changed DWT to use the same (after implementing the forName method), but -- since FileFormat couldn't import JPEGFileFormat -- DWT couldn't resolve the symbol.

ImageLoader is the only place where SWT imports all from the org.eclipse.swt.internal.image package, so I changed the import here to be the same. This then made everything work (locally).

Edit: (corrected cyclic dependency)
Here is the error that would occur:

object.Error@src/rt/minfo.d(371): Cyclic dependency between module constructors/destructors of org.eclipse.swt.internal.DPIUtil and org.eclipse.swt.internal.image.JPEGFileFormat
org.eclipse.swt.internal.DPIUtil* ->
org.eclipse.swt.graphics.ImageData ->
org.eclipse.swt.graphics.ImageDataLoader ->
org.eclipse.swt.graphics.ImageLoader ->
org.eclipse.swt.internal.image.FileFormat ->
org.eclipse.swt.internal.image.JPEGFileFormat* ->
org.eclipse.swt.internal.image.FileFormat ->
org.eclipse.swt.graphics.ImageData ->
org.eclipse.swt.graphics.Image ->
org.eclipse.swt.internal.DPIUtil*

This change is required to allow a call to Object.factory to work
as intended.

For such calls to work, D needs to know about the module (i.e.
the symbol needs to be resolvable). This specific change is in
relation to a change that will happen in the FileFormat class
which will create a child of FileFormat dynamically.
@jacob-carlborg
Copy link
Member

jacob-carlborg commented Jun 27, 2021

This will add a .all for org.eclipse.swt.internal.image

Thinking out loud: would it be better to add a package module?

I encountered a cyclic dependency between DPIUtil and JPEGFileFormat

Cyclic dependencies are caused by module constructors. I had a quick glance at the module constructor in JPEGFileFormat. I looks like the initialization can be done at CTFE instead and remove the module constructor.

@summer-alice
Copy link
Contributor Author

This will add a .all for org.eclipse.swt.internal.image

Thinking out loud: would it be better to add a package module?

I'd be happy with that. I only chose all was because it's used elsewhere.

I encountered a cyclic dependency between DPIUtil and JPEGFileFormat

[...] I looks like the initialization can be done at CTFE instead and remove the module constructor.

Sorry, not entirely sure what you mean by this. What counts as part of the module constructor?

@jacob-carlborg
Copy link
Member

Sorry, not entirely sure what you mean by this. What counts as part of the module constructor?

A module constructor is declared using static this. For example:

@summer-alice
Copy link
Contributor Author

Sorry, not entirely sure what you mean by this. What counts as part of the module constructor?

A module constructor is declared using static this. For example:

Ah, okay. I didn't realise a static this in a class was considered a module constructor.

While I'd be (mostly) fine moving the code out, I thought the general idea was to keep the code as close to Java as possible? The local fix I have does this — the main part being that FileFormat can dynamically create instances of each supported image format, without importing the modules directly.

@jacob-carlborg
Copy link
Member

Ah, okay. I didn't realise a static this in a class was considered a module constructor.

There's no semantic difference of placing a static this inside or outside a class, IIRC.

While I'd be (mostly) fine moving the code out, I thought the general idea was to keep the code as close to Java as possible?

Yes, but correctness first. I was thinking this issue was in the correctness category. But, of course, it can be solved in different ways.

@summer-alice summer-alice mentioned this pull request Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants