Move meta data properties to new classes. by dlemstra · Pull Request #99 · SixLabors/ImageSharp

dlemstra · 2017-02-05T11:43:27Z

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 practise as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This pull requests moves out all the meta data properties to two new classes. One is called ImageMetaData and this is used in the Image<TColor> class and the other one is called ImageFrameMetaData and is used in the ImageFrame class. The latter only contains a FrameDelay value because we only need this at the moment. This could have been added to the ImageFrame class but the TIFF format will not use this.

codecov-io · 2017-02-05T11:55:38Z

Codecov Report

Merging #99 into master will increase coverage by 0.12%.

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   88.19%   88.31%   +0.12%     
==========================================
  Files         406      410       +4     
  Lines       19493    19575      +82     
  Branches     1405     1400       -5     
==========================================
+ Hits        17192    17288      +96     
+ Misses       1893     1880      -13     
+ Partials      408      407       -1

Impacted Files	Coverage Δ
src/ImageSharp/MetaData/Profiles/Exif/ExifValue.cs	`57.55% <ø> (ø)`
.../Profiles/Exif/ExifTagDescriptionAttributeTests.cs	`100% <ø> (ø)`
src/ImageSharp/Image/ImageBase{TColor}.cs	`91.93% <ø> (-0.49%)`	❌
...aData/Profiles/Exif/ExifTagDescriptionAttribute.cs	`53.84% <ø> (ø)`
...rc/ImageSharp/MetaData/Profiles/Exif/ExifReader.cs	`58.69% <ø> (ø)`
...rc/ImageSharp/MetaData/Profiles/Exif/ExifWriter.cs	`95.17% <ø> (ø)`
...ts/ImageSharp.Tests/MetaData/ImagePropertyTests.cs	`100% <ø> (ø)`
...arp.Tests/MetaData/Profiles/Exif/ExifValueTests.cs	`100% <100%> (ø)`
src/ImageSharp/MetaData/ImageMetaData.cs	`100% <100%> (ø)`
src/ImageSharp.Formats.Png/PngDecoderCore.cs	`63.79% <100%> (ø)`	✅
... and 57 more

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 92e74ec...fe4753c. Read the comment docs.

JimBobSquarePants

Looks good to me. I'll let you decide on the AutoProperties.

JimBobSquarePants · 2017-02-05T12:42:25Z

        /// <param name="image">The <see cref="ImageBase{TColor}"/> to encode from.</param>
        /// <param name="stream">The <see cref="Stream"/> to encode the image data to.</param>
-        public void Encode<TColor>(ImageBase<TColor> image, Stream stream)
+        public void Encode<TColor>(Image<TColor> image, Stream stream)


Oops! Yeah that should definitely be Image<TColor>

JimBobSquarePants · 2017-02-05T12:47:18Z

+        /// The default horizontal resolution value (dots per inch) in x direction.
+        /// <remarks>The default value is 96 dots per inch.</remarks>
+        /// </summary>
+        public const double DefaultHorizontalResolution = 96;


Do we still need these constants?

We can probably do public double HorizontalResolution {get; set;} = 96

They are public so that's why I did not remove them. I did not use auto properties with HorizontalResolution because I want to protect them from being set to a value lower or equal to zero. I am okay with removing the constants and setting it in the constructor. Can I go ahead and change this?

JimBobSquarePants · 2017-02-05T12:47:51Z

+        /// </param>
+        internal ImageMetaData(ImageMetaData other)
+        {
+            Debug.Assert(other != null);


Now I know why you were asking...

Yeah, code talks better than twitter 😄.

antonfirsov · 2017-02-06T20:49:43Z

+
+            set
+            {
+              if (value > 0)


I wonder why do we silently ignore <=0 values instead of throwing an exception. Is this some kind of standard imaging behavior I don't know about? :)

Personally not a big fan of throwing exceptions inside properties. Rather ignore invalid values. What do you prefer @JimBobSquarePants?

Meant to respond to this earlier. We're inconsistent in the library. Sometimes we throw, sometimes we clamp. Leave it as is just now but we should readers the library again before final release.

antonfirsov · 2017-02-06T20:57:02Z

+
+            image.MetaData.SyncProfiles();
+
+            Assert.Equal(100, ((Rational)image.MetaData.ExifProfile.GetValue(ExifTag.XResolution).Value).ToDouble());


I really believe that unit tests are more valuable, if they contain a single AAA block / test case and have a descriptive name saying an actual Fact about the component under test.

A little bit longer to write, easier to maintain and understand.

As you have noticed I have a different way of writing the tests. I use Arrange, Act, Assert, Act, Assert ... a lot of times. But I do think we need some common way of how we set up tests. We have a lot of different styles now. I think we need some help from our Grand High Eternal Dictator @JimBobSquarePants

Tests need a cleanup to be honest. We've got a lot of integration tests but we really need a lot more simple AAA unit tests.

We should look at CoreFX for inspiration. Their tests are a great example of how to keep the tests concise and descriptive.

I also think corefx tests show a good example, I strongly support following their standard:

One test case is testing only one aspect of a "thing" (proving one fact), no long test cases containing multiple AAA blocks

All facts/theories have a descriptive name, no need for additional comments

If the fact (to be proven by the test) is coplex enough, it's encouraged to use underscores in it's name to improve readability.

@antonfirsov You and your naming! 😝 I'll actually allow that in tests since naming is so totally different from normal source and it does improve readability in these cases.

antonfirsov · 2017-02-06T21:30:13Z

+            this.SyncExifProfile();
+        }
+
+        private void SyncExifProfile()


Wouldn't it be better to move this method into ExifProfile and use it as this.ExifProfile?.Sync(this)?

To me it felt like the logic should be inside the ImageMetaData class.

I guess all profiles should probably have a common Sync method enforced by an interface.

It seems to be a Profile-concern to sync it from an ImageMetadata.

Okay i'll move it to the ExifProfile.

Dirk Lemstra added 3 commits February 5, 2017 11:46

Moved the meta data of the image to a new MetaData property.

7a799fe

Added metadata class for image frames.

bacac40

Fixed folder name.

f7dcff1

dlemstra requested review from JimBobSquarePants, antonfirsov, olivif and tocsoft February 5, 2017 11:44

Added method to update the profile data (based on SixLabors#96)

00f0839

dlemstra force-pushed the MetaData branch from 7137e96 to 00f0839 Compare February 5, 2017 12:21

JimBobSquarePants approved these changes Feb 5, 2017

View reviewed changes

Added unit test for Horizontal and Vertical resolution.

50edd0c

tocsoft approved these changes Feb 5, 2017

View reviewed changes

antonfirsov reviewed Feb 6, 2017

View reviewed changes

Moved synchronization code to the ExifProfile.

fe4753c

dlemstra merged commit b179eec into SixLabors:master Feb 7, 2017

dlemstra mentioned this pull request Feb 21, 2017

Refactored the decoder and encoder options. #113

Merged

7 tasks done


		image.MetaData.SyncProfiles();

		Assert.Equal(100, ((Rational)image.MetaData.ExifProfile.GetValue(ExifTag.XResolution).Value).ToDouble());

Conversation

dlemstra commented Feb 5, 2017 • edited by antonfirsov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

codecov-io commented Feb 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dlemstra commented Feb 5, 2017 •

edited by antonfirsov

Loading

codecov-io commented Feb 5, 2017 •

edited

Loading

antonfirsov Feb 7, 2017 •

edited

Loading

antonfirsov Feb 6, 2017 •

edited

Loading

antonfirsov Feb 7, 2017 •

edited

Loading