[Bug]: KryptonMessageBox cuts off the last line (sometimes)

[Ahmed-Abdelhameed]

When using KryptonMessageBox to display multiple lines, it sometimes cuts off the last line. I noticed it when displaying exactly three lines, so I ran some tests. AFAICT, it only happens when displaying exactly 3 or 4 lines (less bad for 4 lines though). (this is only true for .NET Core. See comment below for .NET Framework test).

Not sure if this is related to #1592 but it happens whether or not an icon is displayed.

Screenshots:

Code to reproduce:

for (int max = 1; max <= 20; max++)
{
    var lines = Enumerable.Range(1, max).Select(i => $"This is line #{i}.").ToList();
    string joined = string.Join("\r\n", lines);
    KryptonMessageBox.Show(joined);
    KryptonMessageBox.Show(joined, "Error", KryptonMessageBoxButtons.YesNo, KryptonMessageBoxIcon.Error);
}

Tested on latest stable (v85.24.6.176) and latest nightly (v90.24.7.183-alpha).

[Ahmed-Abdelhameed]

Update:

The above test was done on .NET 8.0. The situation is much worse on .NET Framework:

for (int max = 1; max <= 20; max++)
{
    var lines = Enumerable.Range(1, max).Select(i => $"This is line #{i}.").ToList();
    string joined = string.Join("\r\n", lines);
    KryptonMessageBox.Show(joined, $"NumOfLines={max}", KryptonMessageBoxButtons.OK, KryptonMessageBoxIcon.Information);
}

[giduac]

Hi @Ahmed-Abdelhameed,

I'll have look at this.
Might take a bit since we are understaffed ;).

One question though: If possible would a scrollbar be an option to limit the size of the dialog (e.g. a percentage of the screen) as the WinForms MB does overshoot the screen when getting to big?

@Wagnerp
Please assign to me.
This is V90.
Will there be V86?

[PWagner1]

Hi @giduac

Now assigned to you

Will there be V86

No, there will be a patch for v85 in July, fixing the navigator issue (already been dealt with), so you can append to that. In V100, I suggest that we look at the possibility of overhauling VisualMessageBoxForm, as it's causing a lot of issues like this one.

[Ahmed-Abdelhameed]

@giduac

Okay, I did some digging, found the root cause of the problem, and was going to create a PR but there are a few things that I'm not sure I understand.

First, the cause of the problem is that in KryptonMessageBoxForm, there's a method called UpdateMessageSizing; in which, the text is measured based on _messageText.Font, which is incorrect because that's not the font used to display the text.

I checked the history of the file and looks like this bug was introduced in this commit when _messageText was changed from a KryptonWrapLabel to a KryptonTextBox. Previously, _messageText.UpdateFont(); was being called before measuring the text in order to assign the current state's font to the control's .Font property, but that line was removed (presumably because UpdateFont() is not a member of KryptonTextBox).

Now, I'm not sure whether the right thing to do is replicate that same logic for KryptonTextBox because I'm not exactly sure why KryptonTextBox was used in the first place. Looks like there was a problem with KryptonWrapLabel not displaying tab chars but I don't think switching to a text box was the best option. I don't have time to check that bug in the wrap label right now but I'll try to look into it tomorrow. Perhaps fixing that and switching back to using the wrap label would be the best option?

[Ahmed-Abdelhameed]

@giduac

One question though: If possible would a scrollbar be an option to limit the size of the dialog (e.g. a percentage of the screen) as the WinForms MB does overshoot the screen when getting to big?

Sure, that might be a nice-to-have to consider in the future. Although it's not really best practice to display a message box with text that would fill the entire height of the screen, so I wouldn't prioritize this if I were you.

[giduac]

@giduac

Okay, I did some digging, found the root cause of the problem, and was going to create a PR but there are a few things that I'm not sure I understand.

First, the cause of the problem is that in KryptonMessageBoxForm, there's a method called UpdateMessageSizing; in which, the text is measured based on _messageText.Font, which is incorrect because that's not the font used to display the text.

