Add support for radio streams
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Fix display bug for the track mainLabel (-1 was displayed)
  • Removed MediaRadioMetadataView and integrate it to MediaTrackMetadataView
  • TrackMetadataModel: better integration
  • Fix View Detail window not opening in single album view.
  • ContentView: remove unecessary component for radios
  • Removed unecessary lines and variables in qml
  • DataListView: radio bool property is set from viewmanager
  • Fixed warnings in DataListView by adding cases in datamodel
jguidon marked 7 inline comments as done.Jul 27 2019, 11:58 PM

Thank you for your feedback, I still have one warning opening the detail view for a radio: "QFSFileEngine::open: No file name specified". I am working on it.

Would you have some advice/documentation on how to split the revision? I am quite new on using phabricator/arc and I prefer asking before doing anything wrong.

src/qml/DataListView.qml
136

Sorry for that, I had many warnings opening the view.
I tried to correct these warnings by adding some cases in datamodel.cpp

src/qml/ListBrowserView.qml
47

Yes, it is now used in the Connection in DataListView.qml

jguidon updated this revision to Diff 62674.Jul 28 2019, 11:24 AM
  • DatabaseInterface::updateRadioInDatabase: better fields settings for radios on creation
  • rebase on master
jguidon updated this revision to Diff 62677.Jul 28 2019, 12:02 PM

Removed unecessary radioModified signal from ModelDataLoader

jguidon marked an inline comment as done.Jul 28 2019, 12:12 PM
jguidon added inline comments.
src/modeldataloader.h
68

Indeed I have removed radioModified signal which was not needed in this class.

For the three others, I kept a defined role for each of them:

  • allRadiosData: init for the radios list view
  • allRadioData: init for the MetaData view
  • allRadiosDataUIUpdate: update of the corresponding radio line in radio list view and of the MetaData view

Should it be better if I try to integrate them or at least give them some better names?

src/models/trackmetadatamodel.cpp
476

Maybe it is more readable this way?

Nice progress, I think we are getting there!
Some comments inline which look like a lot, but most of them are style nitpicks

Thank you for your feedback, I still have one warning opening the detail view for a radio: "QFSFileEngine::open: No file name specified". I am working on it.

Would you have some advice/documentation on how to split the revision? I am quite new on using phabricator/arc and I prefer asking before doing anything wrong.

No fear, I think everyone had some issues with phabricator :)
The way I do it (which must not necessarily be the ideal/correct way) is creating a branch for each revision I would like to update.
So create a branch for your first changes, do "arc diff", create a new branch from this one, do "arc diff HEAD~numberOfNewCommitsInThisBranch" and repeat.
You can mark the revisions as dependent revision by using the "Depends on DXXXXX" keyword in the git commit message or just mark it afterwards in the phabricator web view.

I actually would like to get it merged rather sooner than later as this revision touches various places and is probably a pain to rebase.
We could merge as a single big commit if you promise to stay around a little while in case some things need changing afterwards.
@mgallien should take a look before and agree with this though

