PICARD-2025: Add file path to metadata view #2552
Conversation
There was a problem hiding this comment.
Aside from the pylint issue, the code looks fine to me.
(With the benefit of hindsight it might have been helpful to split the changes into two commits (one for the non-functional formatting changes, and one for the actual changes for this PR, but it was still possible to review this without these being separated.)
Before this gets merged and sets the metadata names in stone to preserve backwards compatibility, I do wonder whether the variables should be called ["path_old", "~path_saved"] rather than ["~old_path", "~new_path"].
Finally there is nothing wrong with this requirement, but these are hidden variables.
- I am not sure that this PR should have "metadata view" in the name if the variables are not viewable in the UI.
- A separate requirement (and so not for this PR) is IMO to have both the current file location / filename displayed in the old metadata and where it will be saved if you do a save (i.e. taking into account the Move and Rename options) in the new metadata (with the standard value-changed colouring and album icons applied).
picard/file.py
Outdated
| log.debug("Loading %r failed, retrying as %r", self, alternative_file) | ||
| if type(alternative_file) != type( | ||
| self | ||
| ): # pylint: disable=unidiomatic-typecheck # noqa: E721 |
There was a problem hiding this comment.
If you are going to split this code into separate lines, then you need to move the # pylint to the correct line.
There was a problem hiding this comment.
There are too many changes that are totally unrelated to the feature you want to implement.
Please do not change string quotes without a good reason to do it, read https://github.com/metabrainz/picard/blob/master/CONTRIBUTING.md#strings-quoting-single-or-double-quotes
Also, re-indentation, line wrapping, etc, should be done in a separate PR (if really needed).
zytact
force-pushed
the
path-metadata
branch
from
71c538d to
f7c9e92
Compare
|
Yes, that was a mistake on my part. I mistakenly turned on format on save and didn't notice. I apologize. I am new to this and seem to have somehow closed the pull request, should I properly do a new one? |
You can just push your commits on your |
|
The force push you just made removed every single change. Is that what you intended? |
|
Yes that was intended. I wanted to clear the git history. I just pushed the new changes. |
| @@ -367,6 +367,7 @@ def _preserve_times(self, filename, func): | |||
|
|
|||
| def _save_and_rename(self, old_filename, metadata): | |||
| """Save the metadata.""" | |||
| metadata["path_old"] = old_filename | |||
| if not (files or tracks): | ||
| return None | ||
| for file in files: | ||
| old_path = file.metadata.get('path_old', file.filename) |
| files = self.files | ||
| tracks = self.tracks | ||
| self.selection_mutex.unlock() | ||
| try: |
There was a problem hiding this comment.
Why have you introduced a try block here? What exceptions are you expecting?
There is no except statement - so what happens if there is an exception in the try section?
There was a problem hiding this comment.
Thank you for pointing that out, I forgot to add the except block but I simply couldn't get it to work on my machine without using a try block. I'll look into it properly.
There was a problem hiding this comment.
Presumably you were getting an exception without the try block and this is now hidden. This is really not a good idea.
However, please see later comments as you may want to hold off from spending any more time on this code until there is consensus on what is actually needed.
| if new_selection or drop_album_caches: | ||
| self._single_file_album = len({file.metadata['album'] for file in files}) == 1 |
There was a problem hiding this comment.
This seems to have been included in the try block in error since it still exists a few lines further down.
| file.orig_metadata['Path'] = old_path | ||
|
|
||
| new_path = file.metadata.get('~path_saved', file.filename) | ||
| file.metadata['Path'] = new_path |
There was a problem hiding this comment.
You have introduced a new metadata tag here and there are various issues with this:
- I think you should split this into path and filename so that we can compare the two separately - and so that they relate directly to the Move and Rename options.
- This is NOT a tag that should be saved to the file, so it should be
~pathand~filename.Pathmay be saved as a miscellaneous text tag in some formats that support this (e.g. ID3). - You will need to make sure that these metadata variables are recalculated if the user changes Move or Rename options.
- You will need to change the metadata table generation to add rows for
~pathand~filenameand ensure that they are used when comparing old and new metadata and when summarising metadata over several albums or tracks. - You will need to add definitions for Path and Filename metadata elsewhere so that the names get translated.
- You may need to write additional tests.
None of the above are major pieces of work, but they do need to be done.
|
Assuming this is eventually going to be accepted and merged, please add a ticket to update the Picard documentation to include the new tags/variables. When adding the new ticket, please reference the ticket that you're addressing in this PR so that they are linked. Thanks. |
| if error is None and result is not None: | ||
| new_filename = result | ||
| # Add new path info after successful rename | ||
| self.metadata["~path_saved"] = new_filename |
There was a problem hiding this comment.
I must be missing something because it appears that the "~path_saved" variable is being set after the file has been saved. Can you provide an example of how this would be used?
There was a problem hiding this comment.
Now that Bob has prompted me to think further, it seems to me that we need to know:
- The current path i.e. where the file actually is - displayed in Old metadata.
- The future path if we were to save the file now - displayed in New metadata.
- The previous path if we have saved the file since it has been loaded.
To make life easiest for scripters, once a file is loaded and before it is saved we need to decide whether the previous path is set to the current path or left empty / non-existent.
And we should decide whether to save the entire path (directory path and filename as a string) or the directory path and the filename separately or possibly both. (Scripters may want directory and filename separate or they may want it together. UI needs them separate.)
Motivation: We should try to get this exactly right from the start before we create a backwards compatibility problem for scripters.
There was a problem hiding this comment.
3. The previous path if we have saved the file since it has been loaded.
Do we need this? When the file is saved, I think the file variables are automatically updated to the new path and filename, so I'm not sure what the use case would be for the original path and filename.
And we should decide whether to save the entire path (directory path and filename as a string) or the directory path and the filename separately or possibly both.
My suggestion is to keep the path and filename as separate variables for the maximum flexibility of use. In Picard's scripting, it's easier to combine the two than split them out from a combined value.
Another consideration is how path separators are displayed and saved. The current file naming script preview displays the drive and path as used for the file system (using a colon and backslashes for windows paths). I think this is fine (and appropriate) for the display in the table in the Metadata pane. It's probably okay for the values stored in the variables as well, but we may need to be careful because of the use of the backslashes in paths on windows systems. I assume they would need to be escaped for the tags/variables, although I really don't know the details of how they are saved in the file metadata (if the user chooses to copy the variable to a tag so it is saved).
Motivation: We should try to get this exactly right from the start before we create a backwards compatibility problem for scripters.
I agree, although I don't think we're actually changing any of the existing tags/variables so we shouldn't have any backwards compatibility issues. [Famous last words... 😜]
There was a problem hiding this comment.
- The previous path if we have saved the file since it has been loaded.
Do we need this? When the file is saved, I think the file variables are automatically updated to the new path and filename, so I'm not sure what the use case would be for the original path and filename.
I don't personally have a use for this, however this was the initial purpose of this PR.
And we should decide whether to save the entire path (directory path and filename as a string) or the directory path and the filename separately or possibly both.
My suggestion is to keep the path and filename as separate variables for the maximum flexibility of use. In Picard's scripting, it's easier to combine the two than split them out from a combined value.
I agree. Indeed, since we already split out the existing filename and extension, we should probably do that for the proposed filename and extension.
Another consideration is how path separators are displayed and saved. The current file naming script preview displays the drive and path as used for the file system (using a colon and backslashes for windows paths). I think this is fine (and appropriate) for the display in the table in the Metadata pane. It's probably okay for the values stored in the variables as well, but we may need to be careful because of the use of the backslashes in paths on windows systems. I assume they would need to be escaped for the tags/variables, although I really don't know the details of how they are saved in the file metadata (if the user chooses to copy the variable to a tag so it is saved).
Whilst I agree in principle with this, I think that we need to stick with the way ~dirname is set in order to maintain backwards compatibility. Perhaps we should provide a script function to convert local paths to O/S independent paths and back again - and perhaps a path join function to insert the local path separator character ['/', '\']. This should be in a separate PR of course.
I agree, although I don't think we're actually changing any of the existing tags/variables so we shouldn't have any backwards compatibility issues.
I was thinking about not creating future backwards compatibility issues if we later find we got this PR wrong.
|
If I understand PICARD-2025 correctly, isn't the intent to display the same information that is displayed at the bottom of the File Naming Script Editor window (with the addition of the source path to the source filename), perhaps separated into separate lines for path and filename in the Metadata pane?
If so, then I don't believe it's as simple as creating new tags/variables, especially after the files have been saved. It would need to apply the code that calculates the target path and filename to get the values before the files are saved and include them as special lines in the table in the Metadata pane (because if they were tags they would be saved in the resulting files). You could include them as variables in the track metadata (so the user has the option of copying them to a tag for inclusion in the file), and do something special to display those variables in the table. The values for these new variables would need to be updated after every change made to the metadata, including changes made in the table manually. Also, if the variables themselves were used in a tagging script to modify tags/variables used in the file naming script you could end up with a circular reference situation. My suggestion is to not make the (new) values available in scripts, and add them as separate read-only lines in the table in the Metadata pane. That way the user can benefit from the comparison functionality in the table, but not screw up the metadata or file naming. EDIT: Actually, if the lines for these new values in the table were read-only, then I think the new variables could be allowed to be copied to tags and saved to the file metadata when the files are saved. |
|
Further to my earlier comment, note that the existing file's path, filename and extension are already available as file metadata variables |
Yes - I have previously commented that these metadata variables must start with
These are very good points indeed. The possibility of circular references in scripts is an existing possibility (e.g. But if we allow the filenaming script to modify metadata implicitly (e.g.
Preserving another copy of the metadata for the lifetime of a track/file being loaded seems likely to increase memory usage by 50% (because we will be holding original new in addition to the current original and new. Of course, we could probably save memory by enhancing the metadata class to track the changes to the new and so use memory only for those variables that have changed (either during original automated tagging scripts or manually) - but that would be a major increase in complication. Consequently, I am forced to come to the same conclusion as Bob, that having new variables for Reluctantly, I think we need to thank Arnab for all his efforts so far, and (if he is willing) ask him to start afresh and focus on making the proposed path and filename visible in the Metadata pane and NOT to create new hidden (@zytact - Let me be the first to apologise for my lack of analytical forethought being part of having caused you to waste time on the current false start. If others agree with this analysis, then... if you still wish to create new metadata variables for your own use or for minority use, then you will need to use a plugin to do it rather than have it as part of Picard.) |
There was a problem hiding this comment.
Thanks for trying to tackle this. I think this change as it is misses the point and is too simplistic, though.
It introduces path_old, which is only available after saving. The new path is then the current path. So with this patch you have the current path on the right and any path before saving on the left.
What would actually be needed is showing the current path on the left, and having the new path as it is expected to be after saving shown on the right. This is the tricky part and requires running the renaming script on the metadata. In the ticket I also mentioned how this can be a performance issue. But if this is only done for single file selection on display this might be fine.
Also in order to show an entry in the tag list it would not be appropriate to modify the file.metadata or file.orig_metadata, as this will create tags or variables. Another solution needs to be found to add this information to the data displayed in metadatabox.py.
|
Thank you so much for taking the time to review the pull request I submitted. I truly appreciate the effort and feedback you've already provided.
Thank you for saying that @Sophist-UK . Honestly, that is alright, it happens. I wanted to let you all know that looking at the feedback you all provided, I had underestimated how much time this would require, as I am currently in the middle of semester exams. Unfortunately, my schedule has been much busier than anticipated. My exams will conclude in about four weeks, and I will be able to dedicate more focused time to addressing the remaining changes and feedback after that. I hope this delay isn’t too inconvenient, and I sincerely apologize. Thank you for your understanding, and I’ll make sure to follow up as soon as I’m able. |
|
Since the tickets requesting this date back to 2019/2020, I don't think a few weeks delay for you to complete your exams will be a major problem for anyone. 😃 And relatively minor Picard enhancements have got to come a long way down anyone's priority list compared to exams that will decide your future life!! |
Quite the opposite, if you are interested in making the necessary changes you are more then welcome and I think we will all be happy to guide you through the task. This is really one of those issues that should be solved. That it is more complex than it looks like on first sight is the main reason why this ticket is still open. On the other hand I think it is also not as complicated as we maybe first thought when looking at the ticket. |
|
Thank you both for your kind words. I'm definitely still interested in working on it and greatly appreciate your willingness to guide me through the process. I'll make sure to revisit it with fresh focus once my exams are over! 😊 |
|
When you pick this up again, here are a couple of links to existing code that may be of interest: Function to produce the before and after file naming example tuple for the specified file (used in the File Naming Script Editor to display examples): picard/picard/ui/scripteditor.py Line 152 in 15a8fc8 The metadata table widget (where you would be adding the before and after path and filename items for display): picard/picard/ui/metadatabox.py Line 213 in 15a8fc8 |
|
Thank you, I'll be sure to check these out. |
Summary
Problem
Adding File Path to metadata view
Solution
Added a
Pathkey to the metadata dictionary which gets the file name from~new_pathand~old_pathkey .