zufuliu / notepad4

Notepad4 (Notepad2⨯2, Notepad2++) is a light-weight Scintilla based text editor for Windows with syntax highlighting, code folding, auto-completion and API list for many programming languages and documents, bundled with file browser plugin matepath.
Other
2.56k stars 179 forks source link

app position not saved when primary monitor is to right of main monitor #731

Closed JeffMill closed 9 months ago

JeffMill commented 10 months ago

I have my monitors set up as:

[1][2]

where [2] is the "main display".

Dragging notepad2 window to left ([1]) monitor and closing it doesn't persist the location. I'm guessing that's because (I believe) the position of the "[1]" monitor ends up giving a negative x position from the main display?

Should be easy to repro.

Thanks for this editor!

JeffMill commented 10 months ago

actually, I tried changing primary monitor to [1] and am seeing that notepad2 position isn't persisted when put onto the secondary monitor. Maybe a dual monitor issue? Let me know if I can provide logs.

zufuliu commented 10 months ago

Window position is saved in following function: https://github.com/zufuliu/notepad2/blob/ab1de27cec442341ce758a56b4a5853c736ea37c/src/Notepad2.c#L5996-L6005

and restored inside https://github.com/zufuliu/notepad2/blob/ab1de27cec442341ce758a56b4a5853c736ea37c/src/Notepad2.c#L714-L722

JeffMill commented 10 months ago

I opened notepad2, dragged it to the left monitor and picked "save settings now"

When I did that, the notepad2.ini file has:

[Window Position 2560x1440]
WindowPosX=480
WindowPosY=183
WindowSizeX=1287
WindowSizeY=971
FindReplaceDlgSizeX=598
[Window Position 3840x2160]
WindowPosX=-2083
WindowPosY=200
WindowSizeX=1287
WindowSizeY=971
FindReplaceDlgSizeX=598

I haven't gone through the code you have above, but how does notepad2 determine which monitor to default to? Note that my monitors have different resolutions.

JeffMill commented 10 months ago

it looks like it's using whatever windows chooses:

HMONITOR hMonitor = MonitorFromWindow(NULL, MONITOR_DEFAULTTONEAREST);

I believe you want to persist the last PosX,PosY, and then when restarting, read that value, and send it to MonitorFromPoint(). That's the hMonitor to use here.

JeffMill commented 10 months ago

I'll try to get a PR out, but here's the diff.

--- a/src/Notepad2.c
+++ b/src/Notepad2.c
@@ -5707,7 +5707,10 @@ void LoadSettings(void) {

        iValue = IniSectionGetInt(pIniSection, L"FullScreenMode", FullScreenMode_Default);
        iFullScreenMode = iValue;
        bInFullScreenMode = iValue & FullScreenMode_OnStartup;
+
+       const int windowPosX = IniSectionGetInt(pIniSection, L"WindowPosX", 0);
+       const int windowPosY = IniSectionGetInt(pIniSection, L"WindowPosY", 0);

        // toolbar image section
        {
@@ -5731,7 +5734,10 @@ void LoadSettings(void) {
        // window position section
        {
                WCHAR sectionName[96];
-               HMONITOR hMonitor = MonitorFromWindow(NULL, MONITOR_DEFAULTTONEAREST);
+               POINT pt;
+               pt.x = windowPosX;
+               pt.y = windowPosY;
+               HMONITOR hMonitor = MonitorFromPoint(pt, MONITOR_DEFAULTTONEAREST);
                GetWindowPositionSectionName(hMonitor, sectionName);
                LoadIniSection(sectionName, pIniSectionBuf, cchIniSection);
                IniSectionParse(pIniSection, pIniSectionBuf);
@@ -5983,6 +5989,14 @@ void SaveSettings(bool bSaveSettingsNow) {
        IniSectionSetBoolEx(pIniSection, L"AutoScaleToolbar", bAutoScaleToolbar, true);
        IniSectionSetBoolEx(pIniSection, L"ShowStatusbar", bShowStatusbar, true);
        IniSectionSetIntEx(pIniSection, L"FullScreenMode", iFullScreenMode, FullScreenMode_Default);
+
+       if (!IsIconic(hwndMain)) {
+               WINDOWPLACEMENT wndpl;
+               wndpl.length = sizeof(WINDOWPLACEMENT);
+               GetWindowPlacement(hwndMain, &wndpl);
+               IniSectionSetInt(pIniSection, L"WindowPosX", wndpl.rcNormalPosition.left);
+               IniSectionSetInt(pIniSection, L"WindowPosY", wndpl.rcNormalPosition.top);
+       }

        SaveIniSection(INI_SECTION_NAME_SETTINGS, pIniSectionBuf);
        if (!bStickyWindowPosition) {
zufuliu commented 10 months ago

Assuming application is started in primary monitor, what about https://devblogs.microsoft.com/oldnewthing/20141106-00/?p=43683

diff --git a/src/Notepad2.c b/src/Notepad2.c
index 26a7daca..30fb9e36 100644
--- a/src/Notepad2.c
+++ b/src/Notepad2.c
@@ -5731,7 +5731,7 @@ void LoadSettings(void) {
    // window position section
    {
        WCHAR sectionName[96];
-       HMONITOR hMonitor = MonitorFromWindow(NULL, MONITOR_DEFAULTTONEAREST);
+       HMONITOR hMonitor = MonitorFromWindow(NULL, MONITOR_DEFAULTTOPRIMARY);
        GetWindowPositionSectionName(hMonitor, sectionName);
        LoadIniSection(sectionName, pIniSectionBuf, cchIniSection);
        IniSectionParse(pIniSection, pIniSectionBuf);
zufuliu commented 10 months ago

Assuming application is started in primary monitor

I may misread the issue (start Notepad2 in latest used monitor instead of primary monitor?), then position (rcMonitor) for hCurrentMonitor can be saved and used?

milnak commented 9 months ago

generically that would be better, but notepad2 already saves per-monitor positions so I figured that would want to be kept consistent. My patch (above) just remembers the (x,y) and uses that to determine which saved monitor position to use.

zufuliu commented 9 months ago

Is the MonitorFromPoint() patch fixed the bug? if so I can commit it.

milnak commented 9 months ago

It worked on my machine!

zufuliu commented 9 months ago

Committed with minor changes as 49c3f032b47ba7c721dcdebf9ed3e8f107b5b182, please test latest builds from https://github.com/zufuliu/notepad2/actions.

milnak commented 9 months ago

You didn't take my patch as-is so your fix isn't working. My patch adds a global windowposx and windowposy (in the settings section) so that the correct "window position" section will be referenced. Your patch isn't doing that.

zufuliu commented 9 months ago

Should be fixed by edf7ea1020a31990a8ed065942f7681718c82280, failure due to pt loaded from toolbar image section: https://github.com/zufuliu/notepad2/blob/49c3f032b47ba7c721dcdebf9ed3e8f107b5b182/src/Notepad2.c#L5733-L5738

milnak commented 9 months ago

Verified. Thanks for including my patch