Add string formatting function to property info
Needs ReviewPublic

Authored by astippich on Thu, Nov 29, 9:16 PM.

Details

Summary

Adds the ability to format the metadata value
for displaying purposes individually for each
property. Currently, users of KFileMetaData
must implement their own custom formatting
functions.
All custom formatting functions from
Baloo-Widgets and Dolphin are copied into
KFileMetaData. This can be extended later.

This adds a dependency on KCoreAddons to
KFileMetaData.

FEATURE: 398581

Test Plan

tests pass

Diff Detail

Repository
R286 KFileMetaData
Branch
display_value
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5699
Build 5717: arc lint + arc unit
astippich created this revision.Thu, Nov 29, 9:16 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptThu, Nov 29, 9:16 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Thu, Nov 29, 9:16 PM
astippich updated this revision to Diff 46509.Thu, Nov 29, 9:18 PM
  • make KCoreAddons a required framework
ngraham added inline comments.
src/formatstrings.cpp
83

"Transposed" and "Transversed" are very technical terms, and it's hard to understand what they mean in this context. I actually have no idea myself! Could we consider replacing these terms with plainer language?

bruns added inline comments.Fri, Nov 30, 12:21 AM
src/formatstrings.cpp
82

Thats somewhat ambigous - flipped along the vertical axis, or top/bottom flipped?

83

Thats mirroring/flipping along the diagonals. You get the same effect when you mirror vertically and rotate +- 90 degrees.

I don't think there is a less technical term.

84

Clockwise or CCW?

astippich added inline comments.Fri, Nov 30, 9:18 AM
src/formatstrings.cpp
82

Actually I don't know, I just copied the code from Dolphin into here.
Seems to be a common term though according to https://docs.microsoft.com/en-us/uwp/api/windows.storage.fileproperties.photoorientation
GIMP describes Flip vertically as top/bottom, see https://docs.gimp.org/2.10/en/gimp-layer-flip-vertical.html

84
bruns added inline comments.Fri, Nov 30, 2:59 PM
src/formatstrings.cpp
84

Of course I can look this up. But this information is not available when the user looks at the string.

Probably we should ask what is the target audience here? The average user only wants the image to be displayed correctly, i.e. apply the transform and be done with it. This IMHO also implies never showing this value by default.

For power users, it may be better to also show the numerical value, so they can look up whats meant.

Otherwise, it is not only necessary to mention CW or CCW, but also if this is the transform which has been applied to the image, or the inverse transform required to show the image correctly.

astippich added inline comments.Fri, Nov 30, 4:15 PM
src/formatstrings.cpp
84

How about using the descriptions from D15503, which are also consistent with digiKam and Gwenview? The best solution is probably to make D15503 work, but this is out of scope for this revision. And a text is still better than a plain number.

bruns added inline comments.Fri, Nov 30, 6:50 PM
src/formatstrings.cpp
84

IMHO thats not better at all - what does "top, left" refer to? And whats the difference to "left, top"?

Every orientation save "normal" and "rotated 180" is ambiguous without further description. It is of no interest for most users, even serious photographers, as it is a technical implementation detail. Creation times, aperture, exposure time ... are useful for users.

So we should hide it by default, and for the 0.1% interested in it we can give a description they can lookup themselves, e.g. "Rotated 90° CCW (6)"

astippich added inline comments.Sat, Dec 1, 11:14 AM
src/formatstrings.cpp
84

Phew, I didn't anticipate that these strings are that controversial since I'm basically copying existing behavior.

I'm fine with adding "CCW" (I think the number should not be shown, it can still be queried separately after all) if that is sufficient for you. Hiding this information is not the responsibility of KFileMetaData.
Besides image size and date, orientation is the only property for images that can be shown in Dolphin currently, so I suspect there are actually users of this.

If we need more fundamental discussion, I'd like to defer that to another revision, since this is currently a drop-in behavior for Dolphin and Baloo-Widgets and copies the status quo. I'd like to land this revision as a starting point to get the infrastructure done and improve upon afterwards. There are many more properties that need special formatting, especially for exif data.

astippich updated this revision to Diff 46688.Sun, Dec 2, 11:40 AM
  • update strings for image orientation
bruns added inline comments.Sun, Dec 2, 9:32 PM
autotests/propertyinfotest.cpp
63

I think this should be "44.1 kHz". Insert the correct expected value and make it QEXPECT_FAIL?

65

dito, "128 kb/s"

src/formatstrings.cpp
81

for 180°, CCW is irrelevant.

astippich updated this revision to Diff 46859.Tue, Dec 4, 7:21 PM
  • small fixes
astippich marked 3 inline comments as done.Tue, Dec 4, 7:23 PM

I'm wondering, anything else to do if I add a new dependency to KFileMetaData?

autotests/propertyinfotest.cpp
63

That was a localization issue.

65

I'm going to fix this shortly after, so I don't bother

src/formatstrings.cpp
81

That was pretty stupid :)

astippich marked 3 inline comments as done.Tue, Dec 11, 8:33 AM

any further comments/objections?