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 #2358 - BREAKING CHANGE: Pos.Combine is incorrect for scenarios involving PosAbsolute. #2385

Merged
merged 78 commits into from
Mar 7, 2023

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Mar 1, 2023

Fixes #2358 - This a set of commits on tig#10. The redraw button behavior doesn't occurs anymore.

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 and others added 30 commits February 25, 2023 17:05
…s not needed because they already overridden the ToString method.
@BDisp
Copy link
Collaborator Author

BDisp commented Mar 4, 2023

Oh my god that was terrible. I wonder to know what was causing so many breaks. I'm certain that may have another issues but not easy to find. We will fixing as they are discovered.

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 4, 2023

@tig please see this. Running separated doesn't throws exceptions.

[xUnit.net 00:00:47.31]     Terminal.Gui.ConfigurationTests.ConfigurationManagerTests.TestConfigurationManagerToJson [FAIL]
[xUnit.net 00:00:47.31]       System.NotSupportedException : The collection type 'NStack.ustring' is abstract, an interface, or is read only, and could not be instantiated and populated. Path: $.Child.Id | LineNumber: 35 | BytePositionInLine: 19.
[xUnit.net 00:00:47.31]       ---- System.NotSupportedException : The collection type 'NStack.ustring' is abstract, an interface, or is read only, and could not be instantiated and populated.
[xUnit.net 00:00:47.31]       Stack Trace:
[xUnit.net 00:00:47.31]            at System.Text.Json.ThrowHelper.ThrowNotSupportedException(ReadStack& state, Utf8JsonReader& reader, NotSupportedException ex)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.Converters.IEnumerableOfTConverter`2.CreateCollection(Utf8JsonReader& reader, ReadStack& state, JsonSerializerOptions options)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonCollectionConverter`2.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.ReadCoreAsObject(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
[xUnit.net 00:00:47.31]            at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, JsonTypeInfo jsonTypeInfo, ReadStack& state)
[xUnit.net 00:00:47.31]            at System.Text.Json.JsonSerializer.Read[TValue](Utf8JsonReader& reader, JsonTypeInfo jsonTypeInfo)
[xUnit.net 00:00:47.31]         /home/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Configuration/Scope.cs(134,0): at Terminal.Gui.Configuration.ConfigurationManager.ScopeJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
[xUnit.net 00:00:47.31]            at System.Text.Json.JsonSerializer.Read[TValue](Utf8JsonReader& reader, JsonTypeInfo jsonTypeInfo)
[xUnit.net 00:00:47.31]         /home/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Configuration/DictionaryJsonConverter.cs(18,0): at Terminal.Gui.Configuration.DictionaryJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
[xUnit.net 00:00:47.31]         /home/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Configuration/Scope.cs(98,0): at Terminal.Gui.Configuration.ConfigurationManager.ScopeJsonConverter`1.ReadHelper`1.Read(Utf8JsonReader& reader, Type type, JsonSerializerOptions options)
[xUnit.net 00:00:47.31]         /home/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Configuration/Scope.cs(132,0): at Terminal.Gui.Configuration.ConfigurationManager.ScopeJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
[xUnit.net 00:00:47.31]            at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
[xUnit.net 00:00:47.31]            at System.Text.Json.JsonSerializer.ContinueDeserialize[TValue](ReadBufferState& bufferState, JsonReaderState& jsonReaderState, ReadStack& readStack, JsonTypeInfo jsonTypeInfo)
[xUnit.net 00:00:47.31]            at System.Text.Json.JsonSerializer.ReadFromStream[TValue](Stream utf8Json, JsonTypeInfo jsonTypeInfo)
[xUnit.net 00:00:47.31]         /home/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Configuration/SettingsScope.cs(51,0): at Terminal.Gui.Configuration.ConfigurationManager.SettingsScope.Update(Stream stream, String source)
[xUnit.net 00:00:47.31]         /home/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/Configuration/ConfigurationMangerTests.cs(323,0): at Terminal.Gui.ConfigurationTests.ConfigurationManagerTests.TestConfigurationManagerToJson()
[xUnit.net 00:00:47.31]            at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[xUnit.net 00:00:47.32]            at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
[xUnit.net 00:00:47.32]         ----- Inner Stack Trace -----
[xUnit.net 00:00:47.32]  

@BDisp BDisp marked this pull request as draft March 4, 2023 22:07
@tig
Copy link
Collaborator

tig commented Mar 5, 2023

@tig please see this. Running separated doesn't throws exceptions.

[xUnit.net 00:00:47.31]     Terminal.Gui.ConfigurationTests.ConfigurationManagerTests.TestConfigurationManagerToJson [FAIL]
[xUnit.net 00:00:47.31]       System.NotSupportedException : The collection type 'NStack.ustring' is abstract, an interface, or is read only, and could not be instantiated and populated. Path: $.Child.Id | LineNumber: 35 | BytePositionInLine: 19.

This is fixed in my PR: BDisp#134 with this:

image

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 5, 2023

This is fixed in my PR: BDisp#134 with this:

