D10918: taglibextractor: Refactor for better readability

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> michaelh wrote in taglibextractor.cpp:331
> You mean other than lines 225, 233, 240?

Yes, but imho we shouldn't even enter the function for mimetypes not having ogg 
tags. Maybe that is overly careful

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, #baloo, #frameworks, astippich
Cc: astippich, ashaposhnikov, michaelh, spoorun, nicolasfella, ngraham, 
alexeymin

D10918: taglibextractor: Refactor for better readability

[Alexander Stippich]

astippich requested changes to this revision.
astippich added a comment.
This revision now requires changes to proceed.


  Looks good imho, just one small comment inline. of course, @mgallien as the 
maintainer must also give his ok.

INLINE COMMENTS

> taglibextractor.cpp:331
> +} else {
> +extractOgg(stream, mimeType, data);
>  }

I think we should check explicitly for the appropriate mimetypes here

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, #baloo, #frameworks, astippich
Cc: astippich, ashaposhnikov, michaelh, spoorun, nicolasfella, ngraham, 
alexeymin

D10918: taglibextractor: Refactor for better readability

[Alexander Stippich]

astippich added a comment.


  Btw, I'm not opposed anymore for merging before D10803 
, as I need some more time to think about 
the value types and probably also need to extend the tests. I will adapt to the 
changes afterwards.

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, #baloo, #frameworks
Cc: astippich, ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich added a comment.


  after D11049  where the frames for the 
media buttons were removed I think this is quite needed for a consistent look 
and feel. I also added 32px version for no repeat and no shuffle which I need 
for a toggle button for #elisa 
  
  F5747288: Screenshot_20180309_193754.png 

  
  F5747290: Screenshot_20180309_193801.png 


REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska, andreask
Cc: andreask, ngraham, #frameworks, michaelh

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

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


  - add more 32px icons

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10292?vs=26512=29110

BRANCH
  playlist_shuffle_repeat

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

AFFECTED FILES
  icons-dark/actions/32/media-playlist-normal.svg
  icons-dark/actions/32/media-playlist-repeat.svg
  icons-dark/actions/32/media-playlist-shuffle.svg
  icons-dark/actions/32/media-repeat-all.svg
  icons-dark/actions/32/media-repeat-none.svg
  icons/actions/32/media-playlist-normal.svg
  icons/actions/32/media-playlist-repeat.svg
  icons/actions/32/media-playlist-shuffle.svg
  icons/actions/32/media-repeat-all.svg
  icons/actions/32/media-repeat-none.svg

To: astippich, #breeze, #vdg, andreaska, andreask
Cc: andreask, ngraham, #frameworks, michaelh

D10694: epubextractor: Handle multiple subjects better

[Alexander Stippich]

astippich added a comment.


  In D10694#221734 , @mgallien wrote:
  
  > In D10694#221719 , @michaelh 
wrote:
  >
  > > This is bad! I have learned baloo itself doesn't handle stringlists. 
Which in my view would be the natural way to handle token-like items like tags, 
keywords and subject(s). Until this is changed in baloo I'm putting this diff 
on hold and fro the time being work around these long subject-strings within 
baloo-widgets.
  >
  >
  > I believe this is quite the opposite. I am already getting strings list for 
some audio metadata. I would like to get your patch in given you make it works 
like already existing code and apart from the baloo-widgets code everybody 
should be fine with your modifications.
  >
  > > @mgallien: Did I understand you correctly, the refactoring helped you to 
understand it better? That would be an incentive for me to do more refactoring.
  >
  > This is this patch that made me read the code in baloo that store the 
properties returned by the extractors. This code is handling strings list quite 
fine and automatically when a property is added several times.
  
  
  Could you please point me to the code location? I would like to investigate 
how the code is used for D10803 .
  Also, since this discussion also applies for the taglibextractor: Is a string 
list preferred for new properties in KFileMetadata when multiple entries are 
possible? I think right now most of the properties are using a single 
concatenated string.

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, dfaure
Cc: astippich, #frameworks, ashaposhnikov, michaelh, spoorun, navarromorales, 
isidorov, nicolasfella, firef, andrebarros, alexeymin, emmanuelp

D10803: handle more tags in taglibextractor

[Alexander Stippich]

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


  - remove lyrics and map encodedby to generator

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=28300=28467

