musselwhizzle / Focus-Points

Plugin for Lightroom to show which focus point was active in the camera when a photo was taken
Apache License 2.0
340 stars 42 forks source link

Inconsistencies, question mark chars and truncated values #54

Closed rderimay closed 7 years ago

rderimay commented 7 years ago

The metadata dialog can be improved by using JSON as export format from exiftool and adapting a few methods. I had already implemented in some PR but would separate it as feature in one PR so it is easier to merge :-) (And with the sorting like the current master as I understood that the majority does not like the categories)

musselwhizzle commented 7 years ago

LR doesn't allow non UTC-8 chars to display in it's static_text it seems. Maybe there's a property to set it to something else. Exiftool has flags which allow you to set the chars. charset [[TYPE=]CHARSET]

The truncated values and "?" come from the regex I'm running over the output so that LR can display it. I'm replacing all non text chars with "?". If possible, would like to just see Exiftool flag handle it.

rderimay commented 7 years ago

Yes this is exactly what I implemented actually. It greatly simplifies our code and make it more stable between macs and wins. I would pick it in the big PR I made 2 days ago and make a PR alone. (Understood what you wanted)

musselwhizzle commented 7 years ago

this has nothing to do with mac vs pc.

rderimay commented 7 years ago

on mac our current metadata dialog show up nicely, on win it does not. It is full of question marks which are inserted by our code.

musselwhizzle commented 7 years ago

I understand, but that has nothing to do with the charsets. It has to do with line breaks in the static_text. I'll treat pc as a separate issue.

Are you suggesting no flags in Exiftool would give us a real UTF-8 output?

rderimay commented 7 years ago

No this is wrong. The ? replace chars which are not displayable. Using json as exiftool export, we then have the advantage go having exiftool to make the conversion as JSON by design does not allow the chars we dont like and replaces them by escaped one. I made tests about this. Also, I made tests with charset = utf8 which did not solve the problem.

musselwhizzle commented 7 years ago

To me displaying:

"Nikon_0x00ba": "I2\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000[...]",
  "Nikon_0x00bc": "0220D\u0000,\u0000-\u0000+\u0000?\u0000v\u0000\u000C\u0001??\u0001\u0000?\u0002,?B-?r1'\u00031?r$??\u0010??\u0004(?\u0001\u001D?\u0001  \u0002'?\u0002[...]",

is not an improvement over "?"

What are you suggesting to display there?

(Also, current code in the feature branch using -j doesn't use -sort. You can still -sort the json output)

rderimay commented 7 years ago

I find it to be an improvement over "?" because it give you the info what's written in the filed at least. Event if we care about the difference in 1% of the cases or less, it is better to be able to know that the field contains 0000\u000A+\u0001\u0002-\u0000 instead of ?????+??-?

Also it suppresses completely the need for the current filterText function which for me is much cleaner

musselwhizzle commented 7 years ago

I actually dont find it an improvement. There are already others areas where the output is

"PreviewImage": "(Binary data 160543 bytes, use -b option to extract)",

Nobody is going to use our metadata viewer to look at binary info. But i think this hints at a good compromise, how about if the regex hits we dont just replace the chars with "?", we replace it with the string "(Binary data)" and be done with it. If someone wants to view the binary data in the file, our plugin isn't the place.

Do you like that? Replace all binary lines with "(Binary data)"

rderimay commented 7 years ago

No I profoundly think the json way is better. For one other reason by the way. Using json makes exiftool outputs the keys like : "AFPointsUsed" instead of "AF Points Used" which allows us to make a table of all exifs that is passed to the different methods for searching instating of having the current crappy search with TrimWhiteSpace and so on. If you took a look to the PR transmitted, it made everything way easier. Instead of local a = ExifUtils.findValue(MetaData, value)

we can write local a = MetaData[value]

musselwhizzle commented 7 years ago

Ok, then let's leave it as is. I'll close the issue for now.

musselwhizzle commented 7 years ago

p.s. don't call any committers code crappy. not cool.