I checked the history of the file and looks like this bug was introduced in this commit when _messageText was changed from a KryptonWrapLabel to a KryptonTextBox. Previously, _messageText.UpdateFont(); was being called before measuring the text in order to assign the current state's font to the control's .Font property, but that line was removed (presumably because UpdateFont() is not a member of KryptonTextBox).

Now, I'm not sure whether the right thing to do is replicate that same logic for KryptonTextBox because I'm not exactly sure why KryptonTextBox was used in the first place. Looks like there was a problem with KryptonWrapLabel not displaying tab chars but I don't think switching to a text box was the best option. I don't have time to check that bug in the wrap label right now but I'll try to look into it tomorrow. Perhaps fixing that and switching back to using the wrap label would be the best option?

Hi @Ahmed-Abdelhameed

The measuring and sizing thereafter is indeed the problem.
Had a quick look but the KTextBox does not support auto scrollbars. So it's on or off. But would be a quick fix.
Shall have a closer look to what works best.

I'll keep you posted.

[giduac]

Hi @Smurf-IV & @Wagnerp,
cc: @Ahmed-Abdelhameed

I've been testing a bit with this.
Most prominent in .NET framework.

Since @Wagnerp said that KMB will get a review in V100 I suggest a quick fix for now. KMB is really not suited for "pages" of text and more centered toward simple messages and questions as is the standard Win(forms) MB.

The easiest certain thing to do for now is to enlarge the calculated vertical message size by a certain percentage....???

Your advice please...

[Ahmed-Abdelhameed]

Hi @giduac

Yeah, once I found out the root cause, it became clear why it's more prominent in .NET Framework (i.e., because the default Control.Font is different between runtime versions).

Re: "quick fix"
If KTextBox will be kept as the control to display the message for now, can we at least replicate the same logic that was used for KWrapLabel? I think that should be easy enough and would be better than hardcoding a percentage. I can submit a PR today for this if you agree.

Note that this isn't about displaying pages of text or anything. It happens with as few as 3 lines. Check the following for example:

[giduac]

Hi @giduac

Yeah, once I found out the root cause, it became clear why it's more prominent in .NET Framework (i.e., because the default Control.Font is different between runtime versions).

Re: "quick fix" If KTextBox will be kept as the control to display the message for now, can we at least replicate the same logic that was used for KWrapLabel? I think that should be easy enough and would be better than hardcoding a percentage. I can submit a PR today for this if you agree.

Note that this isn't about displaying pages of text or anything. It happens with as few as 3 lines. Check the following for example:

How about if you for now push it to your own branch on you account (without submitting a PR).
We can have a look at it, wait for the others' reaction and decide together how to proceed?

Let us know when you pushed the commit?

[Ahmed-Abdelhameed]

@giduac

Done. Here's the commit: Ahmed-Abdelhameed@a82ce80

So, the font was not the only problem. There were a couple more that I fixed as well:

• The calculations did not include the KTextBox's content padding and its container's (KPanel) margin.
• Graphics.MeasureString() does not give accurate measurements when UseCompatibleTextRendering is false (which is the case). TextRenderer.MeasureText() is the right method to use here.

There are a couple of things left (I will fix in a separate commit):

1. Because KTextBox doesn't support vertical alignment, the text is not centered vertically when it consists of one line (or two but less obvious). As a temporary workaround, we could calculate an offset for the text box when the calculated text area's height is smaller than the icon area's height but there's a better way (See below).
2. Because the measurement is more accurate now, there's minimal margin at the top and bottom of the text increasing the margin will make it look better and will also avoid the other problem above.

I will push another commit for this shortly and will create a new issue for this so you can reference it in your own commit.

[giduac]

@giduac

Done. Here's the commit: Ahmed-Abdelhameed@a82ce80

So, the font was not the only problem. There were a couple more that I fixed as well:

• The calculations did not include the KTextBox's content padding and its container's (KPanel) margin.
• Graphics.MeasureString() does not give accurate measurements when UseCompatibleTextRendering is false (which is the case). TextRenderer.MeasureText() is the right method to use here.

