Add support for radio streams
Needs ReviewPublic

Authored by jguidon on Jun 1 2019, 5:51 PM.

Details

Reviewers
None
Group Reviewers
Elisa
VDG
Maniphest Tasks
T7567: Add support for radio streams
Summary

Add support for radio streams

Steps of development:

  • play a stream and get some information from it
  • Added a table for radios in the database (V13) with some examples in it.
  • Get the examples in the view and add/play them in the playlist.
Test Plan

after the first reviews, add test for the radios view

Diff Detail

Repository
R255 Elisa
Branch
T7567
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13299
Build 13317: arc lint + arc unit
jguidon requested review of this revision.Jun 1 2019, 5:51 PM
jguidon created this revision.

Hi,

I created a first version for the radio stream support with a view for the Radios based on the Tracks view.

Also, I created a table for the radios in the database, with some defaults examples. Adding the radios in the playlist and play them works.

Here is a screenshot:

I am not sure about some points:

For the radio icon, I tried to use one from the system icons, but I do not know yet why it is blue and where it comes from in the system.

About radio finding/listing, I am not sure what should we propose:

  • should it be possible for the user to add some radios ? If yes would it be possible from the radio view or in the app settings for instance ?
  • should be propose some default radios ? I could find some interesting sites to find many streams but not an api to retrieve streams in an organized way.
  • maybe it can be valuable to have some filters by genre for instance ?
ndavis added a subscriber: ndavis.Jun 2 2019, 12:05 PM

For the radio icon, I tried to use one from the system icons, but I do not know yet why it is blue and where it comes from in the system.

It's blue because it's a mimetype icon and those tend to be colored. It could be copied and made into a monochrome device icon for you.

It's blue because it's a mimetype icon and those tend to be colored. It could be copied and made into a monochrome device icon for you.

It could be great thank you.

After talking in the kde vdg channel, I was told some ideas once we get the first essential features: use a grid view instead of the tracks view and get icon images for radios which should be great to have a rich view.

I will at first work on adding the possibility to add a radio stream manually.

Thanks a lot for your work ! This is really nice !

I am starting to review it. So, I am only able to make some comments due to me compiling it.

There is quite a few warning about Radio enum value not handled in some switch. Could you please fix them ?

Could you also rebase it on current master branch ?

I have also noticed that one can display the info dialog but it shows something else not related to the playing web radio.

For the radio icon, I tried to use one from the system icons, but I do not know yet why it is blue and where it comes from in the system.

It's blue because it's a mimetype icon and those tend to be colored. It could be copied and made into a monochrome device icon for you.

It would be very cool if it is possible.

Thanks, for now I just kept the thing working and I did not care about these warnings, I will take it in account and do the rebase also.

I was aware about the info dialog and I may use it as a starting point for the add radio feature (and maybe for modifying a radio in case the radio address changes for instance).

ngraham added a subscriber: ngraham.Jun 4 2019, 5:34 PM
ngraham added inline comments.
src/qml/MediaRadioDelegate.qml
4

Your copyright since you added the file

226

Don't commit commented-out code without a TODO or other comment explaining why this needs to be commented out instead of removed (i.e. it may be useful in the future or something)

262

Ditto

src/qml/RadiosView.qml
3

Your copyright

76

Don't commit commented-out code

ndavis added a comment.Jun 5 2019, 4:19 AM

@jguidon, I'm going to change the existing radio icon, so you won't need to change your code to get the new monochrome version.

jguidon updated this revision to Diff 59227.Jun 5 2019, 8:29 PM

Fixes:

  • rebase on current master branch
  • fixed swich-cases not taking in account radio
  • removed some comments and cleaned some code
  • enqueue and replace button for all the radios in the view does not appears for the Radios view (suggestion from the vdg discussion, as a stream never ends, so adding several radios into the playlist does not make sense).

Fixes:

  • rebase on current master branch
  • fixed swich-cases not taking in account radio
  • removed some comments and cleaned some code
  • enqueue and replace button for all the radios in the view does not appears for the Radios view (suggestion from the vdg discussion, as a stream never ends, so adding several radios into the playlist does not make sense).

Thanks, I am a bit struggling with my current workload. I will try to provide feedback as soon as I can.

What are the next steps from your point of view ?

Maybe we could try to integrate at least the database part without having to wait for the completion of the interface part ?

jguidon marked 2 inline comments as done.Jun 16 2019, 12:30 PM

Thanks, I am a bit struggling with my current workload. I will try to provide feedback as soon as I can.

You are welcome, no worries, the feature is not blocking :)

I have implemented the add radio feature, here are some screenshots:

The add button :

I kept it at the same place as the Enqueue buttons from the other views, I hope it integrates well with the rest of the application.

The add/edit window, edit is opened with the "View Detail" on the row:


The insertion works, I just need to review my code before pushing. Would you have some suggestions about these first screenshots ?

(I have not looked at the white background on the left panel yet, it occurred just after the rebase)

What are the next steps from your point of view ?

I think I will implement a delete feature, It should not be time consuming given the work already done. Also, it will be nice to add some test automation at some time.
Then I think we will have a nice and usable feature for a first shot.

In the future, it could also be nice to get some icon images for radios.

Maybe we could try to integrate at least the database part without having to wait for the completion of the interface part ?

If we expect other changes on the database, this could avoid larger merges to deal with yes.

Finally, I may have a couple of suggestions/questions for the future of the feature:

  • Should we provide more messaging for error handling in the following cases: an error reading the stream, or an error saving the radio data ?
  • Should we propose some default radio library (or let the possibility to download some set of radios like with the themes in plasma ?)
jguidon updated this revision to Diff 60538.Sun, Jun 23, 8:37 PM

Create and edit a radio, first version

jguidon updated this revision to Diff 60539.Sun, Jun 23, 8:46 PM

Fixed copyright

jguidon updated this revision to Diff 60656.Tue, Jun 25, 3:56 PM

Added delete feature for a radio, delete button is in the editing window

Added delete feature for a radio, delete button is in the editing window

Thanks a lot for the progress on this task.

Due to real life (mainly bad effect of the weather) I will miss time to do anything until at least Sunday. I will also have reduced bandwidth for the next weeks.

It would be really nice if other people can help review your work.

astippich added a subscriber: astippich.EditedWed, Jul 10, 5:28 PM

I tested it today but unfortunately could not get it to add some radios. Nothing happens when I click the apply button

I think it would make sense to split this revision up into several pieces (you can do dependent revisions on phabricator), for example:

  1. database and data model parts
  2. remaining c++ code (mediaplaylist etc.)
  3. interface part

Some general remarks:
There have been some changes in the interface recently, you will have to rebase. Note that the usage of QtQuickControls1 is being faded out in Elisa, please use the Action from QtQuickControls2 and FlatButtonWithToolTip components.
You are duplicating a lot of code (qml delegates, radio view, but also the metadata model), while the differences to the existing ones are minimal. Could you please try to integrate that with the available components?
Some smaller issues inline.

src/databaseinterface.h
89

I think this is unneeded, the resource role QUrl should be able to handle http adresses just fine.

src/qml/ListBrowserView.qml
46

is this one used?

src/qml/MediaRadioDelegate.qml
46

This line does not look right.

src/qml/MetaRadioDataDelegate.qml
42

I get a qml warning here about model.name being undefined

src/qml/PlayListEntry.qml
42

seems unused

It is actually working fine, my problems were local database issues

src/qml/RadiosView.qml
105

contentDirectoryView is not defined

Thank you very much for your feedback, I will work on this revision this weekend.