rmcrackan / Libation

Libation: Liberate your Library
GNU General Public License v3.0
2.56k stars 138 forks source link

Track Libation Version and Downloaded Timestamp for Downloaded Files #498

Closed manalive closed 1 year ago

manalive commented 1 year ago

Is your feature request related to a problem? Please describe.

I would like to be able to see what version of Libation was used to download a file.

A recent bug fix was related to malformed downloads. Another recent (to me) enhancement was moving the moov atom to the beginning of the file.

It would be great to know which downloaded files need to be re-downloaded to take advantage of bug fixes or a new feature.

Describe the solution you'd like

Optimally, I would this info in Libation as columns AND as a custom tag (TXX:Downloader, TXX:Downloaded for example) but even just in Libation would be very useful.

Mbucari commented 1 year ago

Date and time download tags were recently added. I think a libation version tag easy enough.

The grid is already pretty busy with lots of info, but I guess that doesn't matter so much since you can disable columns. what do you think @rmcrackan?

manalive commented 1 year ago

Extra points:

rmcrackan commented 1 year ago

a libation version tag easy enough

It's doable. This will be a break with current practices though. Other than the 'file locations' file we actually don't store anything about specific files. We'd need to create a new location for that info. After that it should be added to the search engine and as a hide-able grid column.

Value in Libation version column is clickable link to release/notes on github

It's not hard to do this. It's hard to do without cluttering the user interface. It would be effortless to put it in the details box as normal text just like the other details so that's where I'd start.

Value in Download Date column is clickable link that auto-populates search box with string to find all from the same day

I'm not sold on the specifics but I see where you're going with this. It's a feature idea with far reaching implications. When I filter on this date, do I add the date to my current filter? Or replace my current filter? In my case, I use quite advanced filters. How would it interact with those? Would I just lose my last search? Would we also need a back-button/undo feature?

If we do this, it will need to be well thought out.

or if not possible, copies that string to the clipboard

Taking over the user's clipboard when you normal left click on something shouldn't happen. We do however have right-click > copy and ctrl+c also works.

manalive commented 1 year ago

we actually don't store anything about specific files

Personally, I don't care about storing info about the files in libation. What I really want is to see in Libation when the whole book was last successfully downloaded.

It would be nice to also have stuff in the tags of the file so it's visible in other programs, namely mp3tag.

Mbucari commented 1 year ago

So I misunderstood what you were asking. I thought you wanted a new file naming template tag for Libation version. That's easy. As @rmcrackan said, new database entries is doable, but more than I was imagining.

@rmcrackan I don't fully understand the database schema, but am I correct that if multiple accounts have the same book that the DB contains only 1 entry for that Book, but it contains one LibraryBook for each account with that Book? Also, Libation's products grid has one row per LibraryBook, but UserDefinedItem is linked to Book. Does that mean that when a book is downloaded from one account, it's marked as downloaded for all accounts with that book? And also are Tags set on one LibraryBook automatically set on that book for all other accounts?