@tig but even I want to merge your PR I can't because it has conflicts that must be resolved and so it not work with this my PR.

@tig
Copy link
Collaborator

tig commented Mar 5, 2023

This is fixed in my PR: BDisp#134 with this:

@tig but even I want to merge your PR I can't because it has conflicts that must be resolved and so it not work with this my PR.

Fixed.

@BDisp BDisp marked this pull request as ready for review March 5, 2023 18:35
@BDisp
Copy link
Collaborator Author

BDisp commented Mar 5, 2023

@tig I already finished this PR. The cause of Scrolling scenario is also fixed. Thanks for your merge conflicts fix by submitting the PR. I would like to explain about the Toplevel.Redraw. I did the prior changes because the LayoutNeeded was set to false after the base.Redraw (Bounds); call and thus clearing any sets to true on the View.Redraw. For this reason the tests were failing when calling the Application.Top.Redraw(Application.Top.Bounds)by not call LayoutSubviews. Before it weren't fail but I don't know what cause the failures and and definitely change the Toplevel.Redraw to not call the ClearLayoutNeeded (); and ClearNeedsDisplay (); because the base.Redraw (Bounds); will call it on the View.Redraw. I recommend call the Application.Refresh() which will layout and redraw.

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 5, 2023

@tig the Border changes I'll submit on a separate PR that is still in WIP but is merged on this PR and thus having a solid base branch to continue working.

@tig
Copy link
Collaborator

tig commented Mar 7, 2023

@tig the Border changes I'll submit on a separate PR that is still in WIP but is merged on this PR and thus having a solid base branch to continue working.

I recommend you do not spend any more time on Border for v2. It will all be negated by the new View design.

@tig tig merged commit b7d206b into gui-cs:v2_develop Mar 7, 2023
@BDisp BDisp deleted the v2_layout-improvements branch March 7, 2023 08:53
@BDisp
Copy link
Collaborator Author

BDisp commented Mar 7, 2023

I recommend you do not spend any more time on Border for v2. It will all be negated by the new View design.

@tig I know it's already been negated by the new View design, but I added the Margin on the border scenarios that we can leverage and adapted to the new design. Do you agree?

@tig
Copy link
Collaborator

tig commented Mar 7, 2023

Do you mean what you're working on is adding the ability to set Margin in the scenarios like you can set Padding and Border? If it's just Scenario work, then, yes, I agree.

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 7, 2023

Do you mean what you're working on is adding the ability to set Margin in the scenarios like you can set Padding and Border? If it's just Scenario work, then, yes, I agree.

Yes it's that. Border has already the ability to handle with Margin but content views has a bug by not dealing with the layout of theirs subviews and thus some subviews positions aren't set before they processes the layout of their subviews. Do you want I submit a fix for this bug on a new issue or may I send with the border improvements?

@tig
Copy link
Collaborator

tig commented Mar 7, 2023

Do you mean what you're working on is adding the ability to set Margin in the scenarios like you can set Padding and Border? If it's just Scenario work, then, yes, I agree.

Yes it's that. Border has already the ability to handle with Margin but content views has a bug by not dealing with the layout of theirs subviews and thus some subviews positions aren't set before they processes the layout of their subviews. Do you want I submit a fix for this bug on a new issue or may I send with the border improvements?

I think I understand you, but to make sure, please verify that you understand that the definition of Margin and Padding in v1 are incorrect and in v2 they will use these definitions. Also verify you understand my intent to remove the current Border class completely.

image

See https://github.com/gui-cs/Terminal.Gui/blob/5826321b89512cff8f39c6a5e62864d95135e594/docfx/v2specs/View.md

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 7, 2023

I think I understand you, but to make sure, please verify that you understand that the definition of Margin and Padding in v1 > are incorrect and in v2 they will use these definitions.

Yes I know that are wrong on v1 but with the fixes I'm working on it looks likes identical to your changes. You achieve your changes by adding a view for each element and the Border use only two principal views, The container which have all the thickness (Margin, Border, Padding) and the content view. It also show the 3D effect outside, if enabled. The doubt is if optional the Margin can be draw with a custom color or does nothing keeping the view under it.

Also verify you understand my intent to remove the current Border class completely.

I know you already said that before. I really don't absolute agree with your decision if the new Border behave with the right definitions. In this case Terminal.Gui may have two border definitions. If a border is only used for showing colored thickness around a view the simple class Border is more than enough. If a user want a border where can add adornments inside a Margin, Border or Padding then your implementation is the only that support that.
So, I would suggest keeping both, but for that the Thickness class on the Border class need to kept as struct because the Border (I mean my version) only needs to check if it's empty or not. For that I created a IsEmpty property and I don't need to check if it's null. May be it could have two borders class. Keeping the existent Border and perhaps creating a BorderAdornments.
What do you think?

@BDisp
Copy link
Collaborator Author

BDisp commented Mar 7, 2023

Another idea is creating a IBorder interface and use it the View class as property IBorder Border {get; set; } where the user can use a custom border implementation.

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.

2 participants