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

ColorScheme.SetAttribute is defunct #2286

Closed
tig opened this issue Jan 15, 2023 · 4 comments · Fixed by #2292
Closed

ColorScheme.SetAttribute is defunct #2286

tig opened this issue Jan 15, 2023 · 4 comments · Fixed by #2292
Labels

Comments

@tig
Copy link
Collaborator

tig commented Jan 15, 2023

Current code:

		bool preparingScheme = false;

		Attribute SetAttribute (Attribute attribute, [CallerMemberName] string callerMemberName = null)
		{
			if (!Application._initialized && !preparingScheme)
				return attribute;

			if (preparingScheme)
				return attribute;

			preparingScheme = true;

			switch (caller) {
			case "TopLevel":
				switch (callerMemberName) {
				case "Normal":
					HotNormal = Application.Driver.MakeAttribute (HotNormal.Foreground, attribute.Background);
					break;
				case "Focus":
					HotFocus = Application.Driver.MakeAttribute (HotFocus.Foreground, attribute.Background);
					break;
....
			}
			preparingScheme = false;
			return attribute;
		}

Shouldn't preparingCode be a lock?

While I'm here, the logic in the switch statement has always confused me. I do not understand the intent! It seems arbitrary to be making style decisions in code like this. Can anyone explain clearly?

@tig tig added the bug label Jan 15, 2023
@tig
Copy link
Collaborator Author

tig commented Jan 15, 2023

I think i'm figuring out. New docs:

/// <summary>
		/// 
		/// </summary>
		/// <remarks>
		/// <para>
		/// If <see cref="Application"/> is not initialized yet and <see cref="_preparingScheme"/> is set to <see langword="false"/> 
		/// then the attribute is returned as is (because it's not possible to modify a Scheme wihtout a valid Driver). 
		/// </para>
		/// <para>
		/// If <see cref="Application"/> has been initialized and <see cref="_preparingScheme"/> is set to <see langword="true"/> it means
		/// <see cref="SetAttribute(Attribute, string)"/> is being called recursively and the attribute is returned as is.
		/// </para>
		/// <para>
		/// Otherwise, the attribute returned is created by Driver.MakeAttribute, using the 
		/// foreground and background colors of the attribute.
		/// </para>
		/// </remarks>
		/// <param name="attribute">THe attribute to set.</param>
		/// <param name="callerMemberName">The name of the property (e.g. <see cref="Normal"/> or <see cref="Focus"/>
		/// that called the function. This is used to determine how to make the attribute.</param>
		/// <returns>The attribute that was set.</returns>
		Attribute SetAttribute (Attribute attribute, [CallerMemberName] string callerMemberName = null)

@tig
Copy link
Collaborator Author

tig commented Jan 15, 2023

And... I just set a breakpoint here:

image

And I ran all unit tests and UICatalog and it was never hit.

Is this all defunct code!??!??!

@BDisp
Copy link
Collaborator

BDisp commented Jan 15, 2023

This was made to create automatic attributes, but since they are been creating on the Init method this wasn't used anymore. I don't know who create the preparingScheme flag but for sure was to avoid running the bellow code. I think this private method can be deleted and set the attributes directly with the value.

@tig
Copy link
Collaborator Author

tig commented Jan 15, 2023

After diving in deeper, I agree!

@tig tig changed the title ColorScheme.SetAttribute uses a static var as a lock ColorScheme.SetAttribute is defunct Jan 16, 2023
@tig tig closed this as completed in #2292 Jan 16, 2023
tig added a commit that referenced this issue Jan 16, 2023
Fixes #2286 - ColorScheme.SetAttribute is defunct and should be removed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants