RFC: DefaultWrite mode, CallTip, Completion popup fix

[Pecan]

I'd like comments as to whether the following patch is foolish or usable.

It addresses the constant complaints about the popups font for defaultWrite mode being too big.

It de-scales the font  and width of the defaultWrite mode wxPopupWindow (an idea taken from a cryptic statement by New Pegodi and code by Miguel Gimenez) for defaultWrite CallTip and AutoCompletion.

Suggestions for improvement (if it's usable) are welcome.
Suggestions why to trash it are also welcome.
 

Code

Index: src/sdk/wxscintilla/src/PlatWX.cpp
===================================================================
--- src/sdk/wxscintilla/src/PlatWX.cpp	(revision 13319)
+++ src/sdk/wxscintilla/src/PlatWX.cpp	(working copy)
@@ -2429,19 +2429,22 @@
 /* C::B begin */
     // GETLB(wid)->SetFont(*((wxFont*)font.GetID()));
     wxFont *NewFont = (wxFont*)font.GetID();
+#if wxCHECK_VERSION(3, 1, 4)
+         const double scale = GETLB(wid)->GetDPIScaleFactor();
+ #else
+         const double scale = GETLB(wid)->GetContentScaleFactor();
+ #endif
+
     if (technology == wxSCI_TECHNOLOGY_DIRECTWRITE)
-    {
-#if wxCHECK_VERSION(3, 1, 4)
-        const double scale = GETLB(wid)->GetDPIScaleFactor();
-#else
-        const double scale = GETLB(wid)->GetContentScaleFactor();
-#endif
         GETLB(wid)->SetFont(NewFont->Scaled(72.0/(96.0*scale)));
-    }
-    else
+    else // non direct write mode (wxSCI_TECHNOLOGY_DEFAULT)
+        #if wxCHECK_VERSION(3, 1, 4)
+        GETLB(wid)->SetFont(NewFont->Scaled(72.0/(72.0*scale))); //(ph 2023/07/18)
+        #else
         GETLB(wid)->SetFont(*NewFont);
+        #endif
+}
 /* C::B end */
-}
 
 
 void ListBoxImpl::Create(Window &parent, int ctrlID, Point location_, int lineHeight_, bool unicodeMode_, int technology_) {
Index: src/sdk/wxscintilla/src/scintilla/src/CallTip.cxx
===================================================================
--- src/sdk/wxscintilla/src/scintilla/src/CallTip.cxx	(revision 13319)
+++ src/sdk/wxscintilla/src/scintilla/src/CallTip.cxx	(working copy)
@@ -26,6 +26,13 @@
 using namespace Scintilla;
 #endif
 
+/* C::B begin */ //(ph 2023/07/23)
+#include <wx/version.h>
+#if wxCHECK_VERSION(3, 1, 4)
+#include "wx/window.h" //(ph 2023/07/22)
+#endif
+/* C::B end */
+
 CallTip::CallTip() {
 	wCallTip = 0;
 	inCallTipMode = false;
@@ -283,6 +290,20 @@
 		look = newline + 1;
 		numLines++;
 	}
+
+/* C::B begin */
+    #if wxCHECK_VERSION(3, 1, 4)
+    // Scale the popup window size for non DirectWrite (defaultWrite) mode
+	if (technology == SC_TECHNOLOGY_DEFAULT) //(ph 2023/07/22)
+    {
+      	std::unique_ptr<wxWindow> pwxWin;
+      	pwxWin.reset(new wxWindow());
+      	double sysScaling = pwxWin->GetDPIScaleFactor();
+      	int* pWidth = const_cast<int*>(&width);
+      	*pWidth = ((width*72*sysScaling)/72) + fp.size;
+    }
+    #endif
+/* C::B end */
 	lineHeight = RoundXYPosition(surfaceMeasure->Height(font));
 
 	// The returned
Index: src/sdk/wxscintilla/src/scintilla/src/ScintillaBase.cxx
===================================================================
--- src/sdk/wxscintilla/src/scintilla/src/ScintillaBase.cxx	(revision 13319)
+++ src/sdk/wxscintilla/src/scintilla/src/ScintillaBase.cxx	(working copy)
@@ -60,6 +60,13 @@
 #include "AutoComplete.h"
 #include "ScintillaBase.h"
 
+/* C::B begin */
+#include <wx/version.h>
+#if wxCHECK_VERSION(3, 1, 4)
+#include <wx/window.h> //(ph 2023/07/22)
+#endif
+/* C::B end */
+
 #ifdef SCI_NAMESPACE
 using namespace Scintilla;
 #endif
@@ -428,7 +435,6 @@
 		*buffer = '\0';
 	return 0;
 }
