-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
edit plugin: make temporary file reflect --set CLI arguments #5056
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the interesting approach! It looks like the idea here is to insert to a block comment above the main YAML doc with some reminders to the user. Maybe we should think a little bit about the UI implications. For example:
- Will people get tired of seeing this after the first time? Especially if you edit many albums, the set of fields will always be the same for the same import session.
- Printing this before the YAML document means that people have to move the cursor downward to start editing. Maybe having stuff like this below, like a git commit message, would be more usable?
- Perhaps it would be helpful to separate the two categories of read-only fields so users know why they're appearing?
beetsplug/edit.py
Outdated
self.readonly_fields = {} | ||
for field, view in config["import"]["set_fields"].items(): | ||
self.readonly_fields[field] = view.get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason that you used a separate event handler here? It also seems like we could fetch the list of fields directly in the edit_object
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no specific reason, just bad planning (I initially thought that the set fields were only accessible in the session
object, which is out of scope in the edit_object
method, and then didn't simplify the code when I found out it wasn't the case)
In terms of UI, I made changes that look at the first two points you pointed out.
Actually, it might be less friction to not show that warning message in the file itself, but detect if a read-only/placeholder field has been edited, and warn about that in the terminal? ...It might cause the user to edit a field for 20+ tracks, only to be told that work was useless. I'll need to think more about which solution introduces the least amount of friction. |
for now, I've settled upon:
I just realised that there is no way to proper way to unset a field now, just set it to be blank. Let me fix that. |
actually, don't merge just yet, there might be a really bad breaking bug that escaped the existing tests, and discards user edits in... some situations? |
Description
when using the edit plugin during import, make the user aware that any
--set
fieldswill be overwritten, and pre-fill said fields.
(by
--set
fields, I mean for instancealbum
inbeet imp "--set=album=Greatester Hits" ./tracks-for-which-the-titles-will-be-detected-wrong/
)To Do
Documentation.Changelog. (awaiting review)Tests.Not sure if it is possible to test UI-only changes