Music database update does not reread metadata

[eskil <myth@…>]

As Robin Giks said ;

Greetings

Is it just me or...

I changed the ID3 tags in an album, rescanned and no changes. To get the changes to appear (after about 10 tries), I had to remove the album, scan so that it was removed completely and then put the directory back and rescan a second time.

Is this a known problem or do not many people use tags?

-- Robin Gilks

[eskil <myth@…>]

patch that fixes the problem

[eskil <myth@…>]

The attached patch fixes the behaviour.

Metadata writes date_added and date_modified, containing date and time.

I've changed the behaviour or Decoder::getMetadata slightly. All Decoders implement a ::readMetadata as well, the default implementation of ::getMetadata calls ::readMetadata, all the existing decoders have implementing a new ::readMetadata.

Additionally, ::getMetadata does *not* save the metadata to the db, seems like a very undesired sideeffect of a method called "getMetadata".

main.cpp is now responsible for calling Metadata::dumpToDatabase in CheckFile?, and it compares the file's mtime to the date_modified in the db.

[myth@…]

updated version of the patch

[Eskil <myth@…>]

Attached updated version of the patch, and here's the updated description of the changes;

• Metadata class writes the date_added and date_modified fields to the db.

• I've refactored Decoder::getMetadata. It now tries to find the metadata in the db, if there's no entry, it call ::readMetadata which does the actual read from the file. ::readMetadata call ::doCreateTagger which returns a MetaIO object. The various decoders just implement ::doCreateTagger to return the right MetaIO object, and they all share ::getMetadata, ::readMetadata and ::commitMetadata from the parent.

• Additionally, ::getMetadata does *not* save the metadata to the db, seems like a very undesired sideeffect of a method called "getMetadata". main.cpp is now responsible for calling Metadata::dumpToDatabase in CheckFile??, and it compares the file's mtime to the date_modified in the db.

• progress bars. I've added a MythBusyIndicator? 'widget' to mythdialogs.h/cpp. It's a QThread that displays a MythProgressDialog? and does a 'busy spinner' running in the thread. Also changed MythProgessDialog? slightly to act a 'busy spinner' - it just looks like shit.

• LCD fixes :
  • added a much needed flush...
  • changed generic screen to have TOP priority, since the only thing that uses it is progress meters that block the UI anyway. It was a lower priority back in the day since music used the generic screen to show it was playing (afaicr), but now there's a dedicated screen for music. If it's not top, menu redraws take precedence over a progress and that also looks like shit.
  • hacked the generic progress screen so it can show a busy spinner with the forementions MythBusyIndicator? is running, looks better than the QT one...

Tried to add doxygen comments for all the stuff I've added/methods I've touched.

[anonymous]

Ignore that last update, that patch is bad (the busy indicator is shit), working a better one.

[eskil <myth@…>]

fixed patch

[anonymous]

The fix to patch is to not have MythBusyIndicator? be a thread (guess calling qApp->processEvents) from a thread wasn't such a hot idea).

It's now just a widget that sets up a timer to spn the spinner. Changed the loading of metadata to call qApp->processEvents every now and then.

[danielk]

Can you break down this patch into easier to review chunks?

The LCD and progress bar changes could probably go in separate patches for instance.

[eskil <myth@…>]

Sure, I'll even toss in a fix for #520 as well...

[eskil <myth@…>]

busy spinner for LCD, plus fix for ticket 520

[eskil <myth@…>]

refactoring of Decoder and subclasses

[eskil <myth@…>]

check file timestamps against db when updating

[eskil <myth@…>]

I've split out the patch into four small patches, and cleaned them up a bit (ie. undid some not-that-nessecary variable renaming).