-
 void ScintillaBase::CallTipShow(Point pt, const char *defn) {
 	ac.Cancel();
 	// If container knows about STYLE_CALLTIP then use it in place of the
@@ -443,6 +449,26 @@
 		pt.x += ptOrigin.x;
 		pt.y += ptOrigin.y;
 	}
+
+    /* C::B begin */
+	#if wxCHECK_VERSION(3, 1, 4)
+	// Correct non-DirectWrite sizeZoomed for defaultWrite mode  //(ph 2023/07/19)
+	if (vs.technology == SC_TECHNOLOGY_DEFAULT)
+    {
+        std::unique_ptr<wxWindow> pwxWin;
+        pwxWin.reset(new wxWindow());
+        double sysScaleFactor = pwxWin->GetDPIScaleFactor();
+        int fontsize = vs.styles[ctStyle].size;
+        double dsizeZoomed = fontsize + vs.zoomLevel * SC_FONT_SIZE_MULTIPLIER;
+        double dscale = (72.0 /(72.0 * sysScaleFactor)) +0.01;
+        dsizeZoomed = dsizeZoomed * dscale;
+        if (dsizeZoomed <= 2 * SC_FONT_SIZE_MULTIPLIER)	// Hangs if sizeZoomed <= 1
+            dsizeZoomed = 2 * SC_FONT_SIZE_MULTIPLIER;
+        vs.styles[ctStyle].sizeZoomed = dsizeZoomed;
+    }
+    #endif // wxCHECK_VERSION
+	/* C::B end */
+
 	PRectangle rc = ct.CallTipStart(sel.MainCaret(), pt,
 		vs.lineHeight,
 		defn,

The patch applies to Head r13319 .
The debugging tags like "//(ph 2023/07/23)" will be removed later; if we use it.

[ollydbg]

I like your idea. I will try to build C::B with your patch and test it.

Some of my friend use C::B for development, and they use a High resolution screen. They have met the big font issue in CallTip or Code Completion list. All I can tell them to fix the issue is to switch to the DirectWrite mode.


EDIT:


It looks like git apply command does not work, so I got this:

Code

git apply ./DefaultWritePatch_230723-2.patch
./DefaultWritePatch_230723-2.patch:65: space before tab in indent.
        std::unique_ptr<wxWindow> pwxWin;
./DefaultWritePatch_230723-2.patch:66: space before tab in indent.
        pwxWin.reset(new wxWindow());
./DefaultWritePatch_230723-2.patch:67: space before tab in indent.
        double sysScaling = pwxWin->GetDPIScaleFactor();
./DefaultWritePatch_230723-2.patch:68: space before tab in indent.
        int* pWidth = const_cast<int*>(&width);
./DefaultWritePatch_230723-2.patch:69: space before tab in indent.
        *pWidth = ((width*72*sysScaling)/72) + fp.size;
error: sdk/wxscintilla/src/PlatWX.cpp: No such file or directory
error: sdk/wxscintilla/src/scintilla/src/CallTip.cxx: No such file or directory
error: sdk/wxscintilla/src/scintilla/src/ScintillaBase.cxx: No such file or dire
ctory

but using the patch command works:

Code

 patch -p0 < ./DefaultWritePatch_230723-2.patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/sdk/wxscintilla/src/PlatWX.cpp
Hunk #1 succeeded at 2452 (offset 23 lines).
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/sdk/wxscintilla/src/scintilla/src/CallTip.cxx
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/sdk/wxscintilla/src/scintilla/src/ScintillaBase.cxx

[ollydbg]

I'm testing on setting the DPI to 150% or 100% on my Win10, it looks fine, I mean the font size in the editor and the tip window are the same.

One issue is that the bottom edge of the tip window is a little small?

See the image shot below in the attachment.

[Miguel Gimenez]

These lines

Code

+        GETLB(wid)->SetFont(NewFont->Scaled(72.0/(72.0*scale))); //(ph 2023/07/18)
...
+      	*pWidth = ((width*72*sysScaling)/72) + fp.size;
...
+        double dscale = (72.0 /(72.0 * sysScaleFactor)) +0.01;

may be simplified, as 72.0 is in the numerator and the denominator. Why the 0.01?

[sodev]

Code

+/* C::B begin */
+    #if wxCHECK_VERSION(3, 1, 4)
+    // Scale the popup window size for non DirectWrite (defaultWrite) mode
+	if (technology == SC_TECHNOLOGY_DEFAULT) //(ph 2023/07/22)
+    {
+      	std::unique_ptr<wxWindow> pwxWin;
+      	pwxWin.reset(new wxWindow());
+      	double sysScaling = pwxWin->GetDPIScaleFactor();
+      	int* pWidth = const_cast<int*>(&width);
+      	*pWidth = ((width*72*sysScaling)/72) + fp.size;
+    }
+    #endif
+/* C::B end */

The creation of this temporary windows looks at least weird. And in a multi-monitor setup (if CB does actually use per-monitor DPI, i don't know if it does) this will be wrong, because the returned DPI value depends on the monitor on which this window gets positioned.

[Pecan]

These lines

Code

+        GETLB(wid)->SetFont(NewFont->Scaled(72.0/(72.0*scale))); //(ph 2023/07/18)
...
+      	*pWidth = ((width*72*sysScaling)/72) + fp.size;
...
+        double dscale = (72.0 /(72.0 * sysScaleFactor)) +0.01;

may be simplified, as 72.0 is in the numerator and the denominator. Why the 0.01?

I see how to simplify the "*pwidth" as  (width*sysScaling) + fp.size;
but I do not see how to simplify   (72.0 /(72.0 * sysScaleFactor))
With a scalling factor of 1.25:
72.0 * sysScalFactor is 90
72.0 / sysScalFactor is 57.6
72.0/(72.0*sysScalFactor) = .8 (the correct scaling factor)

Would you make a suggestion please.

[Pecan]

I'm testing on setting the DPI to 150% or 100% on my Win10, it looks fine, I mean the font size in the editor and the tip window are the same.

One issue is that the bottom edge of the tip window is a little small?

See the image shot below in the attachment.

Thanks for testing. What font are you using that caused the bottom edge problem?
It looks like the surface parameters will have to be adjusted to solve this.
I'd rather address that in a separate future patch.

[Pecan]

Code

+/* C::B begin */
+    #if wxCHECK_VERSION(3, 1, 4)
+    // Scale the popup window size for non DirectWrite (defaultWrite) mode
+	if (technology == SC_TECHNOLOGY_DEFAULT) //(ph 2023/07/22)
+    {
+      	std::unique_ptr<wxWindow> pwxWin;
+      	pwxWin.reset(new wxWindow());
+      	double sysScaling = pwxWin->GetDPIScaleFactor();
+      	int* pWidth = const_cast<int*>(&width);
+      	*pWidth = ((width*72*sysScaling)/72) + fp.size;
+    }
+    #endif
+/* C::B end */

The creation of this temporary windows looks at least weird. And in a multi-monitor setup (if CB does actually use per-monitor DPI, i don't know if it does) this will be wrong, because the returned DPI value depends on the monitor on which this window gets positioned.

Thanks, I'll change the code to use the current active window.

[Pecan]

These lines

Code

+        GETLB(wid)->SetFont(NewFont->Scaled(72.0/(72.0*scale))); //(ph 2023/07/18)
...
+      	*pWidth = ((width*72*sysScaling)/72) + fp.size;
...
+        double dscale = (72.0 /(72.0 * sysScaleFactor)) +0.01;

may be simplified, as 72.0 is in the numerator and the denominator. Why the 0.01?

With some fonts, the 0.01 is (experimentally) just enough to keep the right margin from melding with the last character of text.

[ollydbg]

I'm testing on setting the DPI to 150% or 100% on my Win10, it looks fine, I mean the font size in the editor and the tip window are the same.

One issue is that the bottom edge of the tip window is a little small?

See the image shot below in the attachment.

Thanks for testing. What font are you using that caused the bottom edge problem?
It looks like the surface parameters will have to be adjusted to solve this.
I'd rather address that in a separate future patch.

I'm using a font named: [be5invis/Sarasa-Gothic: Sarasa Gothic](https://github.com/be5invis/Sarasa-Gothic), but I think every mono font has such issue. Because I see the similar issue in my friend's PC, and he is using my personal built C::B(with your patch), and he is using the default font.

Thanks.

[Commaster]

These lines

Code

+        GETLB(wid)->SetFont(NewFont->Scaled(72.0/(72.0*scale))); //(ph 2023/07/18)
...
+      	*pWidth = ((width*72*sysScaling)/72) + fp.size;
...
+        double dscale = (72.0 /(72.0 * sysScaleFactor)) +0.01;

may be simplified, as 72.0 is in the numerator and the denominator. Why the 0.01?

I see how to simplify the "*pwidth" as  (width*sysScaling) + fp.size;
but I do not see how to simplify   (72.0 /(72.0 * sysScaleFactor))
With a scalling factor of 1.25:
72.0 * sysScalFactor is 90
72.0 / sysScalFactor is 57.6
72.0/(72.0*sysScalFactor) = .8 (the correct scaling factor)

Would you make a suggestion please.

it simplifies to 1.0/sysScalFactor, which is also .8 for scalling factor of 1.25

[Pecan]

Attached is an updated version of the defaultWrite patch:
DefaultWritePatch_230729-2.patch

Changes:
1) Get the current windows scaling factor from the foreground window.
2) Lift the bottom text line off the bottom border.
3) Push the right border off the right most text character.
4) Some cleanup (as suggested) and more comments.