And my ultimate question is, does it make more sense to place this new info (last DL date and Libation Version#) in the Book, LibraryBook, or UserDefinedItem table?

rmcrackan commented 1 year ago

if multiple accounts have the same book that the DB contains only 1 entry for that Book

Correct. A book is what it is irrespective of who owns it.

but it contains one LibraryBook for each account with that Book?

Yes. First-in wins. (I once had it as a grid entry per library book but that was very confusing in the case of duplicates.) Library Book is the account's relationship with the book. In theory, multiple accounts could own the same book, thus multiple Library Book entries. In practice that's too confusing to show in the grid.

Libation's products grid has one row per LibraryBook, but UserDefinedItem is linked to Book

This was a subtle one. If I have 2 accounts with the same book and I add tags to the book, then my tags are about the book itself, irrespective of how many accounts own it. Likewise: if I've downloaded that book, then the book itself is downloaded. It's not the book from a specific account. I could have 3 accounts which owned the same book but there'd be no need to download it more than once -- it's literally the same file.

Does that mean that when a book is downloaded from one account, it's marked as downloaded for all accounts with that book?

Yes

also are Tags set on one LibraryBook automatically set on that book for all other accounts?

Yes because it's connected to the Book, not the Library Book (ie: the account).

And my ultimate question is, does it make more sense to place this new info (last DL date and Libation Version#) in the Book, LibraryBook, or UserDefinedItem table?

That's a great question. By the above logic, last download info would be UserDefinedItem. It's a relationship between the user and that book -- it's not important which account it came from.

In database terms, you can think of LibraryBook of the x-ref table which joins Accounts and Books. But there is no point for an actual Accounts table since each account is only a name/email + locale. Also, this lets users delete Accounts without breaking data integrity.

Mbucari commented 1 year ago

Thanks for the primer!

Now a design choice question: what happens to these entries when a user manually set the liberated status? When set to NotLiberated, do we retain or clear last downloaded info? When set to Liberated, so we retain existing values or update them with the current DateTime and version number?

rmcrackan commented 1 year ago

Theoretically, it doesn't matter. It should be a meaningless field at that point. In practice though, someone is going to come up with some reason I can't think of for why it should be there; so we may as well leave it. If it's there and no one needs it, no harm done. If it's there and someone wants it for some weird reason -- hey, there it is. And one less thing for us to clean up when the manual switch occurs.

(I'm already dreading when someone asks why we aren't keeping a full record of each time they downloaded and the stats for each. To be completely clear: I do not want to go down that rabbit hole, I'm just not looking forward to that day.)

Off the top of my head, I could see this working either of these ways. (I'm not married to this name. Again: top of my head):

Book
  UserDefinedItem
  DownloadStats

or

Book
  UserDefinedItem

UserDefinedItem
  DownloadStats
Mbucari commented 1 year ago

Theoretically, it doesn't matter. It should be a meaningless field at that point.

I suppose you could leave the cell blank if book status is NotLiberated. But in the case where you set a book to not download it and then set it back to liberated, should these previously assigned properties show back up, or should they be cleared? I could make cases for both.

If these values are visible in the grid, then it does matter. By default they should be null and leave a blank cell and only have a value when assigned (Like rating).

I was playing with it last night, and this is what I have so far.


internal ulong? downloadedWithVersion;
public DateTime? LastDownloaded { get; private set; }

public Version DownloadedWithVersion
{
    get =>
        downloadedWithVersion is null ? null
        : new Version (
            (ushort)(downloadedWithVersion >> 48),
            (ushort)(downloadedWithVersion >> 32),
            (ushort)(downloadedWithVersion >> 16),
            (ushort)downloadedWithVersion);

    private set =>
        downloadedWithVersion = value is null ? null
        : (ulong)(value.Major & ushort.MaxValue) << 48
        | (ulong)(value.Minor & ushort.MaxValue) << 32
        | (uint)(value.Build & ushort.MaxValue) << 16
        | (ushort)value.Revision;
}

public void SetLastDownloaded(Version version)
{
  if (version != DownloadedWithVersion)
  {
      DownloadedWithVersion = version;
      OnItemChanged(nameof(DownloadedWithVersion));

      LastDownloaded = version is null ? null : DateTime.Now;
  }
}
rmcrackan commented 1 year ago

1) If these values are visible in the grid, then it does matter

Fair point

2) The more I think about it, the more I like the idea that those date and version values should remain. That really is when the user last downloaded it. If they manually flip to not downloaded so that they can download again, I don't see a reason to remove that data. Just overwrite it on next download

3) These may as well also be added to the search engine and exports

4) DateTime.Now -- I don't recall if we use Now or UtcNow throughout

5) Not sure why it's better to store Version as ulong. What happens if you just leave it as Version and let EF determine how to store it? If we do need to translate just to appease EF, I recommend using MapDataConversions. I imagine there are a few places I should have done so myself. I use this a lot at work where our db uses postgres jsonb fields. I have to map the db json fields to the domain object. Then in unit tests I override MapDataConversions because the in-memory data context doesn't support json fields so it has to store it another way. This lets all code use domain objects. The implementation details of unvalidated, no-logic, data translation remain in the data layer:

// domain object
public class XyzEntry
{
    public List<Widget> Widgets { get; set; }
}

public partial class MyContext : DbContext
{
    public DbSet<XyzEntry> Xyzs { get; protected set; }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.ApplyConfiguration(new XyzEntryConfig());

        MapDataConversions(modelBuilder);
    }

    private static JsonSerializerOptions options { get; } = new(JsonSerializerDefaults.Web) { WriteIndented = true };

    protected virtual void MapDataConversions(ModelBuilder modelBuilder)
    {
        modelBuilder
            .Entity<XyzEntry>()
            .Property(p => p.Widgets)
            .HasConversion(
                // turn List into a JsonDocument for db storage
                widgetList => JsonSerializer.SerializeToDocument(widgetList, options),
                // turn the JsonDocument back into a List of domain objects
                jsonDocument => jsonDocument.Deserialize<List<Widget>>(options));
    }
}

internal class UnitTestMyDbContext : MyContext
{
    protected override void MapDataConversions(ModelBuilder modelBuilder)
    {
        modelBuilder
            .Entity<XyzEntry>()
            .Property(p => p.Widgets)
            .HasConversion(
                // turn List into a string for in-memory db storage
                widgetList => JsonConvert.SerializeObject(widgetList),
                // turn the string back into a List of domain objects
                jsonString => JsonConvert.DeserializeObject<List<Metabolite>>(jsonString));
    }
}
rmcrackan commented 1 year ago

Now available in version 9.4.3