allow initial state SW_HIDE

[adipose]

First proposed patch. This is used to allow resizelib to work with an initially hidden dialog.

In 2014, wp.showCmd = SW_SHOWNORMAL did not exist, but it still showed the dialog initially.

In order to work with latest code, I put that into an else-block so it doesn't override SW_HIDE.

[ppescher]

I cannot reproduce a hidden dialog, even if I unconditionally set wp.showCmd = SW_HIDE. I tried calling ShowWindow(SW_HIDE) inside OnInitDialog(). I also tried modifying the window style inside OnNcCreate(). Nothing works, the dialog is always made visible at the end. What do you do to make it hidden from the start?

[adipose]

See https://github.com/clsid2/mpc-hc/commit/4dbe739bb3a91ba871ac0bdd37f8d3d97cdc823b for how the code used to be.

    m_wndSubtitlesDownloadDialog.Create(m_wndSubtitlesDownloadDialog.IDD, this);
    m_wndSubtitlesDownloadDialog.ShowWindow(SW_HIDE);

We would create the window and immediately hide it. But the resizelib code would show it again, I believe.

[adipose]

https://github.com/clsid2/mpc-hc/pull/1654

Here is why I had done it originally. The EnableSaveRestore() call was showing the window.

[ppescher]

See clsid2/mpc-hc@4dbe739 for how the code used to be.

    m_wndSubtitlesDownloadDialog.Create(m_wndSubtitlesDownloadDialog.IDD, this);
    m_wndSubtitlesDownloadDialog.ShowWindow(SW_HIDE);

We would create the window and immediately hide it. But the resizelib code would show it again, I believe.

I don't think it does, I tried this pattern under the debugger:

1. The first line will call OnInitDialog() and you just need to specify bRectOnly = TRUE if you don't want to save/restore the maximized/minimized state. The wp.showCmd member is already SW_SHOW and changing it to SW_HIDE does nothing. The dialog is already visible and it stays visible (that's weird, but that's what happens on my Windows 11 PC).
2. The second line will eventually hide the modeless dialog, whatever you do before. The only drawback might be some flash during the previous step. What we need is a way to create the dialog hidden in the first place. I forgot how to do it properly.

[adipose]

We want to restore the state, so that when we do show the dialog, it has the same size as the last run.

[adipose]

There is a way to initially hide the dialog (properly), but we weren't using it.

https://stackoverflow.com/questions/39771342/show-modelless-dialog-initially-hidden

[ppescher]

If the dialog is not initially hidden, the library code does not change it (makes it visible when it already is). If you're ok with calling ShowWindow(SW_HIDE) after modeless dialog creation (initially visible), then you don't need any change. What I can do is simply to avoid overwriting the initial showCmd member, but I would need to test it with a properly initially hidden dialog.

[ppescher]

There is a way to initially hide the dialog (properly), but we weren't using it.

https://stackoverflow.com/questions/39771342/show-modelless-dialog-initially-hidden

Overriding OnWindowPosChanging() works fine and it's the only way to avoid the initial flash if you first do Create() and then ShowWindow(SW_HIDE).

So I think there is no need to change ResizableLib.

[adipose]

Here is what was happening:

CSubtitleDlDlg::OnInitDialog() calls EnableSaveRestore(IDS_R_DLG_SUBTITLEDL, TRUE)

This in turn calls LoadWindowRect(pszSection, TRUE)

Because we want to load "rectangle only," it activates the code in CResizableWndState::LoadWindowRect:

wp.showCmd = SW_SHOWNORMAL;

I believe we were calling the SW_HIDE before because it was forcing it to pop up during init. My change allowed us to force it to hide, even if we use the "rectange only" code.

All of this happens inside the Create() statement.

[adipose]

Overriding OnWindowPosChanging() works fine and it's the only way to avoid the initial flash if you first do Create() and then ShowWindow(SW_HIDE).

Not the only way! The way it works today is wp.showCmd = SW_HIDE which prevents having to override the onWindowPosChanging and add another boolean.

I do not experience initial flash under today's mpc-hc. I used to when we had the extra hide command.

[adipose]

I just reproduced the base case with a simple MFC dialog project. Note, if we do not call the SW_SHOW, the new dialog we just created does not display. If, however, we derive this from a resizablelib class, and have it "restore" the size, it will automatically show, unless we go to extra efforts to stop it, like overriding OnWindowPosChanging which is not necessary with a normal dialog.

//in header file
	CDialog d;