src/CMakeLists.txt
384 ↗(On Diff #62677)

this can be deleted

src/audiowrapper_libvlc.cpp
236–244

code style
if (...) {

239

code style
} else {

569

code style
if (...) {

src/databaseinterface.h
436

can you make them consistently with the other, e.g. radioRemoved(), radiosAdded()?

src/elisaqmlplugin.cpp
170

is this one actually needed?

src/manageaudioplayer.cpp
417

code style
if (...) {

src/mediaplaylist.cpp
600

code style
if (...) {

965

code style
} else if (.....) {

src/modeldataloader.h
68

If you rename it now to radioModified I think it is pretty consistent with the rest

131

can these be renamed for consistency with others?

src/models/datamodel.cpp
179

use QString instead of QStringLiteral(""), it is a little bit cheaper

src/models/datamodel.h
146

just call it radioModified for consistency with the others

src/models/trackmetadatamodel.cpp
177

Not needed anymore for the general case

476

Yeah, much better!

I think your changes makes it even possible to provide some groundwork for future metadata editing for the tracks.
Create a new itemType enum, e.g. EditableTextEntry, and set it to this for the radio/stream case
Implement this new EditableTextEntry for the MetaDataDelegate similar to all other itemTypes.
Ideally, the MetaDataView and the MetaDataDelegate would not need to know if it's a radio or not, but would retrieve this information from the metadatamodel

560–579

this belongs also into the "else" block below, doesn't it?

562

code style
if (....) {

573

code style
} else {

src/qml/DataListView.qml
39

i think "isRadio", or actually "isStream" is better fitting here

src/qml/MediaTrackMetadataView.qml
32

call this isRadio/isStream for consistency and make it an alias of realModel.isRadio

51

remove comment :)

src/qml/MetaDataDelegate.qml
31

codeStyle

40

codeStyle

mgallien requested changes to this revision.Jul 28 2019, 5:17 PM

Thanks for your big work. This will be a really nice contribution.

I think that the database part is nice and could be already integrated.

The views and models could be made more generic. That would be more future proof and avoid having to rework them too much when the next feature lands.

To conclude, I have successfully managed to add a new radio and listen to it. Congratulations, I really enjoy being able to do that !

src/databaseinterface.cpp
60

The initialization order for the members generate some warnings at compilation:

../src/databaseinterface.cpp: In constructor ‘DatabaseInterfacePrivate::DatabaseInterfacePrivate(const QSqlDatabase&)’:
../src/databaseinterface.cpp:158:15: warning: ‘DatabaseInterfacePrivate::mSelectTracksMappingPriority’ will be initialized after [-Wreorder]

QSqlQuery mSelectTracksMappingPriority;
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

../src/databaseinterface.cpp:156:15: warning: ‘QSqlQuery DatabaseInterfacePrivate::mSelectRadioIdFromHttpAddress’ [-Wreorder]

QSqlQuery mSelectRadioIdFromHttpAddress;
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

../src/databaseinterface.cpp:47:5: warning: when initialized here [-Wreorder]

DatabaseInterfacePrivate(const QSqlDatabase &tracksDatabase)
^~~~~~~~~~~~~~~~~~~~~~~~

../src/databaseinterface.cpp:222:15: warning: ‘DatabaseInterfacePrivate::mUpdateAlbumArtistQuery’ will be initialized after [-Wreorder]

QSqlQuery mUpdateAlbumArtistQuery;
          ^~~~~~~~~~~~~~~~~~~~~~~

../src/databaseinterface.cpp:220:15: warning: ‘QSqlQuery DatabaseInterfacePrivate::mUpdateRadioQuery’ [-Wreorder]

QSqlQuery mUpdateRadioQuery;
          ^~~~~~~~~~~~~~~~~

../src/databaseinterface.cpp:47:5: warning: when initialized here [-Wreorder]

DatabaseInterfacePrivate(const QSqlDatabase &tracksDatabase)
^~~~~~~~~~~~~~~~~~~~~~~~
2844

Are you sure this column is needed ?
The tracks have a Priority columns in order to deal with duplicated tracks from different files (i.e. same metadata but different files).
You can keep it anyway but I am curious.

src/databaseinterface.h
392

Please remove this unrelated change.

434–436

Why are they needed ?
The database should just signal that a radio has been modified or removed instead of looking like a special handling of some UI.

452

Is update also dealing with insert ? If it is the case, please follow the existing convention of using insert for insert or modify.

You can always change all of them in a later diff.

src/models/datamodel.cpp
37

It would better (safer) if you add one more type like DataModel::ListRadioDataType. Online radios are similar to tracks but by doing that, the compiler will help avoid mistakes when connecting signals to slots.

137–140

What is the reason for that ?
It does not look good.

176–179

No need for that. If the data is missing, the view should handle that (i.e. an empty/invalid QVariant). This is the same in most places where fallback is an empty/invalid QVariant when the data is missing or unavailable.

403–417

This is too much specific to one usecase of DataModel.
Why do you need this when other views do not need it ?

src/models/trackmetadatamodel.cpp
402–410

I have a general question about the need for a special handling of radio. I do not get what is so special that we would need a complete code duplication here.

422–423

This is probably needed to get file tracks to work ?

src/models/trackmetadatamodel.h
150

Instead of using a custom edit method, please override the existing generic setData method from the base class.
You will get a future safe interface that would be extensible to support editing non radio tracks.

src/qml/ContentView.qml
110

This is too specific.
Please add or modify existing fields to have generic properties that could be usable in other contexts.

For example, I am working again on UPnP/DLNA support and would probably need some special handling for tracks where the file is an http URL instead of a local file. This is a shared need with online radios.

src/qml/DataListView.qml
39

I agree about isStream being a better fit. We need to have something generic and reusable.

172–173

In this case, you could name it createButton and the property would be viewHeader.createButtonActive . What do you think ?

src/qml/ListBrowserDelegate.qml
206

Tracks can have negative numbers I believe @astippich can you confirm ?

src/qml/MediaTrackMetadataView.qml
90–91

Web radios cannot have an image ?

src/viewmanager.h
91–93

Please do not use a bool but a named constant (from an enum).

And yes, there is a bool before your modification and that is an error (from me).

See https://ariya.io/2011/08/hall-of-api-shame-boolean-trap for a good explanation.

This revision now requires changes to proceed.Jul 28 2019, 5:17 PM
astippich added inline comments.Jul 29 2019, 3:23 PM
src/qml/ListBrowserDelegate.qml
206

No, tracks can only be positive, but zero is actually valid. So the correct check here is >= 0

Thank you a lot for your advice.

I actually would like to get it merged rather sooner than later as this revision touches various places and is probably a pain to rebase.
We could merge as a single big commit if you promise to stay around a little while in case some things need changing afterwards.

I agree, the later the revision is, the more time consuming is a rebase.
I will try to get most of the job done during this week, and I will be around until the end of August :).

Thanks for your big work. This will be a really nice contribution.

Thank you :)

I think that the database part is nice and could be already integrated.

Do you think we should propose a list of radio streams by default? I am not sure the one I set are the best ever :) .

I tried to answer some of your comments before going onto the changes:

src/databaseinterface.cpp
2844

For now it is not needed, when I implemented the request, I was not sure about the priority purpose nor about what to keep. Given the role it makes more sense to remove it.

src/databaseinterface.h
434–436

updateUIAfterRadioDeleted has the same role as trackRemoved for tracks.
updateUIAfterRadioInsertOrUpdate has the same role as trackModified also for tracks.

I think I should name them radioRemoved and radioModified.

src/models/datamodel.cpp
403–417

I just copied trackIndexFromId, in the first case we get mAllTrackData[result] and mAllRadiosData[result] in the other. I will integrate both methods.

src/models/trackmetadatamodel.cpp
402–410

I experienced an issue with the meta view if I returned some data in the other roles not concerned by the radio (TitleRole, ResourceRole, CommentRole). I can take a look at the metaView to fix the issue on the UI level.

src/qml/MediaTrackMetadataView.qml
90–91

At first, I disabled the feature for radio to focus only on Title, Address and Comment (comment is not yet used I admit).
It should be quite easy to add the support. Can we do it once the rest of the review is cleaner?

src/viewmanager.h
91–93

Thank you for the reading,

I am a little bit confused about the "best" solution here. "bool radioCase" is heavily linked to dataType (ElisaUtils::PlayListEntryType::Radio).

Should I expose the enum in qml and do the check in DataListView.qml with something like modelType == ElisaUtils::PlayListEntryType::Radio instead of the property "radioCase"?

Thank you a lot for your advice.

I actually would like to get it merged rather sooner than later as this revision touches various places and is probably a pain to rebase.
We could merge as a single big commit if you promise to stay around a little while in case some things need changing afterwards.

I agree, the later the revision is, the more time consuming is a rebase.
I will try to get most of the job done during this week, and I will be around until the end of August :).

You could (if it is more convenient for you) open another diff with just the database stuff. That could land and after that we could ensure the remaining parts land in master branch.

Thanks for your big work. This will be a really nice contribution.

Thank you :)

