Make ScaledGraphics optional

[Phillipus]

Opening this issue as a placeholder for discussion about the ScaledGraphics class and making it optional, why it has problems, and what we can do about it.

More to follow...

[Phillipus]

The ScaledGraphics class is a wrapper around another Graphics instance and seems to add some additional features like Font scaling. But some fonts are clipped at different zoom levels.

We encountered this in our RCP app:

archimatetool/archi#621

And other discussions:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=470334
https://bugs.eclipse.org/bugs/show_bug.cgi?id=442442

I don't know whether the underlying cause of the font problem could be fixed or whether ScaledGraphics should be removed or made optional. We made it optional:

archimatetool/archi@98b038e
archimatetool/archi@2f36456

[Phillipus]

And another RCP tool, Modelio, made the same decision

• Same as {@link ScalableFreeformLayeredPane} but don't use a ScaledGraphics
• to paint the area, this class sucks at zooming texts.

https://github.com/ModelioOpenSource/Modelio/blob/9348609fab75011467b9f04f6965390e2471e5aa/modelio/app/app.diagram.elements/src/org/modelio/diagram/elements/core/figures/freeform/ScalableFreeformLayeredPane2.java

[Phillipus]

One downside of not using ScaledGraphics:

archimatetool/archi#851

[azoitl]

In the 4diac IDE we also replaced the scaled graphics with the graphics [1]. Since then I have the impression the graphics looks nicer under Linux however we also noticed some problems:

• XOR drawing mode seems not to work under Windows. XOR drawing is used by GEF for selection higlight, marquee select border and D&D feedback
• On some zoom-factors the clipping of connections was to small and some connection segments (mostly vertical ones) where not drawn. We could compensate that with increasing the bounding margins. Also it seemed that mostly windows was affected.

As for both windows was mostly affected it could also be an SWT issue.

Although for us it is clear that we stay with the non scaled version. But as there is a downside I think the optional proposal by @Phillipus sounds like a good idea.

[1] https://git.eclipse.org/r/c/4diac/org.eclipse.4diac.ide/+/179573/2/plugins/org.eclipse.fordiac.ide.gef/src/org/eclipse/fordiac/ide/gef/editparts/ZoomScalableFreeformRootEditPart.java

[github-actions]

This issue is stale because it has been open for 90 days with no activity.

[Phillipus]

I'll be submitting a draft PR for discussion soon.

[Phillipus]

@azoitl I created PR #132 for discussion. This is what I use in Archi. There may be a better way to do it. At the very least it is all optional and doesn't affect existing usage.

[azoitl]

@Phillipus thanks for the pullrequest. I think it is a very good starting point and much better what we did in Eclipse 4diac. However I still think there are some issues, which I'm not sure how to solve:

• The changes in PrinterGraphics changes the API or?
• There is quite some duplicated code in ScalableLayeredPane and ScalableFreeformLayeredPane. Could we move that to default methohds of ScalableFigure?
• There are no setters for the 'useScaledGraphics'. In my work for the ZoomScrollPolicies I originally also had no setters but adding them made using the ZoomScrollPolicies easier for clients. They could just get the ZoomManager and set the policy. I think we should also consider this here. Otherwise clients need to override all the creation functions.

BTW is it good to discuss the this here or better at the PR?

[Phillipus]

@azoitl Thanks for looking at the PR.

The changes in PrinterGraphics changes the API or?

This is in class PrintOperation.

Because the Graphics intance can now be either SWTGraphics or PrinterGraphics and as both are sub-classes of Graphics therefore the return type is Graphics in getFreshPrinterGraphics and setupGraphicsForPage. These are protected methods so I'm not sure if this will affect clients who might sub-class PrintOperation or not. Open to suggestions on this one.

There is quite some duplicated code in ScalableLayeredPane and ScalableFreeformLayeredPane. Could we move that to default methods of ScalableFigure?

That won't work. The code in paintClientArea in both classes is already duplicated because they both need to over-ride the method in Figure. Unless there is a better place to move this?

There are no setters for the 'useScaledGraphics'...

Good point. Should there be getters, too?

BTW is it good to discuss the this here or better at the PR?

I think here is best, as it is permanent whereas a PR might change or be abandoned.

[azoitl]

@azoitl Thanks for looking at the PR.

The changes in PrinterGraphics changes the API or?

This is in class PrintOperation.

Because the Graphics intance can now be either SWTGraphics or PrinterGraphics and as both are sub-classes of Graphics therefore the return type is Graphics in getFreshPrinterGraphics and setupGraphicsForPage. These are protected methods so I'm not sure if this will affect clients who might sub-class PrintOperation or not. Open to suggestions on this one.

I got that and understood why. Given my experience with other changes I just wanted to raise a concern. I also have no idea for a better solution.

There is quite some duplicated code in ScalableLayeredPane and ScalableFreeformLayeredPane. Could we move that to default methods of ScalableFigure?

That won't work. The code in paintClientArea in both classes is already duplicated because they both need to over-ride the method in Figure. Unless there is a better place to move this?

I played around a bit today in the morning. Nothing that made my happy but I see your changes a chance to move at least some duplicated things to ScalableFigure.