//in src file
int CTestDialogSHOWDlg::OnCreate(LPCREATESTRUCT lpCreateStruct) {
	if (CDialogEx::OnCreate(lpCreateStruct) == -1)
		return -1;

	d.Create(IDD_DIALOG1, this);
	d.ShowWindow(SW_SHOW);

	// TODO:  Add your specialized creation code here

	return 0;
}

[irwir]

Never needed this kind of tricks. Could initial flash be prevented with wp.flags field?

[adipose]

Never needed this kind of tricks. Could initial flash be prevented with wp.flags field?

The flash is simply because it shows it against our wishes and we have to hide it after. The rectonly flag forces it to show and there is no way to tell it otherwise.

We can intercept the call and stop it, but that is a bit inefficient when you could just pass the correct hide flag when that's what you want.

[irwir]

Too late time caused misreading MS docs; no show flags were in description of other method.

However, your test code raises questions.

1. CDialog declaration, but DialogEx:OnCreate called.
2. Normally OnCreate should be called as a part of window creation, not before. Because this notification call implies interaction with the OS's window (not yet created here), but the test code treats it as an MFC CWnd object.

What prevents use of the clear and simple way with removing WS_VISIBLE flag in resource file or in window class registration?

[adipose]

CTestDialogSHOWDlg is the parent dialog. The CDialog is a member dialog and is created on the fly to show that without calling SW_SHOW, the the (child) dialog doesn't display. This is not a very important test but it's an example of how you may create a dialog and want to show it later, not immediately.

[adipose]

OK, so I have a complete example here of the problem in a simple class.

TestRDialog.cpp

#include "pch.h"
#include "TestRDialog.h"

BOOL TestRDialog::OnInitDialog() {
    BOOL r = __super::OnInitDialog();
    EnableSaveRestore(L"testdlg", TRUE);
    return r;
}

TestRDialog.h

#pragma once
#include "resizablelib-master/ResizableLib/ResizableDialog.h"

class TestRDialog :
    public CResizableDialog
{
  virtual BOOL OnInitDialog();
};

Note, this is the main dialog of the app:

class CTestDialogSHOWDlg : public CDialogEx
...
	TestRDialog d;

int CTestDialogSHOWDlg::OnCreate(LPCREATESTRUCT lpCreateStruct) {
	if (CDialogEx::OnCreate(lpCreateStruct) == -1)
		return -1;

	d.Create(IDD_DIALOG1, this);
//	d.ShowWindow(SW_SHOW);

	// TODO:  Add your specialized creation code here

	return 0;
}

The EnableSaveRestore(L"testdlg", TRUE); call causes the dialog to show. Remove it, and it doesn't show. This is the problem. EnableSaveRestore does not need to show the window.

[irwir]

In 2014, wp.showCmd = SW_SHOWNORMAL did not exist, but it still showed the dialog initially.

Github says this code is about 20 year old: https://github.com/ppescher/resizablelib/blob/c386362f2d5fd85f3d98ffa390229c4e5e0586ad/ResizableLib/ResizableWndState.cpp#L125

[adipose]

In 2014, wp.showCmd = SW_SHOWNORMAL did not exist, but it still showed the dialog initially.

Github says this code is about 20 year old:

https://github.com/ppescher/resizablelib/blob/c386362f2d5fd85f3d98ffa390229c4e5e0586ad/ResizableLib/ResizableWndState.cpp#L125

I think I misunderstood when I calculated 2014. The copyright said 2015 when that code arrived to mpc-hc, but prior to that, mpc-hc was actually using a version with a copyright of 2002. And when that code was merged in 2018, it appears the SW_SHOWNORMAL was swapped for SW_HIDE.

[ppescher]

Yes, the code base is that old... I think the first release I published was back in 2000 and it was only CReziableDialog. Later on it became ResizableLib. I'm amazed that this library still has some users nowadays. I've been away from MFC programming for many years and now there is some kind of resize support even in the dialog editor. I wonder how CMFCDynamicLayout compares to ResizableLib and if my library still has some use after all...

Anyway, back to the topic... I will try to reproduce the issue and see if I come up with a nice solution. There is no problem for me to add some parameter, together with bRectOnly (or make it an enum with options maybe?), that won't touch wp.showCmd, but I need to reproduce the issue first in order to test any modifications. I had no luck in my first attempt, maybe because it was a top level window and the framework was trying to show it anyway? I'll try again with a "secondary" window.

[irwir]