I think that the database part is nice and could be already integrated.

Do you think we should propose a list of radio streams by default? I am not sure the one I set are the best ever :) .

I believe that the best approach would be to allow easy access to a list of user contributed radios (i.e. maybe via store.kde.org ). When testing, I have added one radio I often listen to and that was easy to do. We could easily delay that to a second stage. What do you think ?

I tried to answer some of your comments before going onto the changes:

I will now answer to your questions.

Thanks

src/models/trackmetadatamodel.cpp
402–410

What was the issue ? I can maybe provide some help here.

src/qml/ListBrowserDelegate.qml
206

Thanks, I was not sure if special/hidden tracks would have a negative number.

src/qml/MediaTrackMetadataView.qml
90–91

Yes sure. Let's not delay that for a picture.

src/viewmanager.h
91–93

You can simply use a two values enum like IsRadio and IsTrack . That would be easier to read than a true/false . You will have to expose it like ViewManager::AlbumViewStyle is.

jguidon updated this revision to Diff 63577.Aug 11 2019, 8:29 PM
  • Fixed code style, warnings in databaseinterface
  • Change names of signals/slots
  • datamodel.cpp: Integrate trackIndexFromId and radioIndexFromId
  • Split MetaDataDelegate.qml into EditableMetaDataDelegate.qml for editable fields
  • ListBrowserDelegate: fix check on track number
  • databaseinterface.h: rename updateRadioInDatabase to insertRadio. The method handles insert and update.
  • databaseinterface: remove unrelated change
  • MediaTrackMetadataView: rename radio bool property to isRadio
  • datamodel: add one more type
  • Missed qml declaration of ListRadioDataType
  • DataModel::data: make a better check in case of radio data type
  • DataModel::data: remove case for DurationRole/Radio.
  • Added support for persistent state
  • ViewManager::openListView fix wrong bool parameter
