search

eclipse / gef-classic

Eclipse GEF(tm) Classic code
Eclipse Public License 2.0
24 stars 48 forks source link

Make ScaledGraphics optional #106

Closed Phillipus closed 2 weeks ago

Phillipus commented 2 years ago

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 commented 2 years ago

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:

https://github.com/archimatetool/archi/issues/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:

https://github.com/archimatetool/archi/commit/98b038e346275da27146c7eb95c801f3ef71b6c6 https://github.com/archimatetool/archi/commit/2f364562c93a2d7913cfece92d0a1862ddd20702

Phillipus commented 2 years ago

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 commented 2 years ago

One downside of not using ScaledGraphics:

https://github.com/archimatetool/archi/issues/851

azoitl commented 2 years ago

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:

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[bot] commented 1 year ago

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

Phillipus commented 1 year ago

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

Phillipus commented 1 year ago

@azoitl I created PR https://github.com/eclipse/gef-classic/pull/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 commented 1 year ago

@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:

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

Phillipus commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

scalingcomparision

Top is 100% bottom 125% native scaling.

azoitl commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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...

https://github.com/eclipse/gef-classic/pull/237

azoitl commented 1 year ago

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[bot] commented 9 months ago

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

github-actions[bot] commented 6 months ago

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

github-actions[bot] commented 3 months ago

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