Differential D21525 Diff 60539 src/models/trackmetadatamodel.cpp

Changeset View

Standalone View

View Options

src/models/trackmetadatamodel.cpp

  • This file was copied to src/models/radiometadatamodel.cpp.
Show First 20 Lines • Show All 111 Lines • ▼ Show 20 Line(s)111 case DatabaseInterface::LastPlayDate:
112 result = i18nc("Last play date label for track metadata view", "Last played"); 112​ result = i18nc("Last play date label for track metadata view", "Last played");
113 break; 113​ break;
114 case DatabaseInterface::PlayCounter: 114​ case DatabaseInterface::PlayCounter:
115 result = i18nc("Play counter label for track metadata view", "Play count"); 115​ result = i18nc("Play counter label for track metadata view", "Play count");
116 break; 116​ break;
117 case DatabaseInterface::LyricsRole: 117​ case DatabaseInterface::LyricsRole:
118 result = i18nc("Lyrics label for track metadata view", "Lyrics"); 118​ result = i18nc("Lyrics label for track metadata view", "Lyrics");
119 break; 119​ break;
120​ case DatabaseInterface::HttpAddressRole:
121​ result = i18nc("Radio HTTP address for radio metadata view", "HttpAddress");
122​ break;
astippichUnsubmitted
Not Done

Not needed anymore for the general case

astippich: Not needed anymore for the general case
120 case DatabaseInterface::SecondaryTextRole: 123​ case DatabaseInterface::SecondaryTextRole:
121 case DatabaseInterface::ImageUrlRole: 124​ case DatabaseInterface::ImageUrlRole:
122 case DatabaseInterface::ShadowForImageRole: 125​ case DatabaseInterface::ShadowForImageRole:
123 case DatabaseInterface::ChildModelRole: 126​ case DatabaseInterface::ChildModelRole:
124 case DatabaseInterface::StringDurationRole: 127​ case DatabaseInterface::StringDurationRole:
125 case DatabaseInterface::MilliSecondsDurationRole: 128​ case DatabaseInterface::MilliSecondsDurationRole:
126 case DatabaseInterface::AllArtistsRole: 129​ case DatabaseInterface::AllArtistsRole:
127 case DatabaseInterface::HighestTrackRating: 130​ case DatabaseInterface::HighestTrackRating:
▲ Show 20 Lines • Show All 87 Lines • ▼ Show 20 Line(s)217 case DatabaseInterface::ElementTypeRole:
215 break; 218​ break;
216 } 219​ }
217 break; 220​ break;
218 } 221​ }
219 222​
220 return result; 223​ return result;
221} 224​}
222 225​
223bool TrackMetadataModel::setData(const QModelIndex &index, const QVariant &value, int role) 226​bool TrackMetadataModel::setData(const QModelIndex &index, const QVariant &value, int role)
224{ 227​{
225 if (data(index, role) != value) { 228​ if (data(index, role) != value) {
226 // FIXME: Implement me! 229​ // FIXME: Implement me!
227 emit dataChanged(index, index, QVector<int>() << role); 230​ emit dataChanged(index, index, QVector<int>() << role);
228 return true; 231​ return true;
229 } 232​ }
230 return false; 233​ return false;
231} 234​}
mgallienUnsubmitted
Not Done

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.

mgallien: I have a general question about the need for a special handling of radio. I do not get what is…
jguidonAuthorUnsubmitted
Not Done

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.

jguidon: I experienced an issue with the meta view if I returned some data in the other roles not…
mgallienUnsubmitted
Not Done

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

mgallien: What was the issue ? I can maybe provide some help here.
jguidonAuthorUnsubmitted
Not Done

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:

