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.
mgallien | |
astippich |
Elisa | |
VDG |
Add support for radio streams
Steps of development:
after the first reviews, add test for the radios view
No Linters Available |
No Unit Test Coverage |
Buildable 12321 | |
Build 12339: arc lint + arc unit |
src/modeldataloader.h | ||
---|---|---|
67 | 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:
Should it be better if I try to integrate them or at least give them some better names? | |
src/models/trackmetadatamodel.cpp | ||
341 ↗ | (On Diff #62357) | 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
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 | ||
---|---|---|
392 | this can be deleted | |
src/audiowrapper_libvlc.cpp | ||
234–242 | code style | |
237 | code style | |
567 | code style | |
src/databaseinterface.h | ||
425 | can you make them consistently with the other, e.g. radioRemoved(), radiosAdded()? | |
src/elisaqmlplugin.cpp | ||
172 ↗ | (On Diff #62677) | is this one actually needed? |
src/manageaudioplayer.cpp | ||
417 | code style | |
src/mediaplaylist.cpp | ||
596 | code style | |
941 | code style | |
src/modeldataloader.h | ||
67 | If you rename it now to radioModified I think it is pretty consistent with the rest | |
118 | can these be renamed for consistency with others? | |
src/models/datamodel.cpp | ||
177 | use QString instead of QStringLiteral(""), it is a little bit cheaper | |
src/models/datamodel.h | ||
171 | just call it radioModified for consistency with the others | |
src/models/trackmetadatamodel.cpp | ||
341 ↗ | (On Diff #62357) | Yeah, much better! I think your changes makes it even possible to provide some groundwork for future metadata editing for the tracks. |
177 ↗ | (On Diff #62677) | Not needed anymore for the general case |
560 ↗ | (On Diff #62677) | this belongs also into the "else" block below, doesn't it? |
562 ↗ | (On Diff #62677) | code style |
573 ↗ | (On Diff #62677) | code style |
src/qml/DataListView.qml | ||
38 ↗ | (On Diff #62357) | i think "isRadio", or actually "isStream" is better fitting here |
src/qml/MediaTrackMetadataView.qml | ||
32 ↗ | (On Diff #62677) | call this isRadio/isStream for consistency and make it an alias of realModel.isRadio |
51 ↗ | (On Diff #62677) | remove comment :) |
src/qml/MetaDataDelegate.qml | ||
31 ↗ | (On Diff #62677) | codeStyle |
49 ↗ | (On Diff #62677) | codeStyle |
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&)’: 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) ^~~~~~~~~~~~~~~~~~~~~~~~ | |
2568 | Are you sure this column is needed ? | |
src/databaseinterface.h | ||
382 | Please remove this unrelated change. | |
423–425 | Why are they needed ? | |
435 | 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. | |
134–137 | What is the reason for that ? | |
174–177 | 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. | |
411–425 | This is too much specific to one usecase of DataModel. | |
src/models/trackmetadatamodel.cpp | ||
400–408 ↗ | (On Diff #62677) | 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. |
420–421 ↗ | (On Diff #62677) | This is probably needed to get file tracks to work ? |
src/models/trackmetadatamodel.h | ||
150 ↗ | (On Diff #62677) | Instead of using a custom edit method, please override the existing generic setData method from the base class. |
src/qml/ContentView.qml | ||
134 | This is too specific. 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 | ||
171–172 ↗ | (On Diff #62677) | In this case, you could name it createButton and the property would be viewHeader.createButtonActive . What do you think ? |
38 ↗ | (On Diff #62357) | I agree about isStream being a better fit. We need to have something generic and reusable. |
src/qml/ListBrowserDelegate.qml | ||
206 ↗ | (On Diff #62677) | Tracks can have negative numbers I believe @astippich can you confirm ? |
src/qml/MediaTrackMetadataView.qml | ||
91–92 ↗ | (On Diff #62677) | Web radios cannot have an image ? |
src/viewmanager.h | ||
63–70 | 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. |
src/qml/ListBrowserDelegate.qml | ||
---|---|---|
206 ↗ | (On Diff #62677) | 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 | ||
---|---|---|
2568 | 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 | ||
423–425 | updateUIAfterRadioDeleted has the same role as trackRemoved for tracks. I think I should name them radioRemoved and radioModified. | |
src/models/datamodel.cpp | ||
411–425 | 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 | ||
400–408 ↗ | (On Diff #62677) | 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 | ||
91–92 ↗ | (On Diff #62677) | At first, I disabled the feature for radio to focus only on Title, Address and Comment (comment is not yet used I admit). |
src/viewmanager.h | ||
63–70 | 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"? |
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 | ||
---|---|---|
400–408 ↗ | (On Diff #62677) | What was the issue ? I can maybe provide some help here. |
src/qml/ListBrowserDelegate.qml | ||
206 ↗ | (On Diff #62677) | Thanks, I was not sure if special/hidden tracks would have a negative number. |
src/qml/MediaTrackMetadataView.qml | ||
91–92 ↗ | (On Diff #62677) | Yes sure. Let's not delay that for a picture. |
src/viewmanager.h | ||
63–70 | 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. |
src/models/datamodel.cpp | ||
---|---|---|
134–137 | 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 | ||
---|---|---|
400–408 ↗ | (On Diff #62677) | 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). |
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.
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 ?
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.
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!
In fact,
@jguidon Do you intend to continue to contribute to Elisa ?
Also, thanks a ton for this very nice feature!
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.
Thank you for the information.
I created another diff last week about radios, D23859. Did anyone get some notification about it or did I forgot to put a flag on it ?
Your new review is fine but no one took time to review it. I would like to finish the push on Elisa published on Windows store and will review as soon as I can.
Sorry for the delay