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

Fixes unit tests on Windows #452

Merged
merged 1 commit into from
Jan 6, 2020
Merged

Fixes unit tests on Windows #452

merged 1 commit into from
Jan 6, 2020

Conversation

baelec
Copy link
Contributor

@baelec baelec commented Jan 6, 2020

This fixes unit tests on Windows due to encoding issues.

P.S. If you're interested, I created a Kotlin port over the weekend in my master branch which allows for even finer grained null safety and the removal of a lot of utility methods. Feel free to check it out if that sounds cool.

@drewnoakes drewnoakes merged commit 9602bd7 into drewnoakes:master Jan 6, 2020
@drewnoakes
Copy link
Owner

Thanks for the fix.

I like Kotlin a lot as a language, though I still don't know it as well as I know Java. I'm halfway through (slowly) redesigning the API for the entire lib in the .NET port. Kotlin may be better suited to mirroring the changes I would like to see in the C# version. It possibly creates more friction for contributors though, as fewer people know Kotlin (myself included) and it still compiles to the JVM so doesn't increase platform reach.

Were there any places in particular you felt became much tidier in Kotlin? Can you share some of your experience making the conversion?

@baelec
Copy link
Contributor Author

baelec commented Jan 7, 2020

My first version was just quick and dirty and as close as possible to the original code. I plan to do multiple passes to optimize and streamline.

One possible change would be to add an interface to the static methods / companion objects in the Directory classes thus eliminating the need for mapping static _tagNameMap to getTagNameMap()

There are a couple of places where it made things tidier such as data classes like Face.

Compare

data class Face(val x: Int, val y: Int, val width: Int, val height: Int, val name: String?, val age: Age?) {
  override fun toString(): String {
    var result = "x: $x y: $y width: $width height: $height"
    if (name != null) result = "$result name: $name"
    if (age != null) result = "$result age: ${age.toFriendlyString()}"
    return result
  }
}

to the original that is ~160 lines.

It also helps when mapping a range or collection of values.

getYCbCrCoefficientsDescription went from from ~20 lines to:

val values = _directory.getIntArray(OlympusRawInfoMakernoteDirectory.TagYCbCrCoefficients) ?: return null
      return (0 until values.size / 2)
        .map { Rational(values[2 * it], values[2 * it + 1]).toDouble() }
        .joinToString(" ")
        .takeUnless { it.isEmpty() }

That said, in Kotlin, type conversion is explicit rather than automatic. That means a lot of the bitwise operations can be a bit verbose unless you first cast the values to int or long rather than letting the JVM do it automatically behind the scenes. My previous project in Kotlin was a network traffic analyzer and I didn't find it difficult to manage but if you do an automatic Java-to-Kotlin conversion, the code will be a bit wordy.

@drewnoakes
Copy link
Owner

Thanks very much for the detail. Some of those changes as similar to improvements in expressiveness seen when producing the C# port.

One possible change would be to add an interface to the static methods / companion objects in the Directory classes thus eliminating the need for mapping static _tagNameMap to getTagNameMap()

This area is the primary focus for the API changes I've been exploring. Right now adding a tag involves too much ceremony and duplication. I really need to find the time to sit down and push those through.

One idea I've been floating is moving much of the structure of files into data files that can then be used to more easily produce implementations of the library in different languages, whether by codegen or runtime evaluation.

@baelec baelec deleted the drew branch January 8, 2020 02:58
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.

2 participants