Save window position and dimensions. Improve Preferences Form.

[regs01]

Added optional ability to save window position and/or dimensions.

Also improved Preferences form a bit. Everything is aligned now. And it's resizeable now, which is more convenient.

[JeromeMartinez]

build with this PR.
@regs01 thank you for your proposal, it is very appreciated.
Unfortunately I face an issue when I open the preference windows, I have an empty display:

It is fine after I click on a page.
We use C++Builder 11 Community Edition.

@cjee21 is the change proposal about how to compute window sizes is fine at high DPIs?

PS: @regs01 please rebase, I just merged another PR and now it is conflicting, sorry about that, it is rare.

[cjee21]

@cjee21 is the change proposal about how to compute window sizes is fine at high DPIs?

I'm new to VCL so I just work on improving the existing codes. If there is a better and more proper way from someone more experienced in VCL then we should use that. I will test it later. We need to test on Windows 7 (to ensure no issues like previously) too and ideally it should be tested by someone with a multi-monitor, multi-DPI system. The existing implementation was not tested on this kind of set-up either so I don't know how it handles that.

It seems now the default is to not remember window position and remember window size. We need to consider whether to keep old behaviour as default (set both defaults to not remember) or not.

Also remember to use spaces for indents (it looks like tabs now).

[JeromeMartinez]

It seems now the default is to not remember window position and remember window size. We need to consider whether to keep old behaviour as default (set both defaults to not remember) or not.

I think it is a good intermediate.

Also remember to use spaces for indents (it looks like tabs now).

True. @regs01 please use spaces instead of tabs.

[cjee21]

Unfortunately I face an issue when I open the preference windows, I have an empty display

Yes it's blank unless click on one of the items in the tree. Also it is entirely white with no distinction between the left, right and bottom sections. I don't know if that's a good idea. Also is it intended that the main window is re-spawned every time Preferences is closed?

The issue reported by Klaus1189 is still there and now is triggered as soon as Preferences is opened. Seems to take very long to open Preferences too.

[cjee21]

@cjee21 is the change proposal about how to compute window sizes is fine at high DPIs?

With remember window size disabled it is acceptable for me on different DPIs but it is different size/ratio than what MediaInfo has been using for years so I don't know how others will react.

