Show tracks with empty titles
Needs RevisionPublic

Authored by astippich on Aug 24 2019, 3:17 PM.

Details

Reviewers
mgallien
ngraham
Summary

Tracks with empty title were completely
ignored before. Allow them to be inserted
into the database and display them

Diff Detail

Repository
R255 Elisa
Branch
empty_title
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16215
Build 16233: arc lint + arc unit
astippich requested review of this revision.Aug 24 2019, 3:17 PM
astippich created this revision.

Note that there are two issues with this patch:

  1. Loading the all tracks view seems to take longer
  2. Restoring of the playlist for tracks with empty title does not work

I would like to fix 1. before landing this patch. @mgallien any ideas? something with sql?
I would like to defer 2. as there are also issues with tracks having empty albums etc.

Thanks for your work.

Note that there are two issues with this patch:

  1. Loading the all tracks view seems to take longer
  2. Restoring of the playlist for tracks with empty title does not work

    I would like to fix 1. before landing this patch. @mgallien any ideas? something with sql? I would like to defer 2. as there are also issues with tracks having empty albums etc.

This sounds a good plan for me.

With respect to the slowness, please have a look at my inline comment and let me know if it works.

src/databaseinterface.h
90

Management of this new role is increasing the size of data exported by database to the model and possibly read by the view. This can be a real cause of slowdown in particular if the filename text can be quite long.

You can handle the filename call in the model and only when it is strictly needed to display a track without title (if I understand correctly). That may be enough to keep the same performance level but I cannot be sure.

It would be probably wiser to reduce the size of the data instead of increasing them.

astippich updated this revision to Diff 65581.Sep 7 2019, 1:51 PM
  • deduce filename on demand
astippich added inline comments.Sep 7 2019, 1:52 PM
src/databaseinterface.h
90

Unfortunately, it seems that this does not have a noticable effect. Any other ideas?

I will try to provide a good review as soon as I can. Sorry for the delay.

src/databaseinterface.h
90

Did you try to see if it is database requests that are slow by activating the database logging (especially about slow requests) ?
You could try tuning the threshold and compare with master branch. I added that after I forgot to put some indexes during a database migration.

astippich added inline comments.Sep 14 2019, 12:55 PM
src/databaseinterface.h
90

Stupid question: How do I activate the database logging?

mgallien added inline comments.Sep 15 2019, 7:36 AM
src/databaseinterface.h
90

Sorry for the lack of info.
I meant activating the categorized logging of database and possibly tuning the code in DatabaseInterface::execQuery (the computation of query execution time is only done in debug builds).

astippich added inline comments.Mon, Oct 7, 6:15 PM
src/databaseinterface.cpp
3875

I could not get the current code to output some execution time values, so I've added some QElapsedTimers, which are probably not that accurate, but show the problem anyways. I've tracked it down to the actual sql query . Adding "tracks.Title IS NULL OR ..." here increases the runtime of this query from approximately 150ms to 850 ms. Any ideas?

mgallien added inline comments.Mon, Oct 7, 7:48 PM
src/databaseinterface.cpp
3875

Not yet but I will spent time this week on that. I cannot promise the exact day but before Friday should be possible.

mgallien added inline comments.Fri, Oct 11, 3:39 PM
src/databaseinterface.cpp
3875

This should do the job while keeping reasonable runtime duration:

((tracks.Title IS NULL AND tracks.FileName = tracks2.FileName) OR tracks.Title = tracks2.Title) AND

mgallien requested changes to this revision.Fri, Oct 11, 3:42 PM

I will continue to look at this.

src/databaseinterface.cpp
3170

You are upgrading the v15 schema.
By the way, on my tests, the upgrade is too long and that is a blocker for integration of this work.

This revision now requires changes to proceed.Fri, Oct 11, 3:42 PM
astippich added inline comments.Sat, Oct 12, 6:55 PM
src/databaseinterface.cpp
3170

Well, that is kind of expected with a large collection as the tracks table is huge. I have no idea how to prevent this besides forcing a complete reindex

I spent some time on this. It should be possible to at least cover the no metadata case with requests only on TracksData. What do you think?

If you want, I can try to do that on gitlab?

It should be easier to extend it to the full solution by working together on it and using your work as a basis.

mgallien added inline comments.Wed, Oct 16, 8:58 PM
src/databaseinterface.cpp
3170

I have to do some research to find why it is faster to rebuild the database from scratch rather than the update.

I spent some time on this. It should be possible to at least cover the no metadata case with requests only on TracksData. What do you think?

If you want, I can try to do that on gitlab?

It should be easier to extend it to the full solution by working together on it and using your work as a basis.

If you have some ideas, please go ahead! I will abandon this one then