jguidon: I get some other fields which are not very revelant for radios editing and viewing details, for…
232 235​
233QHash<int, QByteArray> TrackMetadataModel::roleNames() const 236​QHash<int, QByteArray> TrackMetadataModel::roleNames() const
234{ 237​{
235 auto names = QAbstractListModel::roleNames(); 238​ auto names = QAbstractListModel::roleNames();
236 239​
237 names[ItemNameRole] = "name"; 240​ names[ItemNameRole] = "name";
238 names[ItemTypeRole] = "type"; 241​ names[ItemTypeRole] = "type";
239 242​
Show All 39 Lines276{
279 for (auto role : {DatabaseInterface::TitleRole, DatabaseInterface::ArtistRole, DatabaseInterface::AlbumRole, 282​ for (auto role : {DatabaseInterface::TitleRole, DatabaseInterface::ArtistRole, DatabaseInterface::AlbumRole,
280 DatabaseInterface::AlbumArtistRole, DatabaseInterface::TrackNumberRole, DatabaseInterface::DiscNumberRole, 283​ DatabaseInterface::AlbumArtistRole, DatabaseInterface::TrackNumberRole, DatabaseInterface::DiscNumberRole,
281 DatabaseInterface::RatingRole, DatabaseInterface::GenreRole, DatabaseInterface::LyricistRole, 284​ DatabaseInterface::RatingRole, DatabaseInterface::GenreRole, DatabaseInterface::LyricistRole,
282 DatabaseInterface::ComposerRole, DatabaseInterface::CommentRole, DatabaseInterface::YearRole, 285​ DatabaseInterface::ComposerRole, DatabaseInterface::CommentRole, DatabaseInterface::YearRole,
283 DatabaseInterface::LastPlayDate, DatabaseInterface::PlayCounter}) { 286​ DatabaseInterface::LastPlayDate, DatabaseInterface::PlayCounter}) {
284 if (trackData.constFind(role) != trackData.constEnd()) { 287​ if (trackData.constFind(role) != trackData.constEnd()) {
285 if (role == DatabaseInterface::RatingRole) { 288​ if (role == DatabaseInterface::RatingRole) {
286 if (trackData[role].toInt() == 0) { 289​ if (trackData[role].toInt() == 0) {
287 continue; 290​ continue;
astippichUnsubmitted
Done

I am not a fan of all these if(isRadio) {} else {} blocks. Is there a different way to achieve this?

astippich: I am not a fan of all these if(isRadio) {} else {} blocks. Is there a different way to achieve…
astippichUnsubmitted
Done

On a further thought, we may revisit how the metadata works later, but we can leave it as is right now here

astippich: On a further thought, we may revisit how the metadata works later, but we can leave it as is…
jguidonAuthorUnsubmitted
Done

Maybe it is more readable this way?

jguidon: Maybe it is more readable this way?
astippichUnsubmitted
Not Done

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

astippich: Yeah, much better! I think your changes makes it even possible to provide some groundwork for…
288 } 291​ }
289 } 292​ }
290 293​
291 mTrackKeys.push_back(role); 294​ mTrackKeys.push_back(role);
292 mTrackData[role] = trackData[role]; 295​ mTrackData[role] = trackData[role];
293 } 296​ }
294 } 297​ }
295 filterDataFromTrackData(); 298​ filterDataFromTrackData();
▲ Show 20 Lines • Show All 67 Lines • ▼ Show 20 Line(s)356{
363 if (mManager) { 366​ if (mManager) {
364 mManager->connectModel(&mDataLoader); 367​ mManager->connectModel(&mDataLoader);
365 } 368​ }
366 369​
367 connect(this, &TrackMetadataModel::needDataByDatabaseId, 370​ connect(this, &TrackMetadataModel::needDataByDatabaseId,
368 &mDataLoader, &ModelDataLoader::loadDataByDatabaseId); 371​ &mDataLoader, &ModelDataLoader::loadDataByDatabaseId);
369 connect(this, &TrackMetadataModel::needDataByFileName, 372​ connect(this, &TrackMetadataModel::needDataByFileName,
370 &mDataLoader, &ModelDataLoader::loadDataByFileName); 373​ &mDataLoader, &ModelDataLoader::loadDataByFileName);
371 connect(&mDataLoader, &ModelDataLoader::allTrackData, 374​ connect(&mDataLoader, &ModelDataLoader::allTrackData,
372 this, &TrackMetadataModel::trackData); 375​ this, &TrackMetadataModel::trackData);
mgallienUnsubmitted
Not Done

This is probably needed to get file tracks to work ?

mgallien: This is probably needed to get file tracks to work ?
373 connect(&mDataLoader, &ModelDataLoader::trackModified, 376​ connect(&mDataLoader, &ModelDataLoader::trackModified,
374 this, &TrackMetadataModel::trackData); 377​ this, &TrackMetadataModel::trackData);
astippichUnsubmitted
Not Done

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

astippich: this belongs also into the "else" block below, doesn't it?
375} 378​}
376 379​
astippichUnsubmitted
Done

code style
if (....) {

astippich: code style if (....) {
377void TrackMetadataModel::fetchLyrics() 380​void TrackMetadataModel::fetchLyrics()
378{ 381​{
379 auto lyricicsValue = QtConcurrent::run(QThreadPool::globalInstance(), [=]() { 382​ auto lyricicsValue = QtConcurrent::run(QThreadPool::globalInstance(), [=]() {
380 auto trackData = mFileScanner.scanOneFile(mFullData[DatabaseInterface::ResourceRole].toUrl(), mMimeDatabase); 383​ auto trackData = mFileScanner.scanOneFile(mFullData[DatabaseInterface::ResourceRole].toUrl(), mMimeDatabase);
381 if (!trackData.lyrics().isEmpty()) { 384​ if (!trackData.lyrics().isEmpty()) {
382 return trackData.lyrics(); 385​ return trackData.lyrics();
383 } 386​ }
384 return QString{}; 387​ return QString{};
385 }); 388​ });
386 389​
387 mLyricsValueWatcher.setFuture(lyricicsValue); 390​ mLyricsValueWatcher.setFuture(lyricicsValue);
astippichUnsubmitted
Done

code style
} else {

astippich: code style } else {
388} 391​}
389 392​
390void TrackMetadataModel::initializeByTrackId(qulonglong databaseId) 393​void TrackMetadataModel::initializeByTrackId(qulonglong databaseId)
391{ 394​{
392 mFullData.clear(); 395​ mFullData.clear();
393 mTrackData.clear(); 396​ mTrackData.clear();
394 mCoverImage.clear(); 397​ mCoverImage.clear();
395 mFileUrl.clear(); 398​ mFileUrl.clear();
Show All 30 Lines