Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#341 closed patch (fixed)

Music database update does not reread metadata

Reported by: eskil <myth@…> Owned by: Isaac Richards
Priority: minor Milestone: 0.19
Component: mythmusic Version: head
Severity: low Keywords:
Cc: Ticket locked: no

Description

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

Attachments (10)

dbupdate.patch (16.2 KB) - added by eskil <myth@…> 14 years ago.
patch that fixes the problem
dbupdate5.patch (34.3 KB) - added by myth@… 14 years ago.
updated version of the patch
dbupdate9.patch (38.4 KB) - added by eskil <myth@…> 14 years ago.
fixed patch
lcd-busy-520.patch (21.6 KB) - added by eskil <myth@…> 14 years ago.
busy spinner for LCD, plus fix for ticket 520
busy.patch (7.2 KB) - added by eskil <myth@…> 14 years ago.
decoder-refactor.patch (14.1 KB) - added by eskil <myth@…> 14 years ago.
refactoring of Decoder and subclasses
dbupdate-short.patch (9.4 KB) - added by eskil <myth@…> 14 years ago.
check file timestamps against db when updating
dbupdate-short2.patch (9.4 KB) - added by eskil <myth@…> 14 years ago.
replacement for dbupdate-short.patch
decoder-refactor.2.patch (15.1 KB) - added by eskil <myth@…> 14 years ago.
fixed for style
dbupdate-short.2.patch (9.3 KB) - added by eskil <myth@…> 14 years ago.
fixed for style

Download all attachments as: .zip

Change History (31)

Changed 14 years ago by eskil <myth@…>

Attachment: dbupdate.patch added

patch that fixes the problem

comment:1 Changed 14 years ago by 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.

Changed 14 years ago by myth@…

Attachment: dbupdate5.patch added

updated version of the patch

comment:2 Changed 14 years ago by 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.
  • 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.

comment:3 Changed 14 years ago by anonymous

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

Changed 14 years ago by eskil <myth@…>

Attachment: dbupdate9.patch added

fixed patch

comment:4 Changed 14 years ago by 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.

comment:5 Changed 14 years ago by danielk

Milestone: 0.19
Severity: mediumlow
Version: head

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.

comment:6 Changed 14 years ago by eskil <myth@…>

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

Changed 14 years ago by eskil <myth@…>

Attachment: lcd-busy-520.patch added

busy spinner for LCD, plus fix for ticket 520

Changed 14 years ago by eskil <myth@…>

Attachment: busy.patch added

Changed 14 years ago by eskil <myth@…>

Attachment: decoder-refactor.patch added

refactoring of Decoder and subclasses

Changed 14 years ago by eskil <myth@…>

Attachment: dbupdate-short.patch added

check file timestamps against db when updating

comment:7 Changed 14 years ago by 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.

Changed 14 years ago by eskil <myth@…>

Attachment: dbupdate-short2.patch added

replacement for dbupdate-short.patch

comment:8 Changed 14 years ago by 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.

comment:9 Changed 14 years ago by 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

comment:10 Changed 14 years ago by 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 ?

comment:11 Changed 14 years ago by 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.

comment:12 Changed 14 years ago by eskil <myth@…>

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

comment:13 Changed 14 years ago by 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.

comment:14 Changed 14 years ago by danielk

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

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

comment:15 Changed 14 years ago by 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.

comment:16 Changed 14 years ago by 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.

comment:17 Changed 14 years ago by 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.

Changed 14 years ago by eskil <myth@…>

Attachment: decoder-refactor.2.patch added

fixed for style

comment:18 Changed 14 years ago by 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.

Changed 14 years ago by eskil <myth@…>

Attachment: dbupdate-short.2.patch added

fixed for style

comment:19 Changed 14 years ago by danielk

(In [7611]) References #341.

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

comment:20 Changed 14 years ago by danielk

Resolution: fixed
Status: newclosed

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

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

comment:21 Changed 14 years ago by 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.

Note: See TracTickets for help on using tickets.