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

Fix getting paths from Uri #5139

Closed
wants to merge 30 commits into from
Closed

Fix getting paths from Uri #5139

wants to merge 30 commits into from

Conversation

rstm-sf
Copy link
Contributor

@rstm-sf rstm-sf commented Dec 5, 2020

What does the pull request do?

Fix getting paths from Uri

What is the current behavior?

Now comparison with paths is mixed: with characters without encoding and with encoded characters

What is the updated/expected behavior with this PR?

Leads to a consistent comparison with no encoded characters

How was the solution implemented (if it's not obvious)?

Use non-ascii or whitespace paths for resources

Checklist

Breaking changes

Fixed issues

@Gillibald
Copy link
Contributor

Without such a change font files with spaces in its name will not work

var fileNameSegments = fileNameWithExtension.Split('.');

fileExtension = "." + fileNameSegments.Last();
var filename = Path.GetFileName(fontFamilyKey.Source.OriginalString);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is such a change necessary? It seems there used to be issues when there was a dot in the name

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Dec 5, 2020

Without such a change font files with spaces in its name will not work

The search actually started with this issue :)

@kekekeks
Copy link
Member

kekekeks commented Dec 5, 2020

System.IO.Path behaves differently across platforms. It should not ever be used for anything but the local file system access.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Dec 6, 2020

Thanks! We shouldn't be able to use a backslash

public void Should_Load_Single_FontAsset_With_Avares()
{
var source = new Uri(AssetLocationAvares + "/Assets/YourFont.ttf");
var key = new FontFamilyKey(source);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this test is added correctly

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Dec 10, 2020

I'm not sure if this fixes the #2555 issue. But it's close, see the reasoning

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Dec 10, 2020

2020-12-10T18:22:54.5507852Z ##[error][xUnit.net 00:00:01.2220774]     Avalonia.Base.UnitTests.Platform.AssetLoaderTests.AssemblyName_With_Non_ASCII_Should_Load_Avares [FAIL]
2020-12-10T18:22:54.5566714Z   X Avalonia.Base.UnitTests.Platform.AssetLoaderTests.AssemblyName_With_Non_ASCII_Should_Load_Avares [< 1ms]
2020-12-10T18:22:54.5567834Z   Error Message:
2020-12-10T18:22:54.5568430Z    System.NullReferenceException : Object reference not set to an instance of an object.
2020-12-10T18:22:54.5568955Z   Stack Trace:
2020-12-10T18:22:54.5569576Z      at Avalonia.Shared.PlatformSupport.AssetLoader.GetAssembly(Uri uri, Uri baseUri) in /home/vsts/work/1/s/src/Shared/PlatformSupport/AssetLoader.cs:line 108
2020-12-10T18:22:54.5570485Z    at Avalonia.Base.UnitTests.Platform.AssetLoaderTests.AssemblyName_With_Non_ASCII_Should_Load_Avares() in /home/vsts/work/1/s/tests/Avalonia.Base.UnitTests/Platform/AssetLoaderTests.cs:line 50

Ok

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Dec 14, 2020

It seems to me that #2555 was already fixed in 0.9

@rstm-sf rstm-sf marked this pull request as ready for review December 14, 2020 20:52
@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jan 6, 2021

Why does mono break

##[error][xUnit.net 00:00:01.4874134]     Avalonia.Base.UnitTests.AvaloniaObjectTests_Binding.TwoWay_Binding_Should_Not_Call_Setter_On_Creation [FAIL]
  X Avalonia.Base.UnitTests.AvaloniaObjectTests_Binding.TwoWay_Binding_Should_Not_Call_Setter_On_Creation [6ms]
  Error Message:
   Assert.False() Failure
Expected: False
Actual:   True
  Stack Trace:
    at Avalonia.Base.UnitTests.AvaloniaObjectTests_Binding.TwoWay_Binding_Should_Not_Call_Setter_On_Creation () [0x00031] in <3cbf0e034bdb495a8e619e4e8782014b>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0003b] in <ab7d7c14aa7a405ca7d85082a965a195>:0 
Results File: /Users/runner/work/1/s/artifacts/test-results/_Mac-1609931676594_2021-01-06_12_16_08.trx

@grokys
Copy link
Member

grokys commented Sep 9, 2021

@Gillibald @kekekeks how does this look now?

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jan 14, 2022

blocked #7347

@robloo
Copy link
Contributor

robloo commented Mar 9, 2022

@rstm-sf This is unblocked now, correct?

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Mar 18, 2022

#7831

@rstm-sf rstm-sf closed this Mar 18, 2022
@robloo robloo mentioned this pull request Mar 22, 2022
3 tasks
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.

6 participants