Issue Details (XML | Word | Printable)

Key: PICARD-14
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Normal Normal
Assignee: Michael Wiencek
Reporter: voiceinsideyou
Votes: 1
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
Picard

File path and match % not updated in status bar when saving a single file or updating new metadata

Created: 13/Aug/11 11:56 AM   Updated: 31/Oct/11 05:13 AM   Resolved: 22/Oct/11 05:47 PM
Component/s: File Move & Rename, Tags & Metadata, User Interface
Affects Version/s: 0.15.1
Fix Version/s: 0.16

File Attachments: 1. Text File PICARD-14.patch (0.6 kB) 13/Aug/11 11:56 AM - voiceinsideyou

Issue Links:
Relates
 


 Description  « Hide

If you have a single matched file/track selected in right-hand pane and then save it such that it gets renamed or moved, the status bar is not updated with the new path and match %.

Additionally, when you manually change "new metadata" values at the bottom of the pane, it does not adjust the match % until you click to another entry and then click back to the file. Ideally I guess it should adjust the % as you type, just as it does for the "similarity bar" icon.

I think the problem is that AlbumTreeView.update_track does not invoke MainWindow.updateSelection() which has the status-bar-changing logic

Suggested patch below - but I'm no QT nor GUI expert, so I'm not sure about best approach here. I'm not sure if this will cause too many updates... or possibly whether it should be done after the update_album call?

Index: picard/ui/itemviews.py
===================================================================
--- picard/ui/itemviews.py	(revision revno:1173)
+++ picard/ui/itemviews.py	(revision )
@@ -684,6 +684,8 @@
             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))
+        if item.isSelected():
+            self.window.updateSelection(self.panel.selected_objects())
         if update_album:
             self.update_album(track.album, update_tracks=False)
 


Sort Order: Ascending order - Click to sort in descending order
voiceinsideyou added a comment - 13/Aug/11 02:31 PM

Michael Wiencek added a comment - 13/Aug/11 04:03 PM

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)

voiceinsideyou added a comment - 13/Aug/11 05:24 PM

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.


Michael Wiencek added a comment - 13/Aug/11 06:02 PM

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).


Michael Wiencek added a comment - 13/Aug/11 07:05 PM

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()

voiceinsideyou added a comment - 16/Aug/11 01:48 PM

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?


Michael Wiencek added a comment - 22/Oct/11 03:43 AM

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.


voiceinsideyou added a comment - 22/Oct/11 06:41 AM

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.


Michael Wiencek added a comment - 22/Oct/11 05:47 PM

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