Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PICARD-2025: Add file path to metadata view #2552

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+23 −3
Open

PICARD-2025: Add file path to metadata view #2552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions picard/file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"~" missing.

config = get_config()
# Check that file has not been removed since thread was queued
# Also don't save if we are stopping.
Expand Down Expand Up @@ -412,6 +413,10 @@ def _save_and_rename(self, old_filename, metadata):
return new_filename

def _saving_finished(self, result=None, error=None):
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
Comment on lines +416 to +419
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Sophist-UK Sophist-UK Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that Bob has prompted me to think further, it seems to me that we need to know:

  1. The current path i.e. where the file actually is - displayed in Old metadata.
  2. The future path if we were to save the file now - displayed in New metadata.
  3. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... 😜]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Handle file removed before save
# Result is None if save was skipped
if ((self.state == File.REMOVED or self.tagger.stopping)
Expand Down
21 changes: 18 additions & 3 deletions picard/ui/metadatabox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,9 +561,24 @@ def update(self, drop_album_caches=False):

def _update_tags(self, new_selection=True, drop_album_caches=False):
self.selection_mutex.lock()
files = self.files
tracks = self.tracks
self.selection_mutex.unlock()
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

files = self.files
tracks = self.tracks

if not (files or tracks):
return None
for file in files:
old_path = file.metadata.get('path_old', file.filename)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing "~".

file.orig_metadata['Path'] = old_path

new_path = file.metadata.get('~path_saved', file.filename)
file.metadata['Path'] = new_path
Comment on lines +572 to +575
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have introduced a new metadata tag here and there are various issues with this:

  1. 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.
  2. This is NOT a tag that should be saved to the file, so it should be ~path and ~filename. Path may be saved as a miscellaneous text tag in some formats that support this (e.g. ID3).
  3. You will need to make sure that these metadata variables are recalculated if the user changes Move or Rename options.
  4. You will need to change the metadata table generation to add rows for ~path and ~filename and ensure that they are used when comparing old and new metadata and when summarising metadata over several albums or tracks.
  5. You will need to add definitions for Path and Filename metadata elsewhere so that the names get translated.
  6. You may need to write additional tests.

None of the above are major pieces of work, but they do need to be done.

# Update album cache check
if new_selection or drop_album_caches:
self._single_file_album = len({file.metadata['album'] for file in files}) == 1
Comment on lines +577 to +578
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have been included in the try block in error since it still exists a few lines further down.


finally:
self.selection_mutex.unlock()

if not (files or tracks):
return None
Expand Down
Loading