• lcd-busy-520.patch adds a busy spinner to the LCD and fixes ticket#520. See comment at the top of the patch for details.
• busy.patch, requires lcd-busy-520.patch, implements a MythBusyDialog? widget that displays a busy spinner. Also changes MythMusic to use this while searching for music files. Again, comment at then top of the patch.
• decoder-refactor.patch, moves common code from the Decoder subclasses up to the superclass, and uses a factory method to directly call Metadata::dumpToDatabase instead of relying on Decoder::getMetadata to do this as a sideeffect.
• dbupdate-short.patch, requires all three previous patches. When inserting/updating db, write date_modified and date_added. Subsequent scans compare filestamps against db and rereads metadata if nessecary.

[eskil <myth@…>]

replacement for dbupdate-short.patch

[eskil <myth@…>]

dbupdate-short2.patch replaces dbupdate-short.patch, fixes the progress bar during db upgrade.

Also notice that all this will cause a complete update of the musicmetadata db, since all entries will get their date_modified field written for the first time.

[danielk]

(In [7601]) Closes #520, by applying eskil's patch. References #341.

This needed a lot of formatting fixes. Eskil please read: ​http://www.mythtv.info/moin.cgi/CodingStandards

[eskil <myth@…>]

I did and I thought I had followed them. But I can see that I still left spaces after function names and didn't place opening {'s on a new line. Anything else I should clean up ?

[danielk]

Those were the biggies.

I know it sounds trivial and it is. But if I have to go in and make a trivial fix, it breaks the flow of just checking the changes in the diff.

[eskil <myth@…>]

No worries, I'll try and fix'em tonight.

[danielk]

Oh, also you can't just delete QObject derived objects, you need to disconnect() any signals you have going to them, and then call their deleteLater() method. The disconnect() ensures that another MythTV thread doesn't call a method in the class after its memory is freed, and the deleteLater() ensures that that the Qt event thread doesn't send them any events to it after its memory is freed.

Also, if you create any functions not taking a parameter please set declare their parameter list as (void), unless it is the destructor. This just makes it easier to grep for the deceleration and implementation of such a function.

PS I'm already working on merging the busy.patch, so don't bother with that one.

[danielk]

(In [7605]) References #341. Requires a rebuild of MythTV & plugins.

This is a cleaned up version of Eskil's busy patch.

• It adds a new dialog, MythBusyDialog?.
• It also makes some small but good changes to MythProgressDialog?, hence the rebuild requirement.
• It uses the busy dialog in MythMusic during the scan.

[eskil <myth@…>]

But what about all the other uses of MythProgressBar? ? The places I've seen it used, it's just deleted after it's Close is called, is that illegal as well ?

And I'll try and clean up decoder-refactor and dbupdate-short.

[eskil <myth@…>]

Hmmm, current svn crashes during DB scan, but if I replace the "busy->deleteLater ()" with "delete busy", then all is well.

My original path just had the MythBusyDialog? as a stack variable so it was destroyed when the scope was done, and that worked fine.

[danielk]

Yep, there are a lot of bad deletes of QObjects. Technically this is only a problem if signals or slots are in use. But, some normal QObject calls use signals in the background, like timers and the like.

If it is causing a crash, it is probably due to some signal/slot still being connected.

[eskil <myth@…>]

fixed for style

[eskil <myth@…>]

Attached new version of the decoder-refactor patch. I had to replace the

busy->deleteLater ();

with

delete busy;

to be able to run it on my system, I'm not sure how that deleteLater thing works, so feel free to undo that part if I'm completely wrong.

Also reset counter to 0 before the db update loop, otherwise the progressbar is at 100% all the time.

This patch doesn't add any functionality, it's purely a refactoring thing.

[eskil <myth@…>]

fixed for style

[danielk]

(In [7611]) References #341.

Simplifying refactor of Decoder and it's children by Eskil.

[danielk]

(In [7612]) Closes #341, by applying last patch.

This allows id3 changes to be picked up by a MythMusic scan.

[Isaac Richards]

Daniel, err, stuff's not right in your comments/code. 'deleteLater' is only useful for deleting a QObject from the non-Qt thread, or when there may be pending events delivered. It doesn't really do anything at all for signals/slots, so the 'deleteLater's that you've added here aren't doing anything useful at all.