Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #2195. CsvEditor: Column Type dialog not wide enough. #2203

Merged
merged 15 commits into from Dec 4, 2022

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Nov 7, 2022

Fixes #2195 - Changed MessageBox in the CsvEditor to use layout computed. Also fixes the MessageBox width calculation.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tig
Copy link
Collaborator

tig commented Nov 7, 2022

@BDisp - while testing this fix, I found a latent bug in Dialog. I think this will fix it:

		void LayoutStartedHandler ()
		{
			if (buttons.Count == 0 || !IsInitialized) return;

			int shiftLeft = 0;

			int buttonsWidth = GetButtonsWidth ();
			switch (ButtonAlignment) {
			case ButtonAlignments.Center:
				// Center Buttons
				shiftLeft = (Bounds.Width - buttonsWidth - buttons.Count - 2) / 2 + 1;
				for (int i = buttons.Count - 1; i >= 0; i--) {
					Button button = buttons [i];
					shiftLeft += button.Frame.Width + (i == buttons.Count - 1 ? 0 : 1);
					if (shiftLeft < 0) {
						shiftLeft = 0;
					}
					button.X = Pos.AnchorEnd (shiftLeft);
					button.Y = Pos.AnchorEnd (1);
				}
				break;

The bug shows if you add significantly more buttons than will fit. E.g. 20:

Can you incorporate this fix into this PR? The same shiftLeft < 0 fix should probably be applied to the other alignments.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 7, 2022

The bug shows if you add significantly more buttons than will fit. E.g. 20:

@tig this isn't a bug. Is the normal center alignment behavior. Now the width will be less or equal to the width console. If there are significantly more buttons than will fit, then it will be centered on the available space. Otherwise your fix will be considered as left alignment.

@tznind
Copy link
Collaborator

tznind commented Nov 7, 2022

If there are significantly more buttons than will fit, then it will be centered on the available space. Otherwise your fix will be considered as left alignment.

Could an enhancement be using 2+ lines for the Button if they do not all fit on one? As part of layout (incase console is resized)

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 7, 2022

If there are significantly more buttons than will fit, then it will be centered on the available space. Otherwise your fix will be considered as left alignment.

Could an enhancement be using 2+ lines for the Button if they do not all fit on one? As part of layout (incase console is resized)

It's a good suggestion, but I think is better to open a new issue.

What can be still done in this PR is improving some more unit tests.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 8, 2022

@BDisp - while testing this fix, I found a latent bug in Dialog. I think this will fix it:

		void LayoutStartedHandler ()
		{
			if (buttons.Count == 0 || !IsInitialized) return;

			int shiftLeft = 0;

			int buttonsWidth = GetButtonsWidth ();
			switch (ButtonAlignment) {
			case ButtonAlignments.Center:
				// Center Buttons
				shiftLeft = (Bounds.Width - buttonsWidth - buttons.Count - 2) / 2 + 1;
				for (int i = buttons.Count - 1; i >= 0; i--) {
					Button button = buttons [i];
					shiftLeft += button.Frame.Width + (i == buttons.Count - 1 ? 0 : 1);
					if (shiftLeft < 0) {
						shiftLeft = 0;
					}
					button.X = Pos.AnchorEnd (shiftLeft);
					button.Y = Pos.AnchorEnd (1);
				}
				break;

The bug shows if you add significantly more buttons than will fit. E.g. 20:

Can you incorporate this fix into this PR? The same shiftLeft < 0 fix should probably be applied to the other alignments.

You are right @tig, sorry, I've confused about the button.X which can be negative and I didn't realized the shiftLeft is used in the Pos.AnchorEnd margin.
If you don't mind I'll will fix the #2204 into this PR.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 9, 2022

The errors related with this PR are already fixed, but still the tests fails due the same reason as #2212.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 9, 2022

file-dialog

@tig tig merged commit 254b26a into gui-cs:develop Dec 4, 2022
@BDisp BDisp deleted the csv-editor-fix branch December 4, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CsvEditor: Column Type dialog not wide enough
3 participants