This revision was automatically updated to reflect the committed changes.
Closed by commit R286:8e79f7ce2b7c: Implement support for writing rating
information for taglib writer (authored by astippich).
REPOSITORY
R286 KFileMetaData
CHANGES SINCE LAST UPDATE
bruns accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R286 KFileMetaData
BRANCH
rating_write2
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh,
astippich updated this revision to Diff 55298.
astippich added a comment.
- cleanup and formatting
REPOSITORY
R286 KFileMetaData
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D18604?vs=55127=55298
BRANCH
rating_write2
REVISION DETAIL
https://phabricator.kde.org/D18604
bruns added inline comments.
INLINE COMMENTS
> taglibwriter.cpp:45
> +
> +#include
>
Remove ...
> taglibwriter.cpp:71
> +int id3v2RatingTranslation[11] = {
> +0,1,13,54,64,118,128,186,196,242,255
> +};
missing spaces
> taglibwriter.cpp:131
> +
> +}
> +
please add an end of anonymous
astippich updated this revision to Diff 55127.
astippich added a comment.
- rebase
- simplify id3 rating to array
REPOSITORY
R286 KFileMetaData
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D18604?vs=55056=55127
BRANCH
rating_write2
REVISION DETAIL
bruns added a comment.
Can you change the id3v2 rating QHash with range checks and a plain array?
Then its fine to go.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, gennad, domson,
astippich added a comment.
After some thinking, I have split up the function for writing specific Ape
and Vorbis tags, as many more tags like "Opus" etc. are in use for Vorbis tags
and not for Ape, and can be easily added this way. I am still using the
property map, though.
Are you fine
astippich updated this revision to Diff 55056.
astippich added a comment.
- split ape and vorbis tag writing functions
REPOSITORY
R286 KFileMetaData
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D18604?vs=50709=55056
BRANCH
rating_write2
REVISION DETAIL
bruns requested changes to this revision.
This revision now requires changes to proceed.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh,
astippich,
bruns added inline comments.
INLINE COMMENTS
> taglibwriter.cpp:80
> +if (newProperties.contains(Property::Rating)) {
> +int rating = newProperties.value(Property::Rating).toInt();
> +id3Tags->removeFrames("POPM");
This requires a range check here - and then you can use a
astippich added inline comments.
INLINE COMMENTS
> bruns wrote in taglibwriter.cpp:229
> but in case file.tag() returns an ApeTag, the dynamic_cast will return a
> nullptr. This is a dynamic_cast, not a static_cast.
It always returns a nullptr
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
bruns added inline comments.
INLINE COMMENTS
> astippich wrote in taglibwriter.cpp:229
> No. Mp3 supports ID3v1, ID3v2 or Ape tags, so simply casting to ID3v2 is not
> possible.
but in case file.tag() returns an ApeTag, the dynamic_cast will return a
nullptr. This is a dynamic_cast, not a
astippich added inline comments.
INLINE COMMENTS
> bruns wrote in taglibwriter.cpp:229
> Is this the same as
>
> auto id3Tags = dynamic_cast(file.tag());
> if (id3Tags) { ... }
>
> ?
No. Mp3 supports ID3v1, ID3v2 or Ape tags, so simply casting to ID3v2 is not
possible.
REPOSITORY
R286
bruns added inline comments.
INLINE COMMENTS
> taglibwriter.cpp:229
> +writeID3v2Tags(file.ID3v2Tag(), properties);
> +}
> file.save();
Is this the same as
auto id3Tags = dynamic_cast(file.tag());
if (id3Tags) { ... }
?
> taglibwriter.cpp:279
> +if
astippich added a dependent revision: D18665: Cleanup taglib writer test.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
astippich added inline comments.
INLINE COMMENTS
> bruns wrote in taglibwritertest.cpp:409
> What are the differences between the various mp3 test cases?
It tests all possible ratings, since the commonly used numbers are somewhat
arbitrary, and I wanted to make sure writing and extracting
astippich updated this revision to Diff 50709.
astippich added a comment.
- rebase on latest parent revision
REPOSITORY
R286 KFileMetaData
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D18604?vs=50505=50709
BRANCH
rating_write2
REVISION DETAIL
bruns added inline comments.
INLINE COMMENTS
> taglibwritertest.cpp:409
> +QTest::addRow("mp3_0")
> +<< QStringLiteral("mp3")
> +<< QStringLiteral("audio/mpeg3")
What are the differences between the various mp3 test cases?
> taglibwriter.cpp:153
> +//map the rating
astippich added a dependency: D18603: Implement more tags for taglib writer.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns,
astippich retitled this revision from "implement support for writing rating
information for taglib writer" to "Implement support for writing rating
information for taglib writer".
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns,
astippich created this revision.
astippich added reviewers: bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.
REVISION SUMMARY
Add support for writing the rating tag to all
supported
21 matches
Mail list logo
