Add Bitrate to Dolphin's Additional Information
ClosedPublic

Authored by ngraham on Sep 11 2017, 2:54 AM.

Details

Summary

Adds Bitrate to Dolphin's Additional information columns.

BUG: 368418

Test Plan

Tested in KDE Neon. A bitrate column can be added and shows the bitrate in kb/s:

Works for audio as well as video files!

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
ngraham created this revision.Sep 11 2017, 2:54 AM
anthonyfieroni added inline comments.
src/kitemviews/private/kbaloorolesprovider.cpp
203

It should be

float(value) / 1000

or you will not get a float but an int.

ngraham updated this revision to Diff 19393.Sep 11 2017, 4:08 AM
ngraham edited the test plan for this revision. (Show Details)

Made sure the bitrate is a float, not an int

ngraham edited the test plan for this revision. (Show Details)Sep 11 2017, 4:11 AM
alexeymin added inline comments.
src/kitemviews/private/kbaloorolesprovider.cpp
204

Should this string (" kB/s") also be translated or not? :)

emmanuelp requested changes to this revision.Sep 11 2017, 2:16 PM
emmanuelp added inline comments.
src/kitemviews/private/kbaloorolesprovider.cpp
204
This revision now requires changes to proceed.Sep 11 2017, 2:16 PM
ngraham updated this revision to Diff 19405.Sep 11 2017, 2:37 PM
ngraham edited edge metadata.
ngraham edited the test plan for this revision. (Show Details)

Used the appropriate metricated and internationalized bitrate converter. Thanks, @emmanuelp and @alexeymin!

emmanuelp accepted this revision.Sep 11 2017, 2:46 PM

Thanks!

(I'll commit your patches in the evening)

This revision is now accepted and ready to land.Sep 11 2017, 2:46 PM
aacid requested changes to this revision.Sep 11 2017, 8:05 PM

Patch doesn't apply in src/kitemviews/private/kbaloorolesprovider.cpp ?

This revision now requires changes to proceed.Sep 11 2017, 8:05 PM
ngraham updated this revision to Diff 19425.Sep 11 2017, 8:59 PM
ngraham edited edge metadata.
ngraham marked 3 inline comments as done.

Rebased the diff from git master, so hopefully it will apply cleanly now.

ngraham updated this revision to Diff 19660.Sep 19 2017, 4:29 AM

@aacid, does it apply now for you?

aacid resigned from this revision.Sep 19 2017, 10:09 PM

I don't have time to test now, but i understand you rebased it so if it's good it's good :)

aacid accepted this revision as: KDE Applications.Sep 19 2017, 10:10 PM
This revision is now accepted and ready to land.Sep 19 2017, 10:10 PM

I can test this a bit later today to verify.

Nnnope, patch fails to apply for me, on current dolphin master. ๐Ÿ˜† https://paste.kde.org/ppx1r8xxl

rkflx added a subscriber: rkflx.Sep 20 2017, 7:14 AM

Diff 19425 and Diff 19660 are identical (save for a whitespace change). Maybe the wrong patch was uploaded?

I can only recommend investing a couple of minutes in setting up arc, as it will track and restore the base revisions for uploader as well as reviewer (let me know if you need help with that).

ngraham updated this revision to Diff 19772.Sep 22 2017, 4:53 AM

Updating diff to try to get it to apply with arc (will use arc for future revisions)

This revision was automatically updated to reflect the committed changes.