please review the sql changes carefully, as I haven't done this before.
Also, currently some tests fail here locally with and without this patch (with no changes), so please test

In D13685#282254, @mgallien wrote:

I have three main issues with this patch:

• automatic tests related to this change are missing. This is especially important considering this is a big and scary change. You will need to test that all other classes are able to cope with title being empty (for example) ;

I expected this request and will add some tests for the database if we decide this is the way to go.

• As is, the Sql is faulty (syntax error: SELECT tracks.ID, tracks.Title, artistAlbum.Name FROM Tracks tracks, LEFT JOIN Albums album ON album.ID = tracks.AlbumID LEFT JOIN AlbumsArtists artistAlbumMapping ON artistAlbumMapping.AlbumID = album.ID LEFT JOIN Artists artistAlbum ON artistAlbum.ID = artistAlbumMapping.ArtistID " QDEBUG : DatabaseInterfaceTests::addOneTrackWithoutAlbumArtist() DatabaseInterface::initRequest QSqlError("1", "Unable to execute statement", "no such table: LEFT") ;

Thanks, that got lost during my testing because the tests show failures here in any case, but was just a simple typo and should be fixed now.

• I am not convinced this is the right approach. I have the feeling that just deriving a title from the filename when there is no such metadata would fix most problems in a much simpler way.

I disagree here. While deriving a title from file name might work, it is simply guesswork and can be plaing wrong, and such was removed in D8007 for other cases. Also, once we add support for writing metadata, do we then automatically write the generated title (which may be completely wrong) to the file?
And what do we do if the album is also missing?
In addition, an empty title is not the main issue here, fixing this is quite easy. The required album name is much more of an issue.

In D13685#282359, @astippich wrote:

In D13685#282254, @mgallien wrote:

I have three main issues with this patch:

• automatic tests related to this change are missing. This is especially important considering this is a big and scary change. You will need to test that all other classes are able to cope with title being empty (for example) ;

I expected this request and will add some tests for the database if we decide this is the way to go.

I really fear that this change could cause regressions. You need to add test cases for this change in all classes already covered by automatic tests. Usually when doing this, you will probably encounter issues to fix (at least this was often the case for me).

• As is, the Sql is faulty (syntax error: SELECT tracks.ID, tracks.Title, artistAlbum.Name FROM Tracks tracks, LEFT JOIN Albums album ON album.ID = tracks.AlbumID LEFT JOIN AlbumsArtists artistAlbumMapping ON artistAlbumMapping.AlbumID = album.ID LEFT JOIN Artists artistAlbum ON artistAlbum.ID = artistAlbumMapping.ArtistID " QDEBUG : DatabaseInterfaceTests::addOneTrackWithoutAlbumArtist() DatabaseInterface::initRequest QSqlError("1", "Unable to execute statement", "no such table: LEFT") ;

Thanks, that got lost during my testing because the tests show failures here in any case, but was just a simple typo and should be fixed now.

It is fixed, thanks.

• I am not convinced this is the right approach. I have the feeling that just deriving a title from the filename when there is no such metadata would fix most problems in a much simpler way.

I disagree here. While deriving a title from file name might work, it is simply guesswork and can be plaing wrong, and such was removed in D8007 for other cases. Also, once we add support for writing metadata, do we then automatically write the generated title (which may be completely wrong) to the file?
And what do we do if the album is also missing?
In addition, an empty title is not the main issue here, fixing this is quite easy. The required album name is much more of an issue.

393118 is exactly about using the filename as a fallback when there is no title. That would also have the benefit of adding support for all kind of weird audio files that works in Qt Multimedia but have no metadata or no support in taglib.

As far as I can tell, that would be desirable for some people.

In D13685#282879, @mgallien wrote:

In D13685#282359, @astippich wrote:

In D13685#282254, @mgallien wrote:

I have three main issues with this patch:

• automatic tests related to this change are missing. This is especially important considering this is a big and scary change. You will need to test that all other classes are able to cope with title being empty (for example) ;

I expected this request and will add some tests for the database if we decide this is the way to go.

I really fear that this change could cause regressions. You need to add test cases for this change in all classes already covered by automatic tests. Usually when doing this, you will probably encounter issues to fix (at least this was often the case for me).

We should definitely not put this into 0.2. (But I would actually disable the file browser without it). Since the existing tests are passing, we at least do not regress behavior that was previously working.
And as I said, if we agree that this is worthwhile I will start to work on the test side of this patch.

393118 is exactly about using the filename as a fallback when there is no title. That would also have the benefit of adding support for all kind of weird audio files that works in Qt Multimedia but have no metadata or no support in taglib.

As far as I can tell, that would be desirable for some people.

To be more precise about what I meant, I think it is not a good idea to artificially create metadata based on the filename and store it in our database as title. All the models and the trackdatahelper already fall back to displaying the filename if the title is missing.

To be honest, I did tests to better understand the reasons behind your patch:

• a track without a title metadata will never get added to the tracks database ;
• a track without an album metadata will never get added to teh tracks database.

Sorry, that was obvious for myself, I'll try to express my motivation better in the future

We also have some code already dealing with empty titles for track. MediaPlayList class handles this case with the following code:

`

case ColumnsRoles::TitleRole:
  if (!d->mTrackData[index.row()].title().isEmpty()) {
      result = d->mTrackData[index.row()].title();
  } else {
      if (d->mData[index.row()].mTrackUrl.isLocalFile()) {
          result = d->mData[index.row()].mTrackUrl.fileName();
      } else {
          result = d->mData[index.row()].mTrackUrl.toString();
      }
  }
  break;

`
This code has to deal with that given the playlist can get tracks not within the tracks database (from a command line argument or from a loaded playlist).

We also have another two pieces of code doing something close but not quite. AlbumModel and AllTracksModel classes have the following code:

`

case ColumnsRoles::TitleRole:
    if (track.title().isEmpty()) {
        track.resourceURI().fileName();
    } else {
        result = track.title();
    }
    break;

`

This code is only dealing with tracks coming from the database. We should not really keep it there. We have two duplicate copy of it and it is not quite doing the right stuff (see the MediaPlayList version).

I am convinced that the better approach is to deal with missing metadata in the tracks import code of DatabaseInterface. This way, we have one central place where this case is handled. We avoid duplicating code and we avoid needing to make a required data be optional.

I'm still quite opposed for writing automatically created data to the database. When Elisa gets metadata writing support, it will not be possible to distinguish between data created by the user or by automatically by Elisa. But I see your point regarding the code. How about putting the code for handling empty titles into MusicAudioTrack, similar to the albumartist? That way, it will be located in one place and all models get this without extra code. Thinking about this more, this should probably have been the way chosen from the beginning. I would do this change in a separate revision if you agree with this approach.

The part of your patch handling the case of a missing album metadata is a good step in the right direction. Before accepting this part of the patch I really would like to see automatic tests for the models that will have to deal with this case. This is important to ensure we do not break anything.

Okay, I will try to add more tests there.

I've significantly reduced the scope of this revision by only allowing empty albums and track numbers in the database. After this, I would like to work on empty artists, and then finally empty titles

ping. we have multiple bug reports about this

@mgallien could you comment if this is acceptable or not?
I think right now is a good time to merge it into master. I expect there to be at least some issue with the GUI where we previously always expected a value, and it would give some time to sort all issues out before the next release.