Handle HiDPI scaling in GEF diagrams

[akoch-yatta]

I updated my approach for improving HiDPI scaling in GEF diagrams.

Central orchestrator is currently the MonitorAwareZoomManager which connects the Control (usually a FigureCanvas) and ScalableFigures to apply the monitor zoom as scale on. Linking is done via the AbstractEditPartViewer ( MonitorAwareZoomManager and FigureCanvas) FreeformGraphicalRootEditPart/ScalableRootEditPart (register ScalableFigures). Adaptions were made in FreeformGraphicalRootEditPart and ScalableRootEditPart to replace the innerLayer with a scalable layer that will registered into MonitorAwareZoomManager. For the flyout palette I added SimpleAutoscaledRootEditPart as a simple addition to do the same as the other two. The pattern is now everywhere one central scalable layer somewhere up in the hierarchy.

It would be nice to move that into the root figure, but was not able to fully make it work. The problem described in #768 (comment) by @ptziegler is to my analysis a "wrong" clipping caused by the zoom break in the root figure. Lets imaging having the flag active, so the FigureCanvas is at 100% and we have a monitor of 200%, so the root figure scale would be 2.0:
First, the client area of canvas is fetched, e.g. 1000x1000. Then the bounds of root figure are set with the value:

• if the bounds are scaled down, the bounds would be 500x500, which will be used as clipping area and will be too small for later calls
• if the bounds are not scaled down, so they remain 1000x1000, the clipping will be fine, but the children will be positioned at the wrong place

I was not able to properly solve this discrepancy, but I think that would enable us to move all the logic into a specific root figure.

One additional problem (for which I added some workarounds in the PR) are TextUtilities and FigureUtilities that use a static GC for its calculations. This means the GC will always the bound to the zoom that new Shell() is bounds to. At least in windows that mustn't even be the primary monitor, but could be a secondary or any additional monitor as well, which leads to cut off texts, when you wouldn't expect it. What we would need is the possibility to initialize the GC with a given control (e.g. either the parent Shell or the FigureCanvas) to provide the proper context for all calculations on this GC. As they are used at a lot of different places I didn't have a good idea, how to change these. This is a prerequisite for any proper solution for the underlying problem.

[ptziegler]

Thanks for the continuous update! I'll try to take a closer look during the weekend.

[ptziegler]

One additional problem (for which I added some workarounds in the PR) are TextUtilities and FigureUtilities that use a static GC for its calculations. This means the GC will always the bound to the zoom that new Shell() is bounds to. At least in windows that mustn't even be the primary monitor, but could be a secondary or any additional monitor as well, which leads to cut off texts, when you wouldn't expect it.

I think you underestimate just how broken the FigureUtilities class is. It's also used together with the Printer classes, where the concept of a monitor doesn't exist in the first place. I think this is also what caused issues in e.g. #750.

Overall, I think the best approach is to slowly move away from those static methods and instead use a GC that is based on some local Drawable object.

I've added some suggestions of things where I think that they could be made a little bit cleaner. Overall, I think this is the most promising approach right now. I also played around with the root figure some more, but I never got it working the way I want.

Once you think that this is in a good state, I'm willing to give it a go. The release window just started, so we have more than enough time to test this in more detail and also handle any problems that might've been missed.

One thing I'd still like to see would be a decoupling of this zoom handling from GEF so that it can be re-purposed for Zest. But I hope that this is as simple as moving the MonitorAwareZoomManager to Draw2D.

[ptziegler]

I tried to visualize my understanding of the current approach:

Instead of scaling the RootFigure, you went with scaling the RootEditPart. The zoom manager is bound to the EditPartViewer and is responsible for processing the device zoom of the hooked control. In order to receive those zoom changes, the edit parts need to register themselves to this manager.

[akoch-yatta]

@ptziegler I refactored and cleaned the commits up a bit. I hope you can still follow my latest changes.

[akoch-yatta]

@ptziegler As I think we are close to a mergeable state, I undrafted this PR.
We could take out the PR commit for now, if we want to improve on this topic.

[ptziegler]

Apologies for the delayed response. I agree that this is quickly moving towards a great state. I've found some minor nitpicks again, which I think can be cleaned up a little, but nothing severe.

I sadly don't seem to be able to rebase this PR. Can you perhaps do that to (hopefully) fix the failing GitHub checks?

@azoitl What is your take on this? I think it would be great if this can be included in the M1, so that we have enough time to properly test everything.

[azoitl]

@akoch-yatta @ptziegler thx for the the great work. I only found yesterday time to do a first look on this. I agree with both of you that we soon should get that merged for testing.

I would try to test it over the weekend with 4diac IDE I hope that there I can see some special cases. For that I would have one question: In order that 4diac IDE is correctly drawing I need to know the height of my drawing font. So far I'm using the FigureUtilities for this. Can I still use this? Do I need my own GC with hardcoded 100% scaling now?

[akoch-yatta]

@akoch-yatta @ptziegler thx for the the great work. I only found yesterday time to do a first look on this. I agree with both of you that we soon should get that merged for testing.

I would try to test it over the weekend with 4diac IDE I hope that there I can see some special cases. For that I would have one question: In order that 4diac IDE is correctly drawing I need to know the height of my drawing font. So far I'm using the FigureUtilities for this. Can I still use this? Do I need my own GC with hardcoded 100% scaling now?