The line appeared in 1.4a, and upgrading from 1.3 was non-trivial, but all settled down in the end.

OK, so I have a complete example here of the problem in a simple class.

Maybe complete on your disk, but others have to recreate while not knowing what is the goal. It would be complete as a downloadable project with sources. Like a branch in Github where CDialogEx example was modified according to your specifications, so that the issue could be seen immediately.

And I still would love to get an answer for this:

What prevents use of the clear and simple way with removing WS_VISIBLE flag in resource file or in window class registration?

@ppescher

There is no problem for me to add some parameter

A protected member for changing restored visibility in one very specific case? The current PR might be resolving a certain issue, but looks inelegant.

[ppescher]

I forgot this thread was on a PR ...

And I still would love to get an answer for this:

What prevents use of the clear and simple way with removing WS_VISIBLE flag in resource file or in window class registration?

I'm not sure removing WS_VISIBLE would do the trick. If it does, then we don't need anything else I guess. In my test code I put "Visible = False" in the dialog editor and create it as non-modal dialog, it is still immediately visible after I call dlg->Cleate(dlg->IDD). But that happens even if I comment out the line wp.showCmd = SW_SHOWNORMAL; so I don't know what else is forcing the dialog to be visible. I'd have to try first with a CDialog derived class, not CResizableDialog, and see what happens in "normal" dialogs.

There is no problem for me to add some parameter

A protected member for changing restored visibility in one very specific case? The current PR might be resolving a certain issue, but looks inelegant.

I agree that the protected member is kind of "inelegant". I mean either add an additional function argument after bRectOnly or transform that BOOL argument into some enum (or UINT flags maybe) with additional options. But only if I don't find another more general solution: so far the proposed fix does not seem to work for my test app.

I just took the ResizableDialog demo and converted the first dialog to non-modal, but showing it only after the second one is dismissed:

