Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Check font file length Android #13554

Merged
merged 1 commit into from
Nov 17, 2021
Merged

Check font file length Android #13554

merged 1 commit into from
Nov 17, 2021

Conversation

AlleSchonWeg
Copy link
Contributor

Description of Change

Check if a stored fontfile has a different size as the the embedded fontstream. If only the name is checked, changes in the embedded font are not taken into account.

Issues Resolved

API Changes

None

Platforms Affected

  • Android

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@@ -14,7 +14,8 @@ public class EmbeddedFontLoader : IEmbeddedFontLoader
{
var tmpdir = IOPath.GetTempPath();
var filePath = IOPath.Combine(tmpdir, font.FontName);
if (File.Exists(filePath))
var fileInfo = new FileInfo(filePath);
if (fileInfo.Exists && fileInfo.Length == font.ResourceStream.Length)

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this could lead to false positives. Better solution is to create a hash, but i think in most cases it will work.
So i'm not sure what the best/fastest way to implement it in xamarin android. Feel free to create another PR to solve this issue.

@nrmiller
Copy link

nrmiller commented Sep 1, 2021

@AlleSchonWeg Is this OK to be merged? I just ran into this behavior while working on our Xamarin.Forms app and found this bug & PR. Would be great to have a fix in for the next release! :)

@AlleSchonWeg
Copy link
Contributor Author

@nrmiller
I'm not sure. ATM we use a different font file name when upgrading the icon font.

@jsuarezruiz
Copy link
Contributor

The issue continue in 5.0.0, let's change the target to 5.0.0 and try to include in an upcoming SR.

@jsuarezruiz jsuarezruiz changed the base branch from 4.8.0 to 5.0.0 September 17, 2021 07:41
@AlleSchonWeg
Copy link
Contributor Author

@jsuarezruiz
Thank you. But perhaps it's better to use a hash code?! I think creating and comparing with a hash is safer but slower.

@jfversluis
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jfversluis jfversluis added this to Issues in Progress in 5.0.0 SR8 (Planning) - Target Date Dec. 15th via automation Nov 17, 2021
@jfversluis jfversluis merged commit 41a6548 into xamarin:5.0.0 Nov 17, 2021
5.0.0 SR8 (Planning) - Target Date Dec. 15th automation moved this from Issues in Progress to Done Nov 17, 2021
@AlleSchonWeg AlleSchonWeg deleted the 4.8.0 branch November 17, 2021 15:30
@juwens
Copy link

juwens commented Nov 24, 2021

@jsuarezruiz
Thank you. But perhaps it's better to use a hash code?! I think creating and comparing with a hash is safer but slower.

I even posted the code for doing a hash comparison a while ago in the linked ticket 🙄
And we are using almost the same implementation and is working for quite a while now, even other devs adopted it.

Comparing only by length is lazy and unprofessional.

#11843 (comment)

@jfversluis
Copy link
Member

@jsuarezruiz
Thank you. But perhaps it's better to use a hash code?! I think creating and comparing with a hash is safer but slower.

I even posted the code for doing a hash comparison a while ago in the linked ticket 🙄 And we are using almost the same implementation and is working for quite a while now, even other devs adopted it.

Comparing only by length is lazy and unprofessional.

#11843 (comment)

What is it doing in a comment and not a PR? That almost seems lazy and unprofessional...

@jfversluis
Copy link
Member

jfversluis commented Nov 25, 2021

I won't go into a whole discussion here, but Xamarin and Xamarin.Forms are not part of the subscriptions you are paying for. As you can see in the license that is attached to this repo, there is no support and the product is as-is.

I don't mind you not doing the work, that is fine. Although I think if you wish to participate in a (healthy) open-source ecosystem, you should consider helping out and that has nothing to do with who gets paid by who, but that's a whole other discussion.

What I do mind that you give condescending remarks about other peoples work and calling it lazy and unprofessional and also now being passive aggressive with the last line in your comment. Comments like these do not contribute to any healthy discussion and don't provide anything of value other than potentially ruining people's days.

In the future I would ask you to refrain from doing so as it's not only not nice, it also goes against our code of conduct that you agreed to by participating in this repository.

I'd love to discuss some things you're mentioning here and give you my view on it, if you want to do it in public so others can join please consider opening a discussion on this repo, or find my email address on my GitHub profile if you want to do it in private.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Bug] Updating Embedded Font
7 participants