There are a couple of things left (I will fix in a separate commit):

1. Because KTextBox doesn't support vertical alignment, the text is not centered vertically when it consists of one line (or two but less obvious). As a temporary workaround, we could calculate an offset for the text box when the calculated text area's height is smaller than the icon area's height but there's a better way (See below).
2. Because the measurement is more accurate now, there's minimal margin at the top and bottom of the text increasing the margin will make it look better and will also avoid the other problem above.

I will push another commit for this shortly and will create a new issue for this so you can reference it in your own commit.

@Ahmed-Abdelhameed

This commit was made against the V85 branch.
We always create a new branch from Alpha or Master-V85 to work on an issue.
That branch is pushed and used later on to publish the PR.
https://krypton-suite.github.io/Standard-Toolkit-Online-Help/Source/Help/Output/articles/Contributing.html

[Ahmed-Abdelhameed]

@giduac

Oh, okay. Let me fix that.

[Ahmed-Abdelhameed]

@giduac

Done: Ahmed-Abdelhameed@16b4440
Should I make one for Alpha as well?

[PWagner1]

Hi @Ahmed-Abdelhameed & @giduac

My $0.02 cents on this...

Patch it for V90, then do a complete overhaul for V100. It is possible to use a KRichTextBox for the content area. The added benefits would be scrolling & detection of URLs. Doing a test for KToasts, using KRichTextBoxes and it works really well.

Maybe either of you can come up with a todo list for the V100 overhaul?

cc. @Smurf-IV

[giduac]

@Ahmed-Abdelhameed

I tested the fix, it works nicely and don't think there will be any objections to it.
Let's complete this for V85 first so the process we use is explained to you.
I'll get back to you after that and set you up for V90.

So before we all publish a PR we always build the project from the command line and include a screenshot of the results.

For V85 there maybe no errors of course.
For V90 there may be no warnings as well.

Screenshot of the included build results image in a PR:

Include a link to the issue being handled which makes the PR show up in the issue..
Personally I always include the branch name in both commits and PR comments.

When you create the PR in GitHub Desktop make sure to set it against the correct branch (V85 of course in this case).

If you have questions .....

[Ahmed-Abdelhameed]

@giduac

PR submitted.

@Wagnerp

It is possible to use a KRichTextBox for the content area. The added benefits would be scrolling & detection of URLs. Doing a test for KToasts, using KRichTextBoxes and it works really well.