BOOL CDemoApp::InitInstance()
{
	CDemoDlg* dlg = new CDemoDlg();
	CSecondDialog dlg2;
	//CTestDialog dlg3;

	dlg->Create(dlg->IDD);

	dlg2.DoModal();
	dlg->ShowWindow(SW_SHOW);
	m_pMainWnd = dlg;
	//dlg3.DoModal(); //skip last dialog

And also add this to close the app when the non-modal dialog is closed:

afx_msg void CDemoDlg::OnClose()
{
	DestroyWindow();
}

I tried the proposed fix, I tried overriding OnNcCreate(), I also tried removing the WS_VISIBLE style (in the resource editor or at runtime), but nothing.

I can succeed in hiding the dialog only by overriding OnWindowPosChanging() until some flag I add is flipped (after the line dlg->Create(dlg->IDD);. Otherwise, whatever you do in CResizableWndState::LoadWindowRect() the dialog will be visible, right after creation.

I think there must be some stupid side-effect which is re-setting the visibility of the dialog during the creation phase. But to confirm that I would have to try to make it non-resizable first and see the "normal" behavior.

[ppescher]

Actually I was inaccurate...

If I don't call EnableSaveRestore() inside my CDemoDlg::OnInitDialog(), the dialog stay hidden after creation, until I call ShowWindow(SW_SHOW);.

But, if I call it, I have to force SW_HIDE, I cannot just avoid to override wp.showCmd... I'm puzzled now. I'll have to do more tests...

[adipose]

The line appeared in 1.4a, and upgrading from 1.3 was non-trivial, but all settled down in the end.

OK, so I have a complete example here of the problem in a simple class.

Maybe complete on your disk, but others have to recreate while not knowing what is the goal.

It would be complete as a downloadable project with sources.

Like a branch in Github where CDialogEx example was modified according to your specifications, so that the issue could be seen immediately.

And I still would love to get an answer for this:

What prevents use of the clear and simple way with removing WS_VISIBLE flag in resource file or in window class registration?

@ppescher

There is no problem for me to add some parameter

A protected member for changing restored visibility in one very specific case?

The current PR might be resolving a certain issue, but looks inelegant.

I said I had a complete example not that I uploaded it ;)

Anyway it's just a standard dialog based MFC project generated by visual studio.

Then I added resizablelib, added a resizable derived dialog class, set it to enablesaverestore, and added it to the main dialog class as member.

I can upload the whole project but it's literally just enablesaverestore + create that makes it show.

I can upload the whole project if you like but maybe a mod to the demo dialog is more useful?

[adipose]

Actually I was inaccurate...

If I don't call EnableSaveRestore() inside my CDemoDlg::OnInitDialog(), the dialog stay hidden after creation, until I call ShowWindow(SW_SHOW);.

But, if I call it, I have to force SW_HIDE, I cannot just avoid to override wp.showCmd... I'm puzzled now. I'll have to do more tests...

I think I observed before that showCmd=0 just shows it.

My notes above had some errors about how the code used to be. But I do recall I had to explicitly add the hide rather than ignore showCmd.

Edit: can't be, 0=HIDE.

[ppescher]

It seems if I postpone the call to LoadWindowRect() to the first WM_SHOWWINDOW message with wParam == TRUE (visible) it all works nicely: if the dialog resource specifies Visible = False, then the dialog is not shown during creation, and I don't see bad side effects.

But I think it's better if I make a PR here and let you test this solution first.

As of now LoadWindowRect() is called immediately inside EnableSaveRestore(), which is meant to be called inside the user dialog's OnInitDialog(). Apparently, this is not the best place to modify the window placement. If I do it the first time the window is shown, it seems to fix the issue with invisible non-modal dialogs, and it also works with visible windows of course.

So far so good, and this solution actually makes sense to me, but I want to test it a bit more before I place a commit in the master branch.

[adipose]

The flags member of [WINDOWPLACEMENT](https://learn.microsoft.com/en-us/windows/desktop/api/winuser/ns-winuser-windowplacement) retrieved by this function is always zero. If the window identified by the hWnd parameter is maximized, the showCmd member is SW_SHOWMAXIMIZED. If the window is minimized, showCmd is SW_SHOWMINIMIZED. Otherwise, it is SW_SHOWNORMAL.

So a hidden window, I guess, will always get SW_SHOWNORMAL as it is neither minimized or maximized.

[adipose]

After reading the code more, I think I understand the motivation between connecting the show command to the bRectOnly value. If bRectOnly is set, we don't want to consider min/max state, so we use SW_SHOWNORMAL (note, it also hardcodes this value when storing, which is another reason why it will never be set to zero (SW_HIDE), even if loading a previously stored state.

The problem, then, is that the "default" of SW_SHOWNORMAL is always used when loading something with bRectOnly, even if it was previously hidden when saved, or even if we want some way to enable restoring state, without displaying it immediately.

It occurs to me that a messy workaround would be to alter the data of the state, set the showCmd to whatever you need it to be, and then avoid bRectOnly entirely. As long as you don't set it to min/max, the same thing would be achieved.

[ppescher]

So there are 2 issues here:

1. Not changing the initial/default visible state when you enable the save/restore function
2. Preserving the window visibility in the save/restore data

The proposed solution handles the first case. The second one is a different issue: since I save the state only when the window is destroyed, I'm not sure that visiblity has not already been altered there. It might need to intercept other messages: maybe WM_SHOWWINDOW is enough to handle that too.

[irwir]

it was only CReziableDialog. Later on it became ResizableLib.

Maybe this is the root of issues. Dialog usage commonly follows the pattern: create - show - destroy. But MPC-HC tries to use dialogs in other ways - as a tool windows equivalent that could be hidded and shown again, for one. Not intending to discuss this design choice, but if subtitles data were separated from GUI dialog, the problem would never exist because the dialog could be created and destroyed with negligeable overhead.

The existing implementation of Save and Load routines does not literally execute what the name suggests, because values are changed while saving and while loading (MS have added its own part too). Ideally, Save and Load should not alter settings, and capability to load - modify - apply settings should be added. Maybe EnableSaveRestore should only enable without loading and applying parameters (somewhere comments could be found: call this after(!) OnInitDialog). These changes would be breaking, therefore it should be in version 2.0.

[ppescher]

The existing implementation of Save and Load routines does not literally execute what the name suggests, because values are changed while saving and while loading (MS have added its own part too). Ideally, Save and Load should not alter settings, and capability to load - modify - apply settings should be added. Maybe EnableSaveRestore should only enable without loading and applying parameters (somewhere comments could be found: call this after(!) OnInitDialog). These changes would be breaking, therefore it should be in version 2.0.

In fact EnableSaveRestore() is provided by resizable window classes of this library, to be called by derived (user's) classes to enable the automatic save/restore of their size/position and minimized/maximized state. You're not meant to call load/save manually.

My proposal does not change the current meaning of that function, which is to "enable" the automatic save/restore. Only the restore phase would be moved to a more suitable place, that is the first time the window is made visible. This change should not break anything.