jguidon updated this revision to Diff 63671.Aug 13 2019, 4:04 PM
  • Fixed remaining if code style
  • Rebase on master
jguidon marked 41 inline comments as done.Aug 13 2019, 4:18 PM
jguidon added inline comments.
src/models/datamodel.cpp
137–140

The dataCount did not make sense for radios, I reintegrated the check and added a different dataCount on line 128.

I believe that the best approach would be to allow easy access to a list of user contributed radios (i.e. maybe via store.kde.org ). When testing, I have added one radio I often listen to and that was easy to do. We could easily delay that to a second stage. What do you think ?

I also think it would be a very nice improvement in the future, for now we should focus on the main functionality. Also, we could think about the possibility to classify radios by genre?

I will try to open a new diff with only the database stuff no later than this weekend.

src/models/trackmetadatamodel.cpp
402–410

I get some other fields which are not very revelant for radios editing and viewing details, for instance Album (its value, "Radios", is used for the display in the playlist view).
Here is a screenshot with the original data() implementation:

astippich accepted this revision.Aug 17 2019, 1:46 PM

From my point of view, this is mergeable. The trackmetadata stuff is not ideal yet, but this is something we can do later. We have to revisit this anyways when Elisa gets metadata editing.

mgallien accepted this revision.Sun, Aug 18, 7:53 PM

I believe that the best approach would be to allow easy access to a list of user contributed radios (i.e. maybe via store.kde.org ). When testing, I have added one radio I often listen to and that was easy to do. We could easily delay that to a second stage. What do you think ?

I also think it would be a very nice improvement in the future, for now we should focus on the main functionality. Also, we could think about the possibility to classify radios by genre?

I will try to open a new diff with only the database stuff no later than this weekend.

We will need to fix this in a later patch.

Let's land your work. It will be a nice new feature for the Elisa project.

Should I land this patch for you or you have already a KDE developer account ?

This revision is now accepted and ready to land.Sun, Aug 18, 7:53 PM

Should I land this patch for you or you have already a KDE developer account ?

As this is my second contribution to a KDE project, I do not have a KDE developer account to be able to land a patch. So it would be very kind if you can land it.

Should I land this patch for you or you have already a KDE developer account ?

As this is my second contribution to a KDE project, I do not have a KDE developer account to be able to land a patch. So it would be very kind if you can land it.

@ngraham As far as I am concerned, I believe @jguidon would qualify as a KDE developer. In my mind, his two contributions would be enough. Is this correct ?

@jguidon Do you intend to continue to contribute to Elisa ?

This revision was automatically updated to reflect the committed changes.

@ngraham As far as I am concerned, I believe @jguidon would qualify as a KDE developer. In my mind, his two contributions would be enough. Is this correct ?

Two is low, but they are very high-quality contributions. Another criterion is commitment and intention to continue contributing. So an answer to this question would be welcome:

@jguidon Do you intend to continue to contribute to Elisa ?

Also, thanks a ton for this very nice feature!

@ngraham As far as I am concerned, I believe @jguidon would qualify as a KDE developer. In my mind, his two contributions would be enough. Is this correct ?

Two is low, but they are very high-quality contributions. Another criterion is commitment and intention to continue contributing. So an answer to this question would be welcome:

In fact,

@jguidon Do you intend to continue to contribute to Elisa ?

Also, thanks a ton for this very nice feature!

@ngraham As far as I am concerned, I believe @jguidon would qualify as a KDE developer. In my mind, his two contributions would be enough. Is this correct ?

Two is low, but they are very high-quality contributions. Another criterion is commitment and intention to continue contributing. So an answer to this question would be welcome:

I agree and will wait to see some commitment to finalize this feature before.

@jguidon Do you intend to continue to contribute to Elisa ?

Also, thanks a ton for this very nice feature!

I second that. This feature will be a great highlight of the next release.

@jguidon Do you intend to continue to contribute to Elisa ?

Yes, I enjoy working on the project. This is my first experience in an open source community project and I really like it :)

I will continue working on the project on a regular way, particularly on fixing the radio features which also have a nice potential.

Also, thanks a ton for this very nice feature!

Thanks :)

I second that. This feature will be a great highlight of the next release.

I was wondering: how are releases defined ?
Is there a defined deadline for a release (like ubuntu) or does the next version is released when it is stable enough (like mint for example) ?

All releases are time-based, not stability-based. You can find out about KDE's release schedules here: https://community.kde.org/Schedules/

Elisa is currently self-released but will be moving to the KDE Applications bundle and release schedule. So the next major release will be 19.12 which, like its numbers indicate, should be released around December of 2019.