There are no setters for the 'useScaledGraphics'...

Good point. Should there be getters, too?

I would say yes, because this would enable more stuff to be moved to ScalableFigure. Especially when we define that getters and setters there. Also for the scale factor.

BTW is it good to discuss the this here or better at the PR?

I think here is best, as it is permanent whereas a PR might change or be abandoned.

Good point.

[azoitl]

@Phillipus today I finally found some time to play with your PR. See my PR #135 for some results. By introducing a common interface for both ScaleableLayeredPane and ScaleableFreeformLayeredPane I could move some common code to this interface. Not sure if it is the best approach but wanted to get your opinion on it.

I also tried it out with the LogicEditor example. My first test case for new things. There I noticed that my comment with the setters is most probably not needed. Would even make the useScaledGraphics final now.

As the API tools are not very happy about the PrinterGraphics changes I think we should split up the changes in separate parts.

[Phillipus]

Hi @azoitl - thanks so much for taking this further.

Not sure if it is the best approach but wanted to get your opinion on it.

Well, it certainly is a better approach! I think we should continue this way.

As for PrinterGraphics we might need to keep the existing API and take a different approach.

[azoitl]

Hi @Phillipus today one of our users ran into a problem that seems for me related to this one. We noticed that our diagrams are not scaling correctly on Windows when a display zoom is set. In our case the user had a display zoom of 125%. All Eclipse widgets correctly get scaled. However the GEF drawings are still at the same size. However the fonts in the GEF drawings are scaled with the display scale factor. We are not using the scaled graphics as proposed by you. But we use sofar a very ugly workaround. Have you had some similar issues. Is this related to the scaled graphics problem or something else?

[Phillipus]

Hi @azoitl

I'm not sure if it's the same issue but I discovered many years ago that a font for a Draw2d IFigure can be too big on Windows if native scaling is say 125% or 150%.

So I created a method to convert a given font to a scaled font that can be applied to an IFigure:

private static FontRegistry windowsFontRegistry = new FontRegistry();

public static Font getAdjustedWindowsFont(Font font) {
    if(font != null && PlatformUtils.isWindows()) {
        int DPI = font.getDevice().getDPI().y;
        if(DPI != 96) {
            FontData[] fd = font.getFontData();
            String fontName = fd[0].toString();
            if(!windowsFontRegistry.hasValueFor(fontName)) {
                float factor = (float)96 / DPI;
                int newHeight = (int)(fd[0].getHeight() * factor);
                fd[0].setHeight(newHeight);
                windowsFontRegistry.put(fontName, fd);
            }
            font = windowsFontRegistry.get(fontName);
        }
    }

    return font;
}

[azoitl]

Hi @Phillipus,
I think in my case it is the opposite. As I tried to show with the screenshot below. The fonts are correctly scaled. But the drawing elements are in the same size independent of the native scaling:

Top is 100% bottom 125% native scaling.

[azoitl]

@Phillipus after we have the main classes configurable I started to look at the Thumbnail. Here I noticed two three things:

1. Do you see it important that we make it here configurable?
2. In Performance tests of 4diac IDE I noticed that Thumbnail is generating a lot of garbage. This can be a severe performance issue when you are already memory constraint. Therefore I was thinking to ThumbnailUpdater let reuse more of its data. However then I noticed that there is this fix for MacOS to even more freshly using elements. I guess my idea would be against it. I thought by moving the ThumbnailUpdater into a separate file and let a MacOS version inherit and adjust from it, we could handle that better. Am I missing something obvious?
3. Another performance problem of the Thumbnail class is triggering a repaint without stopping a running ThumbnailUpdater first. Especially when scrolling and zooming this can have a performance impact.

While the last thing is IMHO a quick win. I'm less sure about the first 2.

[Phillipus]

@azoitl

1. I made my version of Thumbnail configurable to allow/ not allow scaled graphics, but the quality of the thumbnail is not very high anyway so I'm not sure if it matters.
2. Yes that might be a better approach but makes things complicated.

TBH I wonder if the Thumbnail needs to be re-written? I'm not sure how to do that, though.

[azoitl]

@Phillipus not sure if I could come up with a better version on rewrite. I did some experiments which made it worse. So at least I know how to do that.
Therefore, I think I'll start with some clean-up tasks first so that the code gets in a better shape. Then I would remove the ScaledGraphics and fix the performance issue I mentioned in 3. above.

With that we may learn enough how to proceed with it.

[azoitl]

With the fix for the thumbnail in PR #198 we are nearly there. The only open point of I correctly remember is to fix the printing. Not sure yet how to do this. Any suggestions are warmly welcome.

[Phillipus]

The only open point of I correctly remember is to fix the printing.

PR can be used as a starting point, please feel free to make any changes...

#237

[azoitl]

The only open point of I correctly remember is to fix the printing.

PR can be used as a starting point, please feel free to make any changes...

#237

Thx, will have a look in the next days.

[github-actions]

This issue is stale because it has been open for 90 days with no activity.

[github-actions]

This issue is stale because it has been open for 90 days with no activity.

[github-actions]

This issue is stale because it has been open for 90 days with no activity.