|
That looks like the best place to patch to easily fix both issues. The only problem I see is that if you have multiple tracks selected it'll call updateSelection for each track (and the status bar is usually blank when multiple are selected, so it probably shouldn't be called at all). Something like this would work which uses the cached selected_objects variable so it's not too expensive (it might need testing): === modified file 'picard/ui/itemviews.py'
--- picard/ui/itemviews.py 2011-07-26 23:07:13 +0000
+++ picard/ui/itemviews.py 2011-08-13 15:50:24 +0000
@@ -684,6 +684,9 @@
item.setTextColor(i, color)
item.setBackgroundColor(i, get_match_color(similarity, self.palette().base().color()))
item.setData(1, QtCore.Qt.UserRole, QtCore.QVariant(track.metadata.length or 0))
+ selection = self.window.selected_objects
+ if len(selection) == 1 and track in selection:
+ self.window.updateSelection(selection)
if update_album:
self.update_album(track.album, update_tracks=False)
Yeah, I thought about that. I guess the advantage of the less efficient patch is that if the updateSelection code was changed to change the status bar to say (n tracks selected (average 87%)" (or a less foolish example) then the code would still work. Noticed when debugging that that code ends up getting triggered from a heck of a lot of different signals as it is though... And there is an if len(objects) == 1: check inside updateSelection as it is; it doesn't appear to do much else of significance/cost outside that block. Good point, I guess the alternative optimization would be: if multiple tracks are selected it should still call updateSelection, but it only needs to call it once. That'll be more difficult, and if it's too much effort the current patch is indeed fine (I always thought the statusbar felt empty with several things selected, so maybe showing something like "N selected (M unsaved)" or something is a worthwhile enhancement). Here's my attempt, it only updates the status bar once after (1) all files are saved or (2) the tag editor window is accepted. If it causes any issues let's use your simpler version === modified file 'picard/tagger.py'
--- picard/tagger.py 2011-07-26 05:29:31 +0000
+++ picard/tagger.py 2011-08-13 19:03:16 +0000
@@ -433,6 +433,10 @@
files = self.get_files_from_objects(objects, save=True)
for file in files:
file.save(self._file_saved, self.tagger.config.setting)
+ self.save_queue.put((
+ lambda: self.window.selected_objects,
+ lambda result: self.window.updateSelection(result),
+ QtCore.Qt.LowEventPriority))
def load_album(self, id, discid=None):
if id in self.albumids:
=== modified file 'picard/ui/tageditor.py'
--- picard/ui/tageditor.py 2011-08-09 20:05:18 +0000
+++ picard/ui/tageditor.py 2011-08-13 18:52:19 +0000
@@ -98,6 +98,8 @@
def accept(self):
self.save()
QtGui.QDialog.accept(self)
+ window = self.tagger.window
+ window.updateSelection(window.selected_objects)
def load(self):
all_tag_names = set()
Hmm, that looks like it could work OK, but I'm not sure about putting window.update stuff off in a separate thread like that. Again, I'm no QT/GUI expert, but isn't the current Picard code modelled around using signals for communication across threads of entity events requiring a GUI repaint/update? This would directly call update on the window in a separate QThread off the save queue, right? It would call updateSelection in a separate thread, which I'm not sure is a bad thing, but probably better to avoid the chance that it could run concurrently. So let's ignore that proposal. Personally I prefer the first snippet I posted which checks if len(selection) == 1, because it's several orders of magnitude faster on my machine with multiple tracks selected according to time.time(). If it's later decided we want to display something in the status bar or metadata boxes when multiple tracks are selected, I think we'd better move this somewhere else entirely, because update_track is called too many times. :/ (Either that, or try to investigate if update_track really needs to be called so often.) Currently this is a good fix though, which I'd like to have in 0.16. I also realized we need to add the same code to update_file, to fix the same issue after editing/saving files in a cluster. Yeah, i think your first snippet is best; although it's fair to say that were it easier to do re-testing, a lot of that code could probably do with some refactoring for clarity and efficiency. I have a feeling that the frequency of such calls is possibly related to the unresponsive nature of the GUI on some hosts when you dump a particularly large collection on it. But just gut feel; nothing scientific. I have a patch which refactors the itemviews.py code to make it easier to read/debug, but it won't be merged until after 0.16. It's definitely something I'm looking into for 1.0 though. Committed slightly modified version of voiceinsideyou's fix in http://git.musicbrainz.org/gitweb/?p=picard.git;a=commit;h=ae2c171a9ba2a336d4a5ca2775495cde1afe52c2 |
||||||||||||||||||||||||||||||||||||||||||||||||||||
This also fixes http://bugs.musicbrainz.org/ticket/3196