BRANCH
  enhance_taglib

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, spoorun, 
nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich planned changes to this revision.
astippich added a comment.


  In D10803#213783 , @michaelh wrote:
  
  > In D10803#213767 , @astippich 
wrote:
  >
  > > I don't know dolphin works, but given how KFileMetadata is designed, the 
new tags should be ignored until explicit support is added in dolphin.
  >
  >
  > Please try that. For unknown types baloo-widgets (responsible for 
infopanel/tooltips) falls back to `value.toString()`, no idea what will happen 
when you feed it with binary data. The cover properties return binary data, 
right?
  
  
  The cover is binary data, right. And I think I was wrong. While I don't fully 
understand yet how baloo-widgets work, from what I've read all available 
properties are automatically queried. The binary data would then be converted 
to a string as you said. So adding the cover this way may not be a good 
solution. A different interface should be created for image/binary data.
  I think I will split this into two diffs and do the cover images separately.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  I don't know dolphin works, but given how KFileMetadata is designed, the new 
tags should be ignored until explicit support is added in dolphin.

INLINE COMMENTS

> michaelh wrote in properties.h:164
> ?

Oops, seems like a wrong copy & paste :)  will fix

> michaelh wrote in properties.h:168
> Will we break ABI with correct 'Language'?

I noticed the typo as well, but I think this is a matter of API compatibility, 
so I left it unchanged. Matthieu should know.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: michaelh, ngraham, #frameworks, kmorwinski

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  First of all, please review carefully for ABI compatibility, I do not know 
what to watch for that.
  
  So I decided to walk into the minefield that tags are. Everyone implements it 
differently. I've implemented some tags that seem to be supported across a wide 
range of applications (Kid3, Amarok, Kodi... ) but no guarantee that they will 
work everywhere. Some tags were requested on LWN.net in the comments for the 
first alpha (regarding classical music), but most of them only work for Vorbis 
comments as the others lack support.
  The album cover image is currently passed as a QByteArray which then needs to 
be further processed by the application. I don't know if that is the best 
approach, but I think is the most generic one. Comments welcome.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: #frameworks, michaelh

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: mgallien.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  adds the ability to read more tags through taglib when supported by the 
format. prominent feature is the support for reading (front) cover files mp3, 
flac, ogg, opus and mp4. tests including sample files are adjusted for the new 
features.
  Prerequisite for T6255 

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien
Cc: #frameworks, michaelh

D10293: remove view-media-playlist from preferences icons

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:6927b97af007: remove view-media-playlist from preferences 
icons (authored by astippich).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10293?vs=26499=27719

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

AFFECTED FILES
  icons-dark/preferences/32/view-media-playlist.svg
  icons/preferences/32/view-media-playlist.svg

To: astippich, #breeze, #vdg, andreaska, andreask
Cc: andreask, #frameworks, michaelh

D10279: add 24px media-album-cover icon

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:ff179872c915: add 24px media-album-cover icon (authored 
by astippich).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10279?vs=26495=27718

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

AFFECTED FILES
  icons-dark/actions/24/media-album-cover
  icons/actions/24/media-album-cover

To: astippich, #breeze, #vdg, andreaska, andreask
Cc: ngraham, #frameworks, michaelh

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich added a comment.


  Thanks for the feedback! 
  So the correct fix would be to change the 22px and 24px icons to use circles 
around the icons?

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska, andreask
Cc: andreask, ngraham, #frameworks, michaelh

D10279: add 24px media-album-cover icon

[Alexander Stippich]

astippich added a comment.


  Hi, it would be nice to get some feedback on this. Same for D10293 
 and D10292 
.
  If this is not the proper way to contribute to breeze icons, please point me 
in the right direction.

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska
Cc: ngraham, #frameworks, michaelh

D10365: New icon for Elisa music player

[Alexander Stippich]

astippich added a comment.


  I was a little bit hesitant at first, but I'm starting to like the idea of a 
cassette as an icon for Elisa. But I think the text in the icon is not 
required. It will usually be so small that no-one can read it.
  Anyways, Matthieu should have a comment about this.

REPOSITORY
  R266 Breeze Icons

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

To: paullesur, #breeze, #vdg, #elisa, andreaska
Cc: astippich, andreask, andreaska, ltoscano, ngraham, #frameworks, paullesur, 
michaelh, ognarb, januz, kmf, progwolff, mgallien

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

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


  update to 1px of line width, was 1.5 due to the scaling

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10292?vs=26497=26512

