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 #1358. Attribute.Foreground / Attribute.Background now working with CursesDriver #1359

Merged
merged 3 commits into from
Jul 21, 2021

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Jul 1, 2021

To allowing some test on some specifics ConsoleDriver features, the initialization should be done in the Init method (ConsoleDriver) and in the Setup (IMainLoopDriver), instead in the constructors.

@BDisp
Copy link
Collaborator Author

BDisp commented Jul 5, 2021

282 The active test run was aborted. Reason: Test host process crashed : Error opening terminal: unknown.

I don't know what going here.

@tznind
Copy link
Collaborator

tznind commented Jul 5, 2021

282 The active test run was aborted. Reason: Test host process crashed : Error opening terminal: unknown.

I don't know what going here.

Looks like there are a lot of failed calls to xclip right before the crash message (although that could be coincidence). The issue manifested in 9abcdec right? What exactly does that commit do? Is it possible that that commit makes some tests use the 'real' driver when previously they used FakeDriver?

The error message seems to crop up a lot in real life (i.e. outside of CI) and be associated with the variable $TERM not being defined (also here)

It is also the commit that introduces MakeAttribute_Testing_Almost_All_Drivers which is explicitly going to construct the Terminal.Gui.CursesDriver. Is that the first time any test has constructed that class? maybe it doesn't construct properly given some environmental considerations. Since the error message seems to crop up quite often if you search for it is it possible it is coming from ncurses itself?

@BDisp
Copy link
Collaborator Author

BDisp commented Jul 5, 2021

In the last commit I didn't see any error caused by the xclip, only in the previous.
I run all the unit tests on Windows, Linux and Mac without any problems.

Normally I need to export TERM environment variable on Docker images. Maybe the server is not prepared to run a real terminal in Linux.

Is that the first time any test has constructed that class?

Yes, is the first time and maybe the environment is not properly prepared to run a terminal in the server image. But I'll need some help for this, please. Thanks.

@tznind
Copy link
Collaborator

tznind commented Jul 5, 2021

Ok I have enabled GitHub Actions in my fork of the repo (actions are disabled by default in forks) and have created a minimum repro of the issue:

tznind#19

Its a PR from a branch to main in my repo because the build test workflow seemed to want that in order to run.

I'll do a bit of exploring but I've also noticed that Console.WriteLine throws NotImplementedException in the CI container so it might be that it's simply not designed to run a real console session as you say.

@BDisp
Copy link
Collaborator Author

BDisp commented Jul 5, 2021

I'll do a bit of exploring but I've also noticed that Console.WriteLine throws NotImplementedException in the CI container so it might be that it's simply not designed to run a real console session as you say.

With the WindowsDriver and NetDriver, using windows console, you can't create an handle to a real console session. This the reason why I removed this initialization from the drivers constructors to the Init method. So we must avoid using Console functions.
The CursesDriver is the only that give no error creating a handle to a window and we need to call The Init and the End methods, because ncurses need to load the libraries to deal with the colors functions.

@tznind
Copy link
Collaborator

tznind commented Jul 5, 2021

Yup, seems as soon as you call Init ncurses crashes hard (in CI container). It might be that we just cant test this functionality this way and scrap the test?

I tried setting environment variable TERM but didn't help and felt hacky anyway.

@BDisp
Copy link
Collaborator Author

BDisp commented Jul 5, 2021

It looks like it will have to be. Do you think I should also discard all changes to the drivers?

@tznind
Copy link
Collaborator

tznind commented Jul 6, 2021

It looks like it will have to be. Do you think I should also discard all changes to the drivers?

Its up to you, I think the important thing is to fix the Foreground/Background color issue itself which is what this PR does.

I think changing Constructor/Init logic is something else and if it was only done to delay the 'error creating a handle' exception then we probably don't need/want that behaviour change.

@BDisp BDisp marked this pull request as draft July 6, 2021 10:54
@BDisp BDisp force-pushed the cursesdriver-attribute-fix branch from c0da8ff to f256046 Compare July 6, 2021 11:53
@BDisp BDisp marked this pull request as ready for review July 6, 2021 12:23
@tznind
Copy link
Collaborator

tznind commented Jul 6, 2021

Great stuff! I had a little play around with your InvertColors scenario to make it cooler. If you don't like it you can ignore it:

List<Label> labels = new List<Label> ();
var foreColors = Enum.GetValues (typeof (Color)).Cast<Color> ().ToArray ();
for (int y = 0; y < foreColors.Length; y++) {

	var fore = foreColors [y];
	var back = foreColors [(y + 1) % foreColors.Length];
	var color = Application.Driver.MakeAttribute (fore, back);

	var label = new Label ($"{fore} on {back}") {
		ColorScheme = new ColorScheme (),
		Y = y
	};
	label.ColorScheme.Normal = color;
	Win.Add (label);
	labels.Add (label);
}

var button = new Button ("Invert color!") {
	X = Pos.Center (),
	Y = foreColors.Length+1,
};
button.Clicked += () => {

	foreach(var label in labels) {
		var color = label.ColorScheme.Normal;
		color = Application.Driver.MakeAttribute (color.Background, color.Foreground);

		label.ColorScheme.Normal = color;
		label.Text = $"{color.Foreground} on {color.Background}";
		label.SetNeedsDisplay ();

	}
};

image

@BDisp BDisp marked this pull request as draft July 6, 2021 12:25
@BDisp BDisp marked this pull request as ready for review July 6, 2021 12:34
Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@tig tig merged commit 481bac9 into gui-cs:main Jul 21, 2021
@BDisp BDisp deleted the cursesdriver-attribute-fix branch July 21, 2021 15:55
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.

3 participants