Opened 19 years ago
Closed 18 years ago
Last modified 18 years ago
#341 closed patch (fixed)
Music database update does not reread metadata
Reported by: | 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)
Change History (31)
Changed 19 years ago by
Attachment: | dbupdate.patch added |
---|
comment:1 Changed 19 years ago by
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.
comment:2 Changed 19 years ago by
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.
comment:3 Changed 19 years ago by
Ignore that last update, that patch is bad (the busy indicator is shit), working a better one.
comment:4 Changed 18 years ago by
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 18 years ago by
Milestone: | → 0.19 |
---|---|
Severity: | medium → low |
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.
Changed 18 years ago by
Attachment: | lcd-busy-520.patch added |
---|
busy spinner for LCD, plus fix for ticket 520
Changed 18 years ago by
Attachment: | busy.patch added |
---|
Changed 18 years ago by
Attachment: | decoder-refactor.patch added |
---|
refactoring of Decoder and subclasses
Changed 18 years ago by
Attachment: | dbupdate-short.patch added |
---|
check file timestamps against db when updating
comment:7 Changed 18 years ago by
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 18 years ago by
Attachment: | dbupdate-short2.patch added |
---|
replacement for dbupdate-short.patch
comment:8 Changed 18 years ago by
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 18 years ago by
(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 18 years ago by
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 18 years ago by
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:13 Changed 18 years ago by
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 18 years ago by
(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.
comment:15 Changed 18 years ago by
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 18 years ago by
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 18 years ago by
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.
comment:18 Changed 18 years ago by
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.
comment:19 Changed 18 years ago by
comment:20 Changed 18 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:21 Changed 18 years ago by
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.
patch that fixes the problem