Question - should the patch wait until after the new CB is released or applied to the current HEAD?

@Ollydbg: would you run your test against this patch to see if it lifts the bottom line off the bottom border for the fonts you're using?.

I've tested (on Head) against Windows 10/11 and Linux. Am waiting for results from a Windows 7 test.

Thanks to all for reviews and testing.

Note: Comments starting with "//(ph" will be removed from the final patch.

[Miguel Gimenez]

IMHO the patch should be applied before the release, or default technology should be Directwrite, or both.

[ollydbg]

IMHO the patch should be applied before the release, or default technology should be Directwrite, or both.

The Direct Write(using the Direct2D) has other issues. It will crash if user unlocked the screen or recovered from hibernation.

[ollydbg]

Attached is an updated version of the defaultWrite patch:
DefaultWritePatch_230729-2.patch

Thanks for your great work!

Question - should the patch wait until after the new CB is released or applied to the current HEAD?

I think it should be put in the trunk before the new C::B release.

@Ollydbg: would you run your test against this patch to see if it lifts the bottom line off the bottom border for the fonts you're using?.

Yes, I can confirm it fixes the bottom border issue, thanks.

I've tested (on Head) against Windows 10/11 and Linux. Am waiting for results from a Windows 7 test.

I have a Win7 OS, and I just test the new built C::B, the font works fine, I don't see C::B behave differently between Win7 and Win10.