Re: Review Request: clear filter history

2012-11-27 Thread Matěj Laitl
> On Nov. 26, 2012, 5:44 p.m., Matěj Laitl wrote: > > Screenshot: clear button > > > > > > I'm not a usability guy, but I really think this button belongs to the > > context menu of the "Search Collection" KLineEdit. Manuel, please resolve this remark. - M

Re: Review Request: clear filter history

2012-11-27 Thread Manuel Finessi
> On Nov. 26, 2012, 10:32 p.m., Manuel Finessi wrote: > > Ship It! > > Myriam Schweingruber wrote: > Please, that is not up to you to decide, only the Amarok core developers > are entitled to give green light for shipping code. And one does not give a > green light for it's own code, anywa

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Matěj Laitl
> On Nov. 26, 2012, 6:16 p.m., Matěj Laitl wrote: > > Hi, thanks for your work! > > > > First, I must acknowledge that our current file playlist code is a big mess > > and it certainly needs cleanup and code deduplication, your work is > > therefore greatly appreciated. I have reviewed just on

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Edward Hades Toroshchin
> On Nov. 26, 2012, 6:16 p.m., Matěj Laitl wrote: > > Hi, thanks for your work! > > > > First, I must acknowledge that our current file playlist code is a big mess > > and it certainly needs cleanup and code deduplication, your work is > > therefore greatly appreciated. I have reviewed just on

Jenkins build is back to normal : amarok_master #113

2012-11-27 Thread KDE CI System
See ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel

Build failed in Jenkins: amarok_master #112

2012-11-27 Thread KDE CI System
See -- Started by remote host 127.0.0.1 with note: Triggered by commit Building remotely on LinuxSlave - 1 in workspace Running Prebuild steps [amarok_master] $ /bin/

Jenkins build is back to normal : amarok_master #111

2012-11-27 Thread KDE CI System
See ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs

2012-11-27 Thread Commit Hook
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/#review22641 --- This review has been submitted with commit eaea90c659ddc4a5024

Re: Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs

2012-11-27 Thread Bart Cerneels
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/#review22640 --- Ship it! Merge this. - Bart Cerneels On Nov. 27, 2012, 12:0

Re: Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs

2012-11-27 Thread Matěj Laitl
> On Nov. 27, 2012, 12:27 p.m., Bart Cerneels wrote: > > src/core/playlists/Playlist.cpp, line 20 > > > > > > You missed the opportunity to move this to Meta namespace. Much work > > elsewhere though. > > There,

Re: Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs

2012-11-27 Thread Bart Cerneels
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/#review22636 --- src/core/playlists/Playlist.cpp

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Matěj Laitl
> On Nov. 26, 2012, 6:16 p.m., Matěj Laitl wrote: > > Hi, thanks for your work! > > > > First, I must acknowledge that our current file playlist code is a big mess > > and it certainly needs cleanup and code deduplication, your work is > > therefore greatly appreciated. I have reviewed just on

Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs

2012-11-27 Thread Matěj Laitl
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/ --- Review request for Amarok, Bart Cerneels, Edward Hades Toroshchin, and Tatja

Build failed in Jenkins: amarok_master #110

2012-11-27 Thread KDE CI System
See -- Started by remote host 127.0.0.1 with note: Triggered by commit Building remotely on LinuxSlave - 1 in workspace Running Prebuild steps [amarok_master] $ /bin/

Build failed in Jenkins: amarok_master #109

2012-11-27 Thread KDE CI System
See -- Started by user Ben Cooksley Building remotely on LinuxSlave - 1 in workspace Running Prebuild steps [amarok_master] $ /bin/sh -xe /tmp/hudson22883411258384819

Build failed in Jenkins: amarok_master #108

2012-11-27 Thread KDE CI System
See -- Started by remote host 127.0.0.1 with note: Triggered by commit Building remotely on LinuxSlave - 1 in workspace Running Prebuild steps [amarok_master] $ /bin/

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Edward Hades Toroshchin
> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: > > src/core-impl/playlists/types/file/LazyPlaylistFile.cpp, lines 22-38 > > > > > > I'm surprised your compiler does not trip up on this. Better remove the > > ';

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Matěj Laitl
> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: > > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp, line 218 > > > > > > Here is the real reason why lazy loading is needed. Reading > > playlist-file c

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Bart Cerneels
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review22631 --- This architecture document should help and avoid me repeating m

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Bart Cerneels
> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: > > I think you started with the wrong assumption that lazy loading of the > > contents of the playlist file is required. > > We have some use cases for PlaylistFile: > > 1) Make playlists found in the collection folders available through the

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Matěj Laitl
> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: > > I think you started with the wrong assumption that lazy loading of the > > contents of the playlist file is required. > > We have some use cases for PlaylistFile: > > 1) Make playlists found in the collection folders available through the

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Myriam Schweingruber
> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: > > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp, line 218 > > > > > > Here is the real reason why lazy loading is needed. Reading > > playlist-file c

Re: Review Request: clear filter history

2012-11-27 Thread Myriam Schweingruber
> On Nov. 26, 2012, 10:32 p.m., Manuel Finessi wrote: > > Ship It! Please, that is not up to you to decide, only the Amarok core developers are entitled to give green light for shipping code. And one does not give a green light for it's own code, anyway. - Myriam --

Re: Review Request: Changes in processing playlist files

2012-11-27 Thread Bart Cerneels
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review22621 --- I think you started with the wrong assumption that lazy loading