GNOME Bugzilla – Bug 563630
Split 'File Location' field into 'File Path', 'File Location' and 'File Name'
Last modified: 2020-03-17 08:31:40 UTC
Please describe the problem: In order to have more complex rules for Smart Playlists, it would be good to have fields that match the filename and dir name of the files. Right now we only have "File Location", which maps directly to the file path of the file. What I propose is to rename "File Location" to "File Path" and have another 2 more fields. Example: File Path: "/home/user/Music/ColdPlay/ColdPlay - White Shadows.mp3" File Name: "ColdPlay - White Shadows.mp3" File Location: "/home/user/Music/ColdPlay/" Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 136085 [details] [review] Proposed patch, implemented against git master Note: Possible approaches I considered were: a) Add FileLocation and FileName as new columns, having denormalized data in the DB (redundant) in order to have backwards compatibility. b) Remove Uri replacing it with FileLocation and FileName. Advantage: no redundant data. Disadvantages: we loose 1 field for queries (maybe some user was interested in the full path...), and we're not backwards compatible. However while implementing it, I realised there's a new alternative, the best of both worlds: c) Add FileLocation and FileName, and leave Uri as a QueryField that consists of a concatenation of the new columns. This way we don't loose any search field, and do not have redundant data on the DB. It's not backwards compatible though, but: - I've implemented a Migrate_34() method. - Anyway, as discussed on the mailing list, databases between 1.5.0 and 1.4.3 are not compatible. Please review! Thanks.
Could you take mount point into account and make file location relative to that? If so, how hard would it be add volume/device UUID along with the path so you can show/hide parts of the library as devices are connected and disconnected (also making things work even if the path to the mount point changes)?
Hey Patryk. I'm not sure I understand your particular problem, I guess because I don't understand your use case in the first place. Anyway, it seems like a separate issue, so could you file a new bug (with more details, like steps to reproduce, current results, actual results,...) and CC myself? Thanks!
It's not really an issue in most of the cases but since you're already splitting the field in the DB, I thought about making Banshee more fool-proof. The use case is keeping the music collection on portable media - be it pendrives or hard drives connected to USB or eSATA. Currently Banshee keeps track of the full path, including the mount point. If a file is missing it's impossible to tell if: 1) the disk is still mounted but the file was removed 2) the disk is not mounted 3) the disk is mounted under a different mount point (1) should probably be detected and Banshee could ask the user if he wishes to remove the file from the library,(2) should probably tell the user that the media is not plugged in/inserted while (3) should be handled transparently and JustWork™. (3) happens where multiple devices fight over the common /media/disk path and they end up as either /media/disk, /media/disk-1 or /media/disk-2 depending on the order they were connected and/or detected. I think the easiest way would be to only store the relative path from the longest matching mount point (in case mounts were nested) and the UUID of the device/volume mounted. Not sure about having a separate ticket to do this unless there is support at the DB level.
Patryk, it's definitely a different issue. Supporting your scenario would require MUCH more changes to the patch I posted. It's much better to separate the ideas here, since my patch just refactors the Uri to 2 fields, but your problems requires much more thinking about the design approach to take. (Furthermore, recently Banshee switched to an absolute-URI-always model, so using the relative-path approach would bring regressions again.)
According to the Developer Meeting on July the 20th, this patch goes to 1.8 or beyond. Also, I have to fix some whitespace issues on the patch because right now it seems to change existing migration paths...
Created attachment 138918 [details] [review] Splits FilePath into FileLocation,FileName,FilePath Updated to git master, fixed whitespace issues, removed wrong change in a database migration path
Created attachment 139035 [details] [review] Splits FilePath into FileLocation,FileName,FilePath v3 - I was lacking INDEX replacements. - CoreTracksOld was not needed, as a new DB doesn't execute all migration paths. - Corrected some syntax nitpicks.
Bulk changing the assignee to banshee-maint@gnome.bugs to make it easier for people to get updated on all banshee bugs by following that address. It's usually quite apparent who is working on a given bug by the comments and/or patches attached.
FYI: Now that 1.6 was released, I'll update this patch soon to master so we can apply it to HEAD.
Created attachment 157863 [details] [review] Patch v4 Updating patch to master, so that it can be applied to 1.7.x series and thus be available in 1.8 stable.
Whooboy, this is a big patch. The main benefit is ability to group tracks by their directory, which isn't that huge a benefit for how big a change this is. I'm wary of the complexity of it, the re-building of the CoreTracks table, and the churn in general. Another approach to get the benefit with much less complexity would be to add a column 'FileDirectory' that is read-only, and generated (like the Sort/Lowered columns) from the real, read-write column/property, Uri.
Hey Gabriel. Many thanks for your message. That being said, can we discuss this calmly? I've put a lot of effort in this patch, to just be rejected so simply, precisely when I'm so sure that it's the right approach. Why the right approach (at least compared with the one you propose)? Mainly, because it doesn't mean duplicated data on the DB. I'll explain each change better, sorry for the lack of explanations. (In reply to comment #12) > Whooboy, this is a big patch. The main benefit is ability to group tracks by > their directory, which isn't that huge a benefit for how big a change this is. > I'm wary of the complexity of it, the re-building of the CoreTracks table, and > the churn in general. Two points here I forgot to mention: a) If you're worried about the instability that it may cause, you shouldn't, firstly because I've been using a modified version of banshee with this patch for some months now (almost a year) and it works flawlessly, very stable; secondly because we just tagged a stable version of banshee and now it can receive bigger patches to master, right? (Or create a branch for it, like with other features.) b) The re-building of the CoreTracks table is, indeed, a reduction of complexity. If I hadn't done that, I'd have had to duplicate in the code the creation of the table in the DB migration step. The only thing that I do here is not repeating myself. I understand that if not having a deep look on the patch it looks big, but I can create another version of the patch without the CoreTracks table rework so you can see that the real change is very small (it's basically to divide 1 column into one, and to change all URI queries to use a concatenation of two columns instead of just one column). I'll work on this version. We can also chat on IRC about it, ping me please. Thanks again.
Hey Andrés, I'm sorry I came off as harsh - I appreciate your work on this and all the other patches you've contributed. I guess my intention was to back the discussion up a bit, make sure this approach got thoroughly discussed before being committed. Avoiding duplication is an ok goal, but keep in mind we already duplicate quite a few fields - namely the search/sort fields. And that not duplicating means we have to do more work (combining the dir/file fields) on every row-test of every search, every TrackInfo load, etc. I'm not set either way, but I'm not yet convinced your approach is the best.
Created attachment 157932 [details] [review] Patch v5 (In reply to comment #14) > Avoiding duplication is an ok goal, but keep in mind we already duplicate quite > a few fields - namely the search/sort fields. And that not duplicating means In that case I understand it because those columns are used all the time for searches. But the Uri field is just used to play the song, so we would just lose the performance of 1 extra concatenation. > we have to do more work (combining the dir/file fields) on every row-test of > every search, every TrackInfo load, etc. I would say that the cost of combining the fields in searches is minimal, because it happens at a DB query level (using the "||" operator). > I'm not set either way, but I'm not yet convinced your approach is the best. I've managed to reduce the patch now 10kB from its original form, so now the changes to the CoreTracks creation are minimal.
Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived. Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the responsibility for active development again. See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.