BRANCH
  playlist_shuffle_repeat

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

AFFECTED FILES
  icons-dark/actions/32/media-playlist-repeat.svg
  icons-dark/actions/32/media-playlist-shuffle.svg
  icons/actions/32/media-playlist-repeat.svg
  icons/actions/32/media-playlist-shuffle.svg

To: astippich, #breeze, #vdg, andreaska
Cc: #frameworks, michaelh, ngraham

D10293: remove view-media-playlist from preferences icons

[Alexander Stippich]

astippich added a comment.


  I don't know if it is acceptable to just remove the icon. I also don't know 
if this is used anywhere. Stumbled upon while looking into icons for the Elisa 
music player where this icon then creates strange results. I think another 
workaround would be to create a 32px icon for actions.
  
  This is the icon in the preferences folder:
  F5691724: Screenshot_20180204_140032.png 

  
  Personally, I also think that it doesn't fit very well to its description.
  
  The icon with the same name in the actions folder:
  
  F5691729: Screenshot_20180204_140110.png 


REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska
Cc: #frameworks, michaelh, ngraham

D10293: remove view-media-playlist from preferences icons

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: Breeze, VDG, andreaska.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  the view-media-playlist icon in the preferences folder looks completely 
different than in the actions folder and shadows the action icon when using 
sizes > 24px, so remove it

REPOSITORY
  R266 Breeze Icons

BRANCH
  delete_playlist_preferences

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

AFFECTED FILES
  icons-dark/preferences/32/view-media-playlist.svg
  icons/preferences/32/view-media-playlist.svg

To: astippich, #breeze, #vdg, andreaska
Cc: #frameworks, michaelh, ngraham

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska
Cc: #frameworks, michaelh, ngraham

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich added a comment.


  Before: 
  F5691700: Screenshot_20180204_134019.png 

  
  F5691702: Screenshot_20180204_134042.png 

  
  After, which is also how the 24px and lower icons look like:
  F5691706: Screenshot_20180204_134204.png 

  
  F5691708: Screenshot_20180204_134227.png 

  
  Mouse cursors not included of course :)
  Disclaimer: I'm in no way a graphic designer.

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska
Cc: #frameworks, michaelh, ngraham

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: Breeze, VDG, andreaska.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  the 32px icons for media-playlist-shuffle and media-playlist-repeat look 
totally different than the icons with sizes < 24px.
  Also, they do not fit with the current design with their sharp corners. 
Change to the design of the smaller icons. Icons were created using a scaled 
version of the 24px icons.

REPOSITORY
  R266 Breeze Icons

BRANCH
  playlist_shuffle_repeat

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

AFFECTED FILES
  icons-dark/actions/32/media-playlist-repeat.svg
  icons-dark/actions/32/media-playlist-shuffle.svg
  icons/actions/32/media-playlist-repeat.svg
  icons/actions/32/media-playlist-shuffle.svg

To: astippich, #breeze, #vdg, andreaska
Cc: #frameworks, michaelh, ngraham

D10279: add 24px media-album-cover icon

[Alexander Stippich]

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


  use a proper feature branch, which I forgot to use before

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10279?vs=26484=26495

BRANCH
  media-album-cover

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

AFFECTED FILES
  icons-dark/actions/24/media-album-cover
  icons/actions/24/media-album-cover

To: astippich, #breeze, #vdg, andreaska
Cc: ngraham, #frameworks, michaelh

D10279: add 24px media-album-cover icon

[Alexander Stippich]

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


  - do the same thing for dark icons

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10279?vs=26461=26484

BRANCH
  master

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

AFFECTED FILES
  icons-dark/actions/24/media-album-cover
  icons/actions/24/media-album-cover

To: astippich, #breeze, #vdg, andreaska
Cc: ngraham, #frameworks, michaelh

D10279: add 24px media-album-cover icon

[Alexander Stippich]

astippich added a comment.


  It just adds a missing link to an already existing icon, is a screenshot 
needed?

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze
Cc: ngraham, #frameworks, michaelh

D10279: add 24px media-album-cover icon

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: Breeze.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  media-album-cover icon is missing for the 24px size, create symlink to the 
amarok one like other sizes

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

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

AFFECTED FILES
  icons/actions/24/media-album-cover

To: astippich, #breeze
Cc: #frameworks, michaelh, ngraham