Skip to content

Create Font handles on demand #2087

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

Merged
merged 2 commits into from
May 6, 2025

Conversation

ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented May 2, 2025

Creating a new static method to retrieve font handle. Handles will no longer be created during initialization. Also destroy condition is changed since handle being 0 doesn't mean that it was destroyed anymore.

How to test

  • Run the following snippet with following VM arguments:
-Dswt.autoScale=quarter
-Dswt.autoScale.updateOnRuntime=true
import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class SWTFontExample {
    public static void main(String[] args) {
        Display display = new Display();
        Shell shell = new Shell(display);
        shell.setText("SWT Font Example");
        shell.setSize(400, 200);
        shell.setLayout(new FillLayout());

        Label label = new Label(shell, SWT.CENTER);
        label.setText("Hello, SWT with Custom Font!");

        FontData[] fontData = label.getFont().getFontData();
        for (FontData fd : fontData) {
            fd.setHeight(20); // Set font size to 20
            fd.setStyle(SWT.BOLD); // Make it bold
        }

        Font newFont = new Font(display, fontData);
        label.setFont(newFont);

        shell.open();
        while (!shell.isDisposed()) {
            if (!display.readAndDispatch()) {
                display.sleep();
            }
        }
        newFont.dispose();
        display.dispose();
    }
}
  • The correct formatting should be applied on the text same as before this change was made.
  • This change should not produce any regression and the handle will only be created on demand not during initialization of Font object.

Copy link
Contributor

github-actions bot commented May 2, 2025

Test Results

   545 files  + 6     545 suites  +6   38m 12s ⏱️ + 4m 41s
 4 376 tests +37   4 358 ✅ +35   18 💤 +3  0 ❌  - 1 
16 643 runs  +37  16 503 ✅ +35  140 💤 +3  0 ❌  - 1 

Results for commit 1b0ba0c. ± Comparison against base commit f10721a.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Contributor

laeubi commented May 2, 2025

@ShahzaibIbrahim I once played around with making Fonts GC disposable here:

do you think that would be applicable here as well? So wrapping the (long) handle inside an FontHandle object would then also make the usage of different booleans not required, because you can have a

FontHandle handle;

if (handle == null) {
  //never initialized
}

if (handle != null && handle.pointer == 0) {
   //was initialized but disposed
}

in the code.

@ShahzaibIbrahim
Copy link
Contributor Author

do you think that would be applicable here as well? So wrapping the (long) handle inside an FontHandle object would then also make the usage of different booleans not required, because you can have a

FontHandle handle;

if (handle == null) {
  //never initialized
}

if (handle != null && handle.pointer == 0) {
   //was initialized but disposed
}

in the code.

@laeubi I personally like the idea. I could implement it, but just to be consistent with other resources in win32. I would like it to be handled as I have done with isDestoyed field.

Maybe we could do it for all the resources in a separate issue/ticket.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-268 branch 4 times, most recently from c76ad30 to d1337cf Compare May 5, 2025 11:41
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I marked the new field fontData as final (and explicitly initialized it to null in 2 constructors). The code looks OK but some tests fail because when fontData == null, the newly added static method does an illegal call.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is currently broken. The first commit is supposed to be an independent refactoring, but the code state does not compile.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-268 branch 2 times, most recently from f95af17 to 4e94765 Compare May 5, 2025 16:09
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the commit contents are the wrong way round and do not fit their commit messages: the first, refactoring commit contains the actual change (creating handles on demand) and the second change supposed to create handles on demand encapsulates the field and adds some additional checks and corrects formatting.

The handle of Font is encapsulated and exposed only via static method
win32_getHandle and direct access to the field is replaced by access to
that method.
Handles will no longer be created during initialization but upon first
access to the handle via its accessor. Also the destroy condition is
changed since handle being 0 doesn't mean that it was destroyed anymore.
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.

Create Font handles on demand
4 participants