The GC in FigureUtilities will be configured with 100% if you activate the flag, so it should work.

[akoch-yatta]

This change is a bit interesting. Whatever you do here, will break some scenarios, depending whether the GC that is used to render the Figure is on advanced mode or not. The "safest" approach would probably be to always and everywhere use a GC in advanced mode - at least when the flag is set to true. Is there anything speaking against that?

[ptziegler]

I've got some mixed feelings about using advanced mode everywhere. Here is fine, of course, given that the GC should only be used internally. But if there is ever a need to create a GC somewhere else, it's incredibly easy to forget about it and you suddenly end up with inconsistent and hard to debug problems.

[akoch-yatta]

But it is out of your control anyway, isn't it? I cannot check in the code currently, but If you have a SWTGraphics with scaling, you'll get advanced Mode. If there is no scaling, you won't.

[ptziegler]

I just want to avoid situations where one now has to always do:

SWTGraphics graphics = new SWTGraphics(gc);
graphics.setAdvanced(true);

Or do you always (assuming the flag is set) in the SWTGraphics constructor? The problem is that I can't really predict what side-effects this might cause, so I'm uncomfortable with that idea.

On a different note: I assume this issue exists outside of Draw2D as well? Wouldn't it make more sense to enable this in SWT, rather than here? From what I remember, the GC instance is initialized with the Drawable it is created for.

There you could check whether auto-scaling is disabled for that widget and then activate advanced mode if necessary.

[akoch-yatta]

I agree, that this shouldn't be on the side of consumers of GEF. Just wanted to point out, that it isn't (per se) tied to having auto-scaling disabled, but to when one of the methods of a GC is used, so that the advanced mode is enabled -> e.g drawPath or setTransform or any of the other methods. And that makes it actually very complicated. I tried to come up with some simple snippet and got to something like:

GC gc = new GC(canvas);
System.out.println(gc.textExtent("Representing a half adder"));
gc.drawPath(new Path(shell.getDisplay()));
System.out.println(gc.textExtent("Representing a half adder"));

and the output is

Point {202, 25}
Point {203, 24}

So, e.g. one Figure draws a Path, the next one calculates and renders text differently.
I don't know, if this effect is similar problematic in GTK and cocoa or only in the windows GC, but this is really something to have a look at. Your idea with initializing it like that in SWT could be a way to go, we will need to think about it. But it is more messed up than I thought until now.

[akoch-yatta]

Next round of refactorings. Be aware that I rebased on #801, so the first commit is part of the other PR

[ptziegler]

@akoch-yatta How should we proceed with this one? I think the issue with the +1 for the width has been fixed by using the advanced mode in the FigureUtilities.

When it comes to how this should be handled in general, I would suggest a separate PR to keep each change isolated. Otherwise I fear that those discussion will drag on for a lot longer...

Can you also rebase the PR again to get rid of 89775da? What about the other commits. Do you want to keep them separate or should they be squashed.

Unless I've missed anything, this should then be ready to be comitted, right?

[akoch-yatta]

@ptziegler I rebased the PR. I would propose to keep the three commits, because they tackle three different issues, but I can squash them if you want.

Overall I am in favor of merging this state to continuously improve it in the next weeks and get feedback in case we broke something - with and without setting the flag

[ptziegler]

Sounds like a plan 👍 I'll want to give it a final test, then I'll try to get things merged within this week.

[akoch-yatta]

Perfect. I am on vacation from next Monday until Friday. So would be great to get it done this week, so it is in M1

[azoitl]

I'll also try to test it today. It would be amazing to have it in M1.

[azoitl]

I tested during todays development with 4diac IDE under linux with wayland (100% & 125%). Drawing wise all looks good. Used pallets, at least 6 different GEF Classic editors. 4diac IDE has a special figure pane which I was a bit nervouse. But everything worked out of the box.

I had some issues with selection and hovering on 125%. But I think this is coming from SWT's wayland/gtk4 interaction. Because I also had it sometimes on editor tabs and menus.

Therefore I would go for merging it.

@akoch-yatta thx a lot for this work.

@Phillipus this may be interesting for you as well.

[HeikoKlare]

Thank you all for developing, testing and improving this contribution! 🥇 It's really great to see this going into GEF master now.

Just as an additional information regarding stability and further development of this enhancement: we have already integrated the preliminary concept proposed in #768 into our product and currently update to the solution proposed in this PR. And we are currently in an intensive testing phase for our product, which will give a bunch of additional validation of how well this enhacement works (on Windows). If we experience any issues that require further improvements to this solution in GEF or if we find noteworthy things to consider as a consumer of the feature, we will of course contribute that back to GEF.

[akoch-yatta]

@ptziegler I did a small update to the PR. We noticed, that using the monitor zoom caused issues without monitor specific scaling active, therefor we switched back to using a data value from control. We will need the latest state from SWT so it contains commit eclipse-platform/eclipse.platform.swt@648bb97 for it to work.

[ptziegler]

I gave the latest state a try (outside of the new DATA_SHELL_ZOOM property) and things are looking really good. Even on Zest, the adaptations can be easily integrated.

@akoch-yatta Great job! Thanks for sticking to this to the end, even with all the constant back and forth.