RTB will most likely introduce more bugs and overcomplicate things. In my opinion, KMessageBox should be very simple and behave as close as possible to the original Win32 MessageBox. Then, for extra functionality (URLs, etc.), we can have another component like the one(s) that exist in the extended suite (I haven't tested any of them myself).

[giduac]

@Ahmed-Abdelhameed,

Nice, ty!!

This will (as most of the time) be checked tomorrow morning.

If you want to move ahead with V90 the process is practically the same.

• In GH Desktop
• Select the alpha branch (V90 currently) and make sure it's up to date
• Create a new branch from alpha
  

The V90 KMB consists of 4 KMB's / Forms that need attention.

[giduac]

@giduac

PR submitted.

@Wagnerp

It is possible to use a KRichTextBox for the content area. The added benefits would be scrolling & detection of URLs. Doing a test for KToasts, using KRichTextBoxes and it works really well.

RTB will most likely introduce more bugs and overcomplicate things. In my opinion, KMessageBox should be very simple and behave as close as possible to the original Win32 MessageBox. Then, for extra functionality (URLs, etc.), we can have another component like the one(s) that exist in the extended suite (I haven't tested any of them myself).

@Wagnerp & @Ahmed-Abdelhameed
I think if this works that will satisfy the scope of the KMBs.

There's a plan for an instantiable MessageDialog in V100 which will have more options to it and will be designed differently.
KRTB will probably a good control for that if a text area is needed. But that's in the future. We still have quite a lot of open bugs that need to be addressed for V90.

[Ahmed-Abdelhameed]

@giduac

Looks like V90 requires the latest version of Visual Studio Preview, which I'm not using at the moment. I'll see if I can upgrade to it later.

[giduac]

@Ahmed-Abdelhameed ,
Yeah 9.0 preview is needed...

[giduac]

Hi @Ahmed-Abdelhameed,

Been a long time :)...
Do you have an idea when you will finish these MessageBox tickets?

[Ahmed-Abdelhameed]

Hi @giduac

Sorry for the sudden disappearance. It was a pretty hectic week for me.

Okay, I finally installed VS 2022 Preview so that I can build against .NET 9.0. Let me see what I can do.

[giduac]

@Ahmed-Abdelhameed

If you are pressed for time I can finish the KMB tickets?

[Ahmed-Abdelhameed]

@giduac

They're all merged into the V85 branch, right? I was just about to review the changes made to V90, see what's missing, and go from there but I was faced with this problem. I'll wait and see what gets decided and then continue with the fixes.

[giduac]

@Ahmed-Abdelhameed,

V85 was finished afaik.
@Wagnerp already forwarded some of the code into V90. Also the Change log entries have been added by him.
Best to check PR #1622 and see what still needs to be done.

[Ahmed-Abdelhameed]

@giduac

Yes, I checked all that yesterday and already working on the fixes. I have one question though. Is there a reason that the icon in VisualMessageBoxFormDep has a height of 79 (compared to 35 in VisualMessageBoxForm), thus increasing the minimum height of the form?

CC: @Wagnerp

[giduac]

I think @Wagnerp is in charge of the icons around here ;)

[Ahmed-Abdelhameed]

@giduac

Well, the icon itself isn't the problem, but rather the PictureBox control that contains the icon (that's what I meant). The icon size is the same, AFAICT. I think we should make the default size the same as in VisualMessageBoxForm but I wanted to double-check that this wasn't done on purpose.

Edit: Actually, the PictureBox is docked, so the form size is the problem.

[PWagner1]

I think @Wagnerp is in charge of the icons around here ;)

The actual size of the image does not need to be changed, but the size of the form itself. This can be done in the UpdateSizing itself.

[giduac]

Hi @Ahmed-Abdelhameed,
cc: @Smurf-IV, @Wagnerp

The WinForms MessageBox always closes on escape, the krypton kmb sometimes.

            // Escape key kills the dialog if we allow it to be closed
            if (ControlBox
                && (e.KeyCode == Keys.Escape)
               )
            {
                Close();
            }
            else if (e.KeyData == (Keys.Control | Keys.C))

ControlBox sometimes is true and therefore escape only works sometimes.

Could you please have look (when you have time) if ControlBox safely can be removed from the if to replicate the WinForms behaviour.
Probably a new ticket if...

[Ahmed-Abdelhameed]

Hey @giduac

Could you please have look (when you have time) if ControlBox safely can be removed from the if

Definitely not. The WinForms MessageBox does not always close with the Esc key. It only allows closing when it makes sense (i.e., when there's either an OK or a Cancel button), so for example, a YesNo MBox cannot be closed, whereas a YesNoCancel one can be. Looking at the code of KMessageBox, ControlBox is disabled when KryptonMessageBoxButtons is equal to YesNo or AbortRetryIgnore, which is the expected behavior.

[giduac]

Hey @giduac

Could you please have look (when you have time) if ControlBox safely can be removed from the if

Definitely not. The WinForms MessageBox does not always close with the Esc key. It only allows closing when it makes sense (i.e., when there's either an OK or a Cancel button), so for example, a YesNo MBox cannot be closed, whereas a YesNoCancel one can be. Looking at the code of KMessageBox, ControlBox is disabled when KryptonMessageBoxButtons is equal to YesNo or AbortRetryIgnore, which is the expected behavior.

TY & Case closed then I would say :)

[Ahmed-Abdelhameed]

This is already fixed and merged into both V85 and alpha branches.

[giduac]

@Ahmed-Abdelhameed
Nice job!