D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:0f16dd4d4cbf: implement more basic tags for taglibwriter 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15220?vs=41856=42518

REVISION DETAIL
  https://phabricator.kde.org/D15220

AFFECTED FILES
  autotests/taglibwritertest.cpp
  src/writers/taglibwriter.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Stefan Brüns]

bruns accepted this revision.
bruns added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> astippich wrote in taglibwriter.cpp:67
> taglib only allows unsigned int for year and track, so I don't know how 
> that's possible with taglib.
> I added checks to ensure only values greater zero are written to it.

Sorry, erred on the track number, hidden track is "0".

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write

REVISION DETAIL
  https://phabricator.kde.org/D15220

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich added a dependent revision: D15715: remove own implementation of 
QString to TString conversion for taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15220

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich added a dependent revision: D15704: increase test coverage of 
taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15220

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich added a comment.


  In D15220#325580 , @bruns wrote:
  
  > Can you add some Unicode exercising tests?
  
  
  I plan to extend the tests in a later patch, if you don't mind, I would 
prefer to add this there.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:18
> Side note - bad implementation, see D13700 
> 
> 
> Also see inverse q2t in taglibwritertest.cpp

I have that on my todo list to switch to taglib's builtin functions

> bruns wrote in taglibwriter.cpp:67
> Tracknumber can be negative AFAIK, "Hidden track"

taglib only allows unsigned int for year and track, so I don't know how that's 
possible with taglib.
I added checks to ensure only values greater zero are written to it.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15220

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich updated this revision to Diff 41856.
astippich added a comment.


  - check for values greater than zero for tracknumber and year

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15220?vs=40861=41856

BRANCH
  taglib_write

REVISION DETAIL
  https://phabricator.kde.org/D15220

AFFECTED FILES
  autotests/taglibwritertest.cpp
  src/writers/taglibwriter.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Stefan Brüns]

bruns added a comment.


  Can you add some Unicode exercising tests?

INLINE COMMENTS

> taglibwritertest.cpp:18
>  
>  static QString t2q(const TagLib::String& t)
>  {

Side note - bad implementation, see D13700 

Also see inverse q2t in taglibwritertest.cpp

> taglibwriter.cpp:67
> +if (properties.contains(Property::TrackNumber)) {
> +tags->setTrack(properties.value(Property::TrackNumber).toUInt());
> +}

Tracknumber can be negative AFAIK, "Hidden track"

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15220

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich updated this revision to Diff 40861.
astippich added a comment.


  - use different test data

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15220?vs=40858=40861

BRANCH
  taglib_write

REVISION DETAIL
  https://phabricator.kde.org/D15220

AFFECTED FILES
  autotests/taglibwritertest.cpp
  src/writers/taglibwriter.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  implement all basic tags directly provided by taglib for the taglibwriter

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write

REVISION DETAIL
  https://phabricator.kde.org/D15220

AFFECTED FILES
  autotests/taglibwritertest.cpp
  src/writers/taglibwriter.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams