Skip to content

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 #1164

@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Apr 9, 2020
@JimBobSquarePants JimBobSquarePants requested a review from a team April 9, 2020 22:47
@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #1172 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1172   +/-   ##
=======================================
  Coverage   82.39%   82.40%           
=======================================
  Files         684      684           
  Lines       29516    29519    +3     
  Branches     3327     3328    +1     
=======================================
+ Hits        24319    24324    +5     
+ Misses       4508     4507    -1     
+ Partials      689      688    -1     
Flag Coverage Δ
#unittests 82.40% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
src/ImageSharp/Advanced/AdvancedImageExtensions.cs 53.33% <ø> (-16.67%) ⬇️
src/ImageSharp/ImageFrame{TPixel}.cs 87.03% <100.00%> (+1.17%) ⬆️
src/ImageSharp/Image{TPixel}.cs 92.30% <100.00%> (+1.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddfe643...2ec7768. Read the comment docs.

if (mg.Count > 1)
{
span = default;
return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

@antonfirsov Is there a test somewhere I can tap into to test the false condition?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can limit MemoryAllocator's BufferCapacity in many ways to ensure the backing memory is fragmented:

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Should be good after adding tests and exception docs.

if (mg.Count > 1)
{
span = default;
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can limit MemoryAllocator's BufferCapacity in many ways to ensure the backing memory is fragmented:

/// at row <paramref name="rowIndex"/> beginning from the first pixel on that row.
/// </summary>
/// <param name="rowIndex">The row.</param>
/// <returns>The <see cref="Span{TPixel}"/></returns>
Copy link
Member

Choose a reason for hiding this comment

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

The docs are missing <exception> for ArgumentOutOfRangeException.

/// </summary>
/// <param name="rowIndex">The row.</param>
/// <returns>The <see cref="Span{TPixel}"/></returns>
public Span<TPixel> GetPixelRowSpan(int rowIndex)
Copy link
Member

Choose a reason for hiding this comment

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

See previous note.

@JimBobSquarePants JimBobSquarePants merged commit de73c63 into master Apr 13, 2020
@JimBobSquarePants JimBobSquarePants deleted the js/span-accessibility branch April 13, 2020 14:23
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.

Improve accessability of Span<T> methods on Image<T> / ImageFrame<T>

3 participants