With remember window size enabled, if user changes DPI after using MediaInfo at least once, it uses size for old DPI and window is too small (or maybe large). This may also cause issues for people using multi-monitors with different DPIs (I don't know how the behaviour is here since I no longer have multi-monitors).

[JeromeMartinez]

it is different size/ratio than what MediaInfo has been using for years so I don't know how others will react.

I am not shocked by the default and not so different so it is acceptable for me.

if user changes DPI after using MediaInfo at least once, it uses size for old DPI and window is too small (or maybe large).

@regs01 is it possible to save a kind of DPI independent ratio rather than a pixel size?

[cjee21]

I am not shocked by the default and not so different so it is acceptable for me.

How default looks here:

Acceptable for me but no idea about other more particular users.

[JeromeMartinez]

@cjee21 yours is not near from former version, ouch. too much height.

I have that:

Difference due to DPI somewhere?

[JeromeMartinez]

@regs01 in practice we have 2 topics, the save window pos/dim on one side and the default size on another side. IMO it would be better to split the PR in 2: the first one keeps the same method for computing the default size. when it is merged, you send a PR with the changes you suggest about the default size.

[cjee21]

Difference due to DPI somewhere?

Not DPI. that's the default on 100% (96DPI). It is because I'm using higher resolution display with more screen estate (>=1440 height). Not sure if it'll be even longer at 4K. So if there is a particular user who does not like to remember window size, that user may not be happy with a big change in the percentage of screen estate that MediaInfo uses by default. It is useful for text/HTML view but if the user uses Basic most of the time, that user may see that as wasted white space (and also blocking stuff behind that the user may want to see to compare).

[cjee21]

@JeromeMartinez Another major bug. When invoked from explorer context menu produces a window with no file open.

This change is causing lots of effects besides remember window size and change Preferences look.

• Preferences taking long time to open (I have a fast PC, what will happen on older PCs?)
• Main window re-spawning and flashing after Preferences closed (take longer and looks glitchy compared to the re-spawning we get when changing themes)
• The open via command line / context menu issue

[cjee21]

At least it does not seem to break Windows 7 compatibility.

Note: We can remove delay load from user32 if we no longer need to get DPI manually.

[cjee21]

We should set a minimum size if we allow resizing Preferences window:

and for main window too.

[cjee21]

Took a more detailed look at the changes in this PR.

I agree with changing the window size code to use VCL (Monitor and ScaleFactor) instead of the current one and remove all the native Windows API calls along with delay load of user32.

I also like that the window size/position saving is planned to be implemented as an option and that window position is disabled by default.

I think this should not be added:

    if (fpUnscaledMonitorHeight>=1440)
        Height=900;

as it is deemed "too much height" and may cause other users to complain although I can tolerate that

@cjee21 yours is not near from former version, ouch. too much height.

This should not be simply removed from FormShow without sufficient testing and other changes:

    //Refresh global
    FormResize(NULL);
    Refresh();

These two functions have been triggered by FormShow long before I started contributing. There is a reason why it is there.

@JeromeMartinez these are my opinions since you asked me for a review. As for the other changes, I am not so sure but think they require more testing and changes due to many side-effects/issues mentioned earlier. Of course, you make the decision on what is to be done (I'm just a user/contributor) :)

[JeromeMartinez]

@JeromeMartinez these are my opinions since you asked me for a review.

Thanks :).

As for the other changes, I am not so sure but think they require more testing and changes due to many side-effects/issues mentioned earlier.

I guess that the best method here is to split this PR is 2 or more PRs with specific changes rather than all in one, too much difficult to review impacts.

[cjee21]

Maybe it is a good idea to make both window size and position saving disabled by default so that users used to existing behaviour will not be affected and especially with the DPI change causing unintended window size. That is why I try to keep the window size the same as before even on high-DPI displays after adding high-DPI support. Then users who want their own size can turn the setting on and resize the window to their liking and deal with resizing it again when changing DPI since the feature will be documented in changelog. If concerned about people not reading changelog and knowing the existence of the feature, maybe can make MediaInfo display changelog on first run after upgrade like what some apps do.

[regs01]

Unfortunately I face an issue when I open the preference windows, I have an empty display:

I moved switching tab visibility before selecting active page.

@cjee21 is the change proposal about how to compute window sizes is fine at high DPIs?

I commented in the line computing Preferences form height. There is no need as VCL scales it automatically. It's enough to have fine looking height in design time. I see also new commits to chage lables height for Language and Output. There is also no need for that. They just needed to be aligned to baseline of comboboxes, as they were aligned to bottom of comboboxes. VCL take control of scaling.

We use C++Builder 11 Community Edition.

Yeah, I used C++ Builder 11.3 Community Edition. On VirtualBox, so it wouldn't mess up with old lifetime Delphi 10.2 Starter license. Although i'm using Lazarus now. But I must say how bad C++ Builder code completion is. It's so slow and buggy. Delphi's one is a little better. Even CnPack can't help much of situation. But Lazarus is completely different level - works instantly and only showing items within the scope. It just doesn't have prediction, as Visual Studio, but in everything else Lazarus code completion is even better than VS.

True. @regs01 please use spaces instead of tabs.

I always use spaces, although RAD Studio seem to default tabs. There could be few, before I changed it. Probably CnPack also does it, i'll look up on it.

Maybe it is a good idea to make both window size and position saving disabled by default so that users used to existing behaviour will not be affected and especially with the DPI change causing unintended window size. That is why I try to keep the window size the same as before even on high-DPI displays after adding high-DPI support. Then users who want their own size can turn the setting on and resize the window to their liking and deal with resizing it again when changing DPI since the feature will be documented in changelog. If concerned about people not reading changelog and knowing the existence of the feature, maybe can make MediaInfo display changelog on first run after upgrade like what some apps do.

I have 4K with 150% myself. It's just uncommon if users want to change scale factor on a daily basis. It is also highly unlikely that a person with high DPI display would use Windows 7. I left Position off by default, as this could be matter of preference. But with dimensions users wouldn't notice difference, as they will keep having default size and location. But those using Text, Tree views and similar will be able to quickly address form height issue without going into Settings.

@regs01 please rebase, I just merged another PR and now it is conflicting, sorry about that, it is rare.

I'll look up on rebase.

We should set a minimum size if we allow resizing Preferences window:

I did for Height, as i didn't do scroll box. I can do for Width. It's 1-second operation.

as it is deemed "too much height" and may cause other users to complain although I can tolerate that

#203
There is no too much on large screen, like I have. Especially when you use Text/Tree views. I think most large screen owners will appreciate this, although I can remove it, considering Dimensions are saved on by default. So those using Text/Tree wound just be able to resize themselves.

This should not be simply removed from FormShow without sufficient testing and other changes:

Cut off little bit less. I'll readd it. Although as I see at FormResize now, there is no need in most of it. All controls just need to be properly aligned.

[cjee21]

One more thing. If MediaInfo is closed while maximized, the next time it will start un-maximized but filling up the entire screen. This is not a problem for me and I have seen other apps behave like this but other users may find it annoying. I suggest if possible and not too difficult, make it remember if it is maximized or not and also the window dimensions saved should be for un-maximized so that user can restore their original un-maximized size.

[cjee21]

is it possible to save a kind of DPI independent ratio rather than a pixel size?

We can save the window size at 96DPI and scale it accordingly when we read/write the value. That should work. It may be rare for a user to change DPI but what if there are people who use laptop and dock it on a monitor with a different DPI every day?

[JeromeMartinez]

dev snapshot.

A lot better but it is still slower, as if something is called too many times (the language files maybe?).
the "new version available" test is also there but I guess it is a sidecar bug due to dynamic update of the interface without the right settings.

he dark mode is fine but in light mode it is super flat, some separation lines disappeared:

This PR has too many topics in one shot, it would still be better to split it in several PRs, one per topic, e.g. a first PR could be a resizable pref window and only that, and we merge it after validation then do a next step e.g. save window preferences without touching the algorithm for the default size then touching the algorithm for the default size.

[cjee21]

dev snapshot.

Confirmed that invoking from CLI/context menu is still broken as I expected earlier after a quick look at the codes.

@JeromeMartinez I don't feel like reviewing this anymore

1. The indents are not fixed as desired by you even though this is the easiest thing

2. The complexity of the commits has increased instead of becoming clearer as you asked for and it was merged instead of rebased?

3. This PR has too many topics in one shot, it would still be better to split it in several PRs

   Topics that are actually in this PR:

   1. Changing native Windows APIs to VCL APIs for DPI
   2. Adding new default window height for >1440p
   3. Implementing remember window position/size and the option
   4. Redesigning look of Prefs window
   5. Unknown changes that broke CLI invocation and caused main window to refresh on Preferences closed

4. I said to not remove FormResize and Refresh from FormShow to prevent breaking things. It was re-added but now FormShow is removed? So still broken and high risk of other things breaking that are yet to be discovered.

5. I do not get why it is required to redesign Preferences window

   • The existing one is perfectly serving it's purpose
   • The new all-white design without separation does not match with the rest of MediaInfo or other Windows apps
   • I don't see any benefit of being able to resize it, it's just all white space
   • It is now slower to open even noticeable on a 5GHz 1-year-old CPU (will it take ages for users on old PCs?)
   • The fix we added recently for aligning the buttons' heights on high DPI as requested by Klaus1189 has been undone:
     

6. All these make me can't help but question if it was sufficiently tested and if there is a clear understanding of how the existing codes work before attempting change.

7. I suggest getting Klaus1189 to check the Preferences window instead of me since he appears to care about this window way more than me and to a pixel level. I only see this window once every update so I don't really care what happens here as long as can be used to change settings when required.

[regs01]

A lot better but it is still slower, as if something is called too many times (the language files maybe?).

You mean, when switching tabs? That's because ComboBoxes are filled every time tab is switched. I didn't touch it. It's an old behavior. A better option would be to fill them once at form creation. And when a language is added.

[JeromeMartinez]

You mean, when switching tabs? That's because ComboBoxes are filled every time tab is switched.

With the General tab, the one with languages.
I remark that the behavior is slower than without this PR, so something changed in this PR which is not related to its purpose.
Again, it would be easier for all if we have separate PR, one small PR per purpose, so we can easily catch the piece of code which changes the behavior.

It's an old behavior.

There is definitely bad old behaviors but it is not good to make that worse while adding good things.

Your work is really appreciated but we need to find a way to avoid regressions when we add features.

[regs01]

Confirmed that invoking from CLI/context menu is still broken as I expected earlier after a quick look at the codes.

What context menu and where? Any steps to reproduce?

I said to not remove FormResize and Refresh from FormShow to prevent breaking things. It was re-added but now FormShow is removed? So still broken and high risk of other things breaking that are yet to be discovered.

Everything is moved to GUI_Configure, which is called by constructor, as it should be. FormResize event is there. Deprecating it would be a next move.
https://github.com/regs01/MediaInfo/tree/mainformrearrange

The fix we added recently for aligning the buttons' heights on high DPI as requested by Klaus1189 has been undone:

I didn't see the issue in VirtualBox with 125%, so I removed it. Check on my main system. Now I can see it. ComboBox does not scale properly. LCL has a feature to lock two controls to central lines of each other, as well as to top and or bottom line. Unfortunately VCL doesn't have it. Guess Embarcadero have no wish to develop VCL further. I'll brought it back, so as added to AfterMonitorDpiChanged.

• The new all-white design without separation does not match with the rest of MediaInfo or other Windows apps

Since Windows 7 dialogs generally using clWindow color. I just made it the way so misalignments wouldn't be noticeable. But as there are no TabSheet borders now and if you don't like clWindow, it's can be easily changed to clBrnFace for TPanel and TForm. However, I might suggest something in between - clWindow for TPanel and clBtnFace for Form.

There is definitely bad old behaviors but it is not good to make that worse while adding good things.

Your work is really appreciated but we need to find a way to avoid regressions when we add features.

I literally didn't touch it. TabSheet events are same. Delay is exactly the same.
You see ComboBox_Update call RefreshFilesList, which is reading all CSV language files. Slower the drive is, the longer delay would be. It might be also possible that you are using just one CSV file for development. So the only way to fix it is to move ComboBox_Update outside of drawing - into form creation.

screen2gif-2024-07-03.webm

[cjee21]

What context menu and where? Any steps to reproduce?

Right click file in Windows Explorer > MediaInfo or using CLI MediaInfo.exe %pathtoafile% must not produce a MediaInfo window that looks like no file is open.

Everything is moved to GUI_Configure, which is called by constructor, as it should be. FormResize event is there. Deprecating it would be a next move. https://github.com/regs01/MediaInfo/tree/mainformrearrange

This one is up to @JeromeMartinez but since MediaInfo is working well right now, I think it is very risky to make changes that may break things and that require lots of testing especially since there are other more important issues that time should be spent on (including MediaInfoLib stuff). Also I'm not sure if the plan to replace this VCL version is still on.

I didn't see the issue in VirtualBox with 125%, so I removed it. Check on my main system. Now I can see it. ComboBox does not scale properly. LCL has a feature to lock two controls to central lines of each other, as well as to top and or bottom line. Unfortunately VCL doesn't have it. Guess Embarcadero have no wish to develop VCL further. I'll brought it back, so as added to AfterMonitorDpiChanged.

Everything should be tested upto 200% DPI scale as there are users using 200% that reported issues previously.

I literally didn't touch it. TabSheet events are same. Delay is exactly the same. You see ComboBox_Update call RefreshFilesList, which is reading all CSV language files. Slower the drive is, the longer delay would be. It might be also possible that you are using just one CSV file for development. So the only way to fix it is to move ComboBox_Update outside of drawing - into form creation.

The slowness I noticed and mentioned is when opening the Preferences window it takes longer to appear and I'm using a 5GHz CPU + DDR5 + PCIe 4.0 SSD. I'm also comparing with release version, not development.

As for the other things relating to Preferences window design, I suggest to just ask Klaus1189 who originally reported design issues with it instead of asking me.

Lastly I agree with splitting the PRs. I suggest the first PR for replacing the native Windows API calls as that can be merged immediately after one more quick test. Second one probably for implementing option to save window size/position since there is a feature request for it on MediaInfo website.

[regs01]

The slowness I noticed and mentioned is when opening the Preferences window it takes longer to appear and I'm using a 5GHz CPU + DDR5 + PCIe 4.0 SSD. I'm also comparing with release version, not development.

As for the other things relating to Preferences window design, I suggest to just ask Klaus1189 who originally reported design issues with it instead of asking me.

But I just made a video. It's totally identical. There is nothing in my commits that affect it. But I can fix it anyway by moving reading CSV language files into form creation, so it just fires once. As well as a new language is added.
It might be not loading in Firefox though. Here is the link.
https://github.com/MediaArea/MediaInfo/assets/10468217/e8924105-7079-46bf-8925-86a0b95953b8

Lastly I agree with splitting the PRs. I suggest the first PR for replacing the native Windows API calls as that can be merged immediately after one more quick test. Second one probably for implementing option to save window size/position since there is a feature request for it on MediaInfo website.

We can discuss it all first, as it saves time

Right click file in Windows Explorer > MediaInfo or using CLI MediaInfo.exe %pathtoafile% must not produce a MediaInfo window that looks like no file is open.

I moved it down after reading args. Speaking of Refresh procedure it's better to rename it to avoid ambiguity, as TForm has own Refresh as a descendant of TWinControl. Could be named something like UpdatePageContent.

[regs01]

Moving ComboBox_Update to FormCreate. And it was already in Language_NewClick and Language_DeleteClick. Same to be done to other ComboBoxes.

Link, if embeded video doesn't work in Firefox
https://github.com/MediaArea/MediaInfo/assets/10468217/af133654-9bad-4754-bee2-85337265370d

screen2gif-2024-07-04-1955.webm

[JeromeMartinez]

New build.
This is still slow but as slow as in a build without this PR, I guess that a slowdown was introduced in a a previous PR so not anymore a topic here but still need investigation in a separate issue or PR (@cjee21 I think it is from a previous PR made by you, before v24.06 as v24.06 also has it).

We can discuss it all first, as it saves time

For the moment the only regression I still see is the lack of lines (they are there before this PR) in the light theme, also visible in the video, and it is not expected that theses lines change in a PR about main window position and re-sizable preference windows.

[regs01]

I still can't get what slowness are you talking about? Is it one in the video?
https://github.com/MediaArea/MediaInfo/assets/10468217/e8924105-7079-46bf-8925-86a0b95953b8

I didn't make a commit yet. But there is a video with partial fix. I'll finish later, when come home.
https://github.com/MediaArea/MediaInfo/assets/10468217/af133654-9bad-4754-bee2-85337265370d

[cjee21]

New build. This is still slow but as slow as in a build without this PR, I guess that a slowdown was introduced in a a previous PR so not anymore a topic here but still need investigation in a separate issue or PR (@cjee21 I think it is from a previous PR made by you, before v24.06 as v24.06 also has it).

If we're still talking about the Preferences window, here are the commits history: https://github.com/MediaArea/MediaInfo/commits/master/Source/GUI/VCL/GUI_Preferences.cpp
All commits I made are after 24.06 release.
As for the function that launches Preferences window in GUI_Main, the only change I made was to FormResize to fix the blank toolbar visible despite being disabled issue that was reported. This only executes after Preferences window is closed and is after 24.06 release too. (https://github.com/MediaArea/MediaInfo/blame/65794412e6888ff1f9f68a8069600f787f56a6f2/Source/GUI/VCL/GUI_Main.cpp#L1625)

Time taken to open Preferences window:

24.04 (last release before I contribute/32-bit):	0.851s
24.06.20240702 (this PR's first snapshot/32-bit):	1.602s
24.06.20240703 (latest master?/64-bit):	0.751s
24.06.20240704 (this PR's latest snapshot/64-bit):	1.017s

I don't see any statistically significant difference regarding Preferences window after my contributions as expected from the commits history. The slightly faster time might be due to 64-bit and considering that a new option was added by your team recently, the results look good.

I don't know what happened but 1st version of this PR was bad. Now it can be considered okay. But if the faster speed is just due to now having 64-bit, then it is still no good. Take note that the above test results are likely not accurate to tens of ms (based on 30fps video & only run once) so hundred or so ms difference can be ignored.

I still can't get what slowness are you talking about? Is it one in the video? https://github.com/MediaArea/MediaInfo/

I'm not sure which slowness he is talking about (if he is talking about the one in your video, it has been like that for years AFAIK) but the one I talk about in all my comments is time taken for the Preferences window to show up which was bad in 1st build of this PR but can be considered okay now for me.

---

UPDATE: If you want to know the reason why Preferences window now takes longer to show up, the cause is as I had initially suspected and now confirmed with some tests.

This takes 0~2ms on both 64-bit and 32-bit to execute:

Page->Top=-(Page->TabHeight*1.15);
Page->Height=Page->Height+(Page->TabHeight*1.15);
Cancel->Top=Page->Top+Page->Height+(Cancel->Height*0.15);
OK->Top=Cancel->Top;
ClientHeight=OK->Top+(OK->Height*1.15);

This takes ~310ms on 64-bit and ~530ms on 32-bit to execute:

for (int cTabIndex = 0; cTabIndex <= Page->PageCount-1; cTabIndex++)
    Page->Pages[cTabIndex]->TabVisible = False;

I timed this using code so it is accurate. It also agrees with the difference found from the video recording method above.
This is on a 5GHz CPU from 2023 with high-speed DDR5 RAM so it may be way slower on older PCs.

---

UPDATE2: This still not reverted to previous fix:

[regs01]

UPDATE2: This still not reverted to previous fix:

Cherry picked to this branch

[regs01]

This takes ~310ms on 64-bit and ~530ms on 32-bit to execute:

Oh this, insignificant. Still it cannot take half a second. Mine isn't 5 GHz. And what's more it's VirtualBox. So it's slow, very slow. But once again - this (whole GUI_Configure) is about to be moved from OnShow to OnCreate, so will take 0 ms (under 1 ms).

screen2gif-2024-07-05-0754.webm

[regs01]

I made a commit here, so you measure it.

However, moving ComboBox_Update into TForm.OnCreate from TTabSheet.OnShow will make form opening slow anyway. But it's still better than having visual lags and flickering.