optimize adding album and multiple files to mediaplaylist
ClosedPublic
Actions

Authored by astippich on Jul 1 2018, 11:29 AM.

Details

Reviewers

mgallien

Commits

Summary

use the existing methods to batch adding an album and files.
adjust tests and fix exposed errors

Test Plan

test pass, adding of album and files in elisa still works

Repository

R255 Elisa

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

astippich requested review of this revision.Jul 1 2018, 11:29 AM

astippich created this revision.

Thanks for taking care of that. Please have a look at my comments.

src/mediaplaylist.cpp
590	Instead of that, please extend MusicAlbum to allow access to the list of tracks and just enqueue a list of MusicAudioTrack. That would be the fastest solution.
621–629	Instead of modifying it, it is possible to only keep one instead of having enqueue(QList<String>) and enqueue(QList<QUrl>) ?
791–801	Why do you want to change that ? It seems at least unrelated to your patch.

This revision now requires changes to proceed.Aug 31 2018, 8:58 PM

astippich marked an inline comment as done.Sep 1 2018, 9:58 AM

astippich added inline comments.

src/mediaplaylist.cpp
621–629	You mean providing only one method for file urls, and getting rid of adding stringlists? that is certainly possible, but requires more changes throughout the code. for example, elisaapplication currently only gives a stringlist to the mediaplaylist. I would prefer to do that separately.
791–801	This is needed to pass the tests and makes it more similar to the method adding a single file url. The method was implemented by me for the file browser, and was not unit tested until now.

ping

src/mediaplaylist.cpp
621–629	I agree.
791–792	Can you use auto here ?
src/musicalbum.cpp
224	Please use a const reference ? That should avoid allocating a list that will later be only read.

Thanks for your work

This revision is now accepted and ready to land.Oct 15 2018, 7:52 PM

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			autotests/mediaplaylisttest.cpp (119 lines)
M			src/mediaplaylist.cpp (21 lines)
M			src/musicalbum.h (2 lines)
M			src/musicalbum.cpp (5 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	37005	282e527		Jul 1 2018, 11:29 AM	★	★
Diff 2	40810	282e527	- directly query track list from music album	Sep 1 2018, 9:55 AM	★	★
Diff 3	43510	baaa538	- make const ref and use auto	Oct 12 2018, 8:35 PM	★	★
Diff 4	43688	26d9a72	R255:2d812af35e77b179c59cdb4a1bbc494829dde350	Oct 15 2018, 8:17 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
8908d6666bb8	85b6b9bae080	5586c44cf51d	Alexander Stippich	fix test	Oct 12 2018, 8:35 PM
5586c44cf51d	c4a4c84c6685	7b7637a93131	Alexander Stippich	make const ref and use auto	Oct 12 2018, 8:17 PM
7b7637a93131	e586703fa77a	baaa53863113	Alexander Stippich	optimize adding album and multiple files to mediaplaylist (Show More…)	Oct 12 2018, 8:12 PM