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 Tracking of Last Line Break #447

Merged
merged 2 commits into from
Feb 2, 2025

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #466

I had a bug in the tracking of the last wrapped position, have fixed that, cleaned up a little and added some caching so that the wrap measurements are not repeated unnecessarily.

@jez9999 if you have the time and fancy running your code against this branch to see if anything else appears that would be very useful. You seem to have the magic touch for finding issues.

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • tests/Images/ReferenceOutput/Issue_446_A__WrappingLength_860_.png: Language not supported
  • tests/Images/ReferenceOutput/Issue_446_B__WrappingLength_860_.png: Language not supported
@jez9999
Copy link

jez9999 commented Jan 30, 2025

Is there a way for me to run just this repo with the other ImageSharp NuGet packages or do I need to remove all NuGet package references and clone all the repos?

@JimBobSquarePants
Copy link
Member Author

Is there a way for me to run just this repo with the other ImageSharp NuGet packages or do I need to remove all NuGet package references and clone all the repos?

You should be able to use the NuGet packages and add Fonts as a project reference pointing to this branch. That’s how we do the sample project in this repo.

@jez9999
Copy link

jez9999 commented Jan 30, 2025

.Drawing seems to have a dependency of Fonts >= 2.0.8 so I'm not sure I can just remove fonts without that too?

@JimBobSquarePants
Copy link
Member Author

.Drawing seems to have a dependency of Fonts >= 2.0.8 so I'm not sure I can just remove fonts without that too?

You should be able to do it like this.

@jez9999
Copy link

jez9999 commented Jan 30, 2025

Doesn't build because of: Unable to find package Microsoft.NET.ILLink.Analyzers. No packages exist with this id in source(s): C:\Program Files\dotnet\library-packs, https://f.feedz.io/sixlabors/sixlabors/nuget/index.json

No idea what that's about.

@JimBobSquarePants
Copy link
Member Author

It looks like it's not finding your standard NuGet package source. I wonder if it's a submodule thing, do you have a populated submodule shared-infrastructure in your Fonts clone?

@jez9999
Copy link

jez9999 commented Jan 31, 2025

Ah. Darn git behaviour, should init all submodules by default I say.

Still now getting various errors along the lines of this for some reason: Duplicate 'System.Reflection.AssemblyCompanyAttribute' attribute

@JimBobSquarePants
Copy link
Member Author

Yeah because you tried to build without the submodules there’s now obj bin folders in all project folders. We use props to put them all under an artefacts folder. You’ll need to close VS, delete all those folders and reopen/build

@jez9999
Copy link

jez9999 commented Jan 31, 2025

OK well I got it building now, but as I suspected, it seems to be using the old Fonts 2.0.8 pulled in by the Drawing NuGet package rather than the referenced Fonts project.

@jez9999
Copy link

jez9999 commented Jan 31, 2025

Never mind, forgot to add the explicit project reference. It now seems to be using the project instead of the package, and I'm still seeing the bug happening. Trying to debug it to see why.

@JimBobSquarePants
Copy link
Member Author

Are you in the correct branch?

@jez9999
Copy link

jez9999 commented Jan 31, 2025

Yeah, I discovered I wasn't. I'm now testing this branch with various stuff. So far it looks good.

@jez9999
Copy link

jez9999 commented Jan 31, 2025

Yep, it seems to be good now with the stuff I have.

@JimBobSquarePants JimBobSquarePants merged commit d74f3fa into main Feb 2, 2025
15 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/additional-linebreak-fixes branch February 2, 2025 01:34
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 this pull request may close these issues.

3 participants