#1432 D2D support per-monitor rendering parameters

[Zufu Liu]

As previously discussed at [feature-requests:#1427], defaultRenderingParams and customClearTypeRenderingParams created inside LoadD2DOnce() are steal after Windows settings change or when the window moved to a different monitor. [bugs:#2305] may be related.

CreateMonitorRenderingParams() can be used to fix the problem, there is sample code at https://docs.microsoft.com/en-us/windows/win32/DirectWrite/how-to-add-support-for-multiple-monitors

I find three ways to implement this:
1. move both parameters local to SurfaceD2D::SetFontQuality() by storing HWND or HMONITOR in Init() method. This change makes the method 100x slow and increased total paint time due to CreateMonitorRenderingParams() (which is slower than current used CreateRenderingParams()). The slow might be the reason they were created once, this is also not efficient as both parameters are recreated each time the window gets paint.
2. add void *defaultRenderingParams and void *customRenderingParams into SurfaceMode and EditModel, then create them in Message::SetTechnology, WM_SETTINGCHANGE and WM_WINDOWPOSCHANGED.
3. Change cbWndExtra to 3*sizeof(LONG_PTR) for all three window classes and store the two rendering parameters at index sizeof(LONG_PTR) and 2*sizeof(LONG_PTR).

[Zufu Liu]

Changes by move both parameters local to SurfaceD2D::SetFontQuality(), can be used to measure paint performance impact.

[Zufu Liu]

Patch (see https://github.com/zufuliu/notepad2/commit/8043a8fdeaaa88cf4c5470aa4f835f43129138fa) for adding defaultRenderingParams and customRenderingParams into SurfaceMode. The change added CurrentSurfaceMode() and GetSurfaceMode(int codePage) into EditModel and overrides the later for D2D inside ScintillaWin (which holds two std::unique_ptr<IDWriteRenderingParams, UnknownReleaser>).

PS. pRenderTarget and sysCaretBitmap can be changed to std::unique_ptr just like the changes in GTK.

Edit: hPreviousMonitor should be renamed to hCurrentMonitor.

[Zufu Liu]

Updated patch: renamed hPreviousMonitor to hCurrentMonitor, in Message::SetTechnology changed the call to UpdateRenderingParams(true) as cleartype settings may be changed between changing technologies.

[Neil Hodgson]

Since this moved the define for SPI_GETFONTSMOOTHINGCONTRAST, updated [ca94b3] the Windows API version to 0x0A00 (Windows 10) and removed all the definitions used to cover up old SDKs. Windows 10 was released in 2015. This has the small downside of not warning when newer APIs are called but that will be checked by building for XP.

[Neil Hodgson]

Inlining CurrentSurfaceMode and GetSurfaceMode in headers doesn't appear justified as they aren't performance-sensitive, could become non-trivial, and GetSurfaceMode is virtual. Only defining ScintillaWin::GetSurfaceMode for USE_D2D mode is confusing with GDI having two distinct implementations so should probably always be declared with an #if in the implementation to perform the more complex case.

[Zufu Liu]

Updated patch with suggested changes,

[Neil Hodgson]

It appears that there could be pointer lifetime issues for the rendering params. Pixmap surfaces persist between paints and potentially past rendering param invalidation (window move or setting change) but they hold onto default/customRenderingParams. If this occurs, the pointers may point to freed memory leading to crashes.

Another issue here is that with this approach changes may be required to the platform-independent code for different platforms. An approach that allowed each platform to extend SurfaceMode independently would be more maintainable.

[Zufu Liu]

At the moment these Pixmap surfaces (in EditView and MarginView) does not draw any text, so they just hold dangle pointers. All can be fixed by call SetMode(model.CurrentMode()) before drawing.

Maybe Font like SetMode(std::shared_ptr<SurfaceMode> mode) would fix the lifetime issue.

[Neil Hodgson]

pixmapLine is used to draw text in buffered drawing mode. Adding calls to InvalidateStyleData() or InvalidateStyleRedraw() after any effectual UpdateRenderingParams (not currently included for WM_WINDOWPOSCHANGED) would clear the pixmap which would then be recreated with the new settings. This still leaves bare untyped pointers in SurfaceMode that could later be a lifetime hazard.

Changing the signature of Surface::SetMode breaks platform compatibility. Adding a shared_ptr<PlatformMode> field to SurfaceMode would be less trouble. Platform code could subclass an empty PlatformModeadding platform-specific fields.

Another possibility would be to move more of Surface creation into Editor where the ScintillaWin subclass would override the method(s) and push rendering params into any SurfaceD2D.

CallTip::CallTipStart which lays out text doesn't get the full surface mode as it doesn't have access to EditModel::CurrentSurfaceMode(). May not matter for measurement but this could be problem if other additions made to SurfaceMode. Should have the measuring surface passed in from ScintillaBase::CallTipShow.

[Zufu Liu]

Updated patch:
1. add surface->SetMode(CurrentSurfaceMode()); in Editor::PaintSelMargin() as the surface may be used to draw text inside MarginView::PaintOneMargin().
2. addDropGraphics() after UpdateRenderingParams() inside WM_SETTINGCHANGE and WM_WINDOWPOSCHANGED.
3. change CallTipStart into following, no font creation (just use already created font from StyleCallTip or StyleDefault)

PRectangle CallTipStart(Sci::Position pos, Point pt, int textHeight, const char *defn,
    int codePage_, Surface *surfaceMeasure, std::shared_ptr<Font> font_);

[Neil Hodgson]

Committed changes to CallTip surface and font [eab98f] but omitted std::move on font. SetForeBack [f29604]. Minor cosmetic changes in [bbcf99].

[Zufu Liu]

std::move was suggested by clang-tidy: warning: parameter 'font_' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param].

DropGraphics() in Message::SetTechnology, Message::SetBidirectional and case WM_SETTINGCHANGE can be omitted.

[Neil Hodgson]

Committed removal of redundant DropGraphics with [46652e].

Committed basic implementation of CurrentSurfaceMode without rendering parameters additions in [cd45e4].

[Zufu Liu]

codePage field in CallTip can be removed then.

I have tested that, font = std::move(font_) and font = font_ both cause use_count() for style.font to be increased by one after call CallTipStart().

[Neil Hodgson]

codePage field in CallTip can be removed then.

The codePage field is used in the Qt implementation and possibly out-of-repo platform layers.

There's likely a small performance and size code benefit to adding a std::move there but it's insignificant when a window is about to be displayed. Code that isn't performance-sensitive should be optimized for clarity and brevity.

[Neil Hodgson]

Here is an implementation (ScintillaWin::SetRenderingParams) that hides the rendering params inside the platform layer. AddRef is used so the pointers inside SurfaceD2D won't dangle.

[Zufu Liu]

Oh, not even thought about COM object are reference counted.
Some small changes that I think would improve the code:
1. in DropGraphics() inside WM_SETTINGCHANGE can be omitted.
2. technologyOpt.value_of(technology) seems more readable.
3. SurfaceD2D::AllocatePixMap() missed SetRenderingParams().

I think at the moment there is no need to override CreateMeasurementSurface() and CreateDrawingSurface() as rendering parameters are only useful for D2D drawing surface which only created from ScintillaWin::CTWndProc(), ScintillaWin::PaintDC() or SurfaceD2D::AllocatePixMap(). SetRenderingParams() inside ScintillaWin can be moved into respect else branches.

[Neil Hodgson]

ISetRenderingParams3.patch has those changes.

There should probably be a call to Redraw in WM_SETTINGCHANGE. Without that, sometimes the window is redrawn with the new settings and sometimes it isn't. The change may be delayed until later like when focus changes to Scintilla. Redraw is less likely when the clear type and settings windows don't overlap Scintilla.

RenderingParamsStdShared.patch changes the implementation to use a separately allocated RenderingParams struct which is shared between ScintillaWin and it's surfaces. That way, there is less likelihood of stale rendering params in pixmaps and explicit reference counting is avoided. Uncertain which approach is better.

[Zufu Liu]

RenderingParamsStdShared.patch seems simpler and more extensible.

There should probably be a call to Redraw in WM_SETTINGCHANGE. ....

I don't understand this: as pixmap graphics are dropped inside in InvalidateStyleData() next paint would allocate new surfaceWindow and pixmaps with new settings.

[Neil Hodgson]

I think the problem is that there isn't always another paint.

To reproduce the issue, move the app hosting Scintilla to one side so it will not overlap other windows. Open font settings and ensure it doesn't overlap the Scintilla window, click on Adjust ClearType text. The ClearType Text Tuner must also not overlap Scintilla. Choose distinctive settings - I flip between darkest and lightest text. Press Finish and focus/active should return to the Fonts settings panel and not go to Scintilla. Scintilla may not show with the new font settings until it receives focus when it does a full redraw.

[Zufu Liu]

(Run Notepad2 as console app) I see PaintDC is called twice (after received WM_SETTINGCHANGE) after finish ClearType tuner without focus in. Observed the same behavior after move Notepad2 and the console window into virtual destop2 in task view, and change settings in destop1.

It seems we missed a notification on check/uncheck "Enable ClearType", only app menu is repainted (but PaintDC is actually called), while text in in windows Notepad did get repainted.

[Neil Hodgson]

With SciTE, the only WM_SETTINGCHANGE is with wParam == 0x2013 which is SPI_SETFONTSMOOTHINGORIENTATION, lParam == "". Two of those are received and SciTE forwards them to both the edit and output pane so total of 4 calls. There aren't any notifications for SPI_SETFONTSMOOTHING, SPI_SETFONTSMOOTHINGCONTRAST, SPI_SETCLEARTYPE, or SPI_SETFONTSMOOTHINGTYPE even when ClearType is turned on/off.

Redraw isn't too expensive so it's OK to call it for every WM_SETTINGCHANGE although its possible there are badly behaved apps that set settings excessively.

[Neil Hodgson]

Committed std::shared_ptr technique as [b1aa62] with call to Redraw when setting change notification received.

[Zufu Liu]

for redraw, what about call InvalidateStyleRedraw() instead.

diff -r b1aa62c88dab win32/ScintillaWin.cxx
--- a/win32/ScintillaWin.cxx    Sat Mar 12 10:26:33 2022 +1100
+++ b/win32/ScintillaWin.cxx    Sat Mar 12 12:28:06 2022 +0800
@@ -2007,13 +2007,12 @@
 #if defined(USE_D2D)
            if (technology != Technology::Default) {
                UpdateRenderingParams(true);
-               Redraw();
            }
 #endif
            UpdateBaseElements();
-           InvalidateStyleData();
            // Get Intellimouse scroll line parameters
            GetIntelliMouseParameters();
+           InvalidateStyleRedraw();
            break;

        case WM_GETDLGCODE:
@@ -2026,7 +2025,7 @@
        case WM_SYSCOLORCHANGE:
            //Platform::DebugPrintf("Setting Changed\n");
            UpdateBaseElements();
-           InvalidateStyleData();
+           InvalidateStyleRedraw();
            break;

        case WM_DPICHANGED: