Bug fixes and guided tour

[Mbucari]

More migrations to Avalonia 11.0.0-preview6

Mostly just cleaning up unused namespaces that became unused in preview 6, but also updating code that was deprecated.

Add Series Order column (#529)

Also, SeriesOrder and LastDownload columns are now hidden by default.

Do not launch settings dialog after installation (#510)

What is says, plus a couple of Avalonia UI tweaks for window sizes. Also, Avalonia's setup dialog has a ComboBox to select the theme.

Add guided walkthrough (also #510)

Allow users to cancel walkthrough

I think this is really cool. The first time libation is launched (tracked by the new FirstLaunch setting), users are asked if they'd like a guided walk through. If they say yes, they're taken on a tour of the accounts dialog, settings dialog tabs, and the import > scan action. The menu items are opened and selected, with short pauses for the user to see how to get to everything.

Don't warn for blank password with external login

login eager dialog was not letting users do external login if the password box was blank.

Improve book availability detection (#551)

Based on my and Charlie's scans, it appears that an item is unavailable if it's content_type is null.

Do not import orphaned episodes (#553)

Some orphaned episodes were still getting through. For this issue, the offending episode claimed that it had a series parent, but the supposed parent's asin was the same as the episode. So now any episode that doesn't have a series will not be imported. Also, no logging of ImportValidationException. That's what caused the >400MB log file.

Replace editable DataGridTextColumn with TextBox (#552)

The user said Libation was crashing when he double-clicked the DataGridTextColumn. I replaced those column types with template columns that contain a TextBox. It's a bandaid, but hopefully it's sufficient.

[rmcrackan]

I'm too tired for a proper PR tonight. I'll take a look manana. At first glance though :dancing_banana:

[Mbucari]

I think I Fixed #554. I made the assumption that book ASINs were unique in the DB. I know they're not the primary key, but I still thought there weren't supposed to be duplicates.

[rmcrackan]

I made the assumption that book ASINs were unique in the DB. I know they're not the primary key, but I still thought there weren't supposed to be duplicates.

This used to be true and first in wins. I believe this was only changed recently.

[Mbucari]

This used to be true and first in wins. I believe this was only changed recently.

Did you change this? Because I haven't touched the book importer. And a quick look at the change log doesn't reveal any change to importing unique books either.

[rmcrackan]

Did you change this?

Nope. I thought you did for a recent requester who had strange ideas about how to include duplicates. Guess I remembered wrong. To the best of my memory, books have always been 'first in wins' because there's no clean way to display duplicates of the same book from different accounts.

[Mbucari]

Scratch that, it's not that there's a duplicate Book asin, it's that there's more than one LibraryBook pointing to the same Book. Still that should not happen either. We still check for existing LibraryBook entries by asin when importing, and don't import duplicates. This shouldn't happen

[rmcrackan]

That's actually what I meant above: multiple library book entries with the same book. Schema-wise it makes sense: you own it in multiple accounts. For our app though, how can you reasonably depict the same book twice? I remember some recent request pushed back on this but I don't see how it wouldn't just be confusion.

[Mbucari]

That's still the approach used on import. We are not supposed to add more than one LibraryBook-Book pair. Ever. I tried playing with that idea, but I got nowhere with it. The change I made (When book is unavailable, check other accounts (#535)) only changed the existing LibraryBook.Account, it doesn't add a new LibraryBook

[Mbucari]

OK, so existing LibraryBook entries were found like this:

var existingEntries = DbContext.LibraryBooks.AsEnumerable().Where(l => l.Book is not null).ToDictionary(l => l.Book.AudibleProductId);

When Books are upserted during the BookImporter run, they are linked to their LibraryBook in the DbContext instance. If a LibraryBook has a null book here, that means it's Book was not imported during by BookImporter. So far, so good.

Next we loop through ALL ImportItems, and we only create and add a new LibraryBook if it was not found in existingEntries.

So the only way two LibraryBooks linked to the same book could happen is if, during a single import run, BookImporter somehow added a book to BookImporter.Cache but the DbContext's instance did not link the LibraryBook to that Book.

I'm stumped. I don't know how that is possible, but I'm still a db/EF novice.

[rmcrackan]

Race condition? Could the recent parallelization improvements cause this?

[rmcrackan]

Do not launch settings dialog after installation

Cool. So this just launches it with full defaults out of the gate? I think people will like that.

[rmcrackan]

Add guided walkthrough

I'm looking forward to trying this out

[rmcrackan]

Don't warn for blank password with external login

Does this have an associated ticket number?

[rmcrackan]

Replace editable DataGridTextColumn with TextBox

Do you want to wait on OP's feedback before merging this code?

[Mbucari]

So this just launches it with full defaults out of the gate?

Yup.

Does this have an associated ticket number?

No.

Do you want to wait on OP's feedback before merging this code?

Yeah, let's give it a day. It will also give me more time to ponder the duplicate LibraryBook problem. The LibraryBookImporter is run sequentially after all scanning tasks complete and all Items are aggregated, so it can't be a race condition.

[rmcrackan]

The LibraryBookImporter is run sequentially after all scanning tasks complete and all Items are aggregated, so it can't be a race condition.

Oh good; that's how it should be.

I like what I see. Just ping me when you're ready to go live.

[Mbucari]

OK, I've poured over the import process, and I just don't see how it's possible with the current system. This error is happening because there is already more than one LibraryBook linked to a book with the same asin in the db. For that to happen, BookImporter would have to import one Asin with two separate BookIds, otherwise LibraryBookImporter would have throw an exception when it tried to add a duplicate. But that also should not be possible because BookImporter only imports distinct asins:

Libation/Source/DtoImporterService/BookImporter.cs

Lines 51 to 53 in e62f01d

	Cache = DbContext.Books
	.GetBooks(b => productIds.Contains(b.AudibleProductId))
	.ToDictionarySafe(b => b.AudibleProductId);

So, I'm truly stumped. Maybe a change over the past few weeks temporarily allowed duplicates somehow (except as I've shown, BookImporter would have to have added two books with the same ASIN, and I haven't touched BookImporter). But whatever the cause, the distinct check I added today should solve it without side effects.


I did some more tweaking on the Accounts Dialog. It's possible the previous janky code was causing the crash instead of the DataGridTextColumn. If you ever look through the Avalonia code in detail, you'll notice that it tends to look cleaner and clearer the more recently that it's been edited. I haven't touched the Accounts Dialog since my initial port (except to update the file dialogs with new Avalonia versions). I was still figuring out MVVM and Avalonia back then, and a lot of my early efforts ware very hacky.

[Mbucari]

Oh, I also added two new steps to the walkthrough: Filters and Quick Filters

Go ahead and review and pull. I'm not holding my breath that that guy will test and get back to me...

Crap, I forgot I wanted to add one more thing: A menu item to run the walkthrough again.

[Mbucari]

Alright, she's ready. Can't wait for some feedback on the walkthrough!

[rmcrackan]

Fuuuuuuuuck, that guided tour/walk through is tight. I can see why you're proud of it.

• I like the cancel option throughout
• I accidentally did things twice because I thought I was supposed to click it, then the walk through did it too. That is: something blinked so I clicked on it, then the tutorial also took the same action. Was especially confusing when multiple dialogs launched. Which come to think of it shouldn't have been possible
• Accounts. 'click save'. I only did this in Classic and this is correct. Don't remember if Save button exists in Chardonnay
• "Change where liberated books are stored." ... but I don't want to. What to do? Looks like "Save" worked. Same confusion on subsequent Settings tabs. Let user know that "Save" will take them to the next step
• Even the boxes' titles are right. Nice touch
• Pretty slick that it even initiates initial scan. However my first walk through attempt reset my existing accounts, prompting for password. Could not reproduce later
• Love that there's the Settings-menu option to take a guided tour in case you want to do so later. In later walk-throughs it will be especially important that the user be clear on how to progress through any step without making obligatory changes
• The '?' box filter cheat sheet needs serious work. I'm embarrassed that it's lived this long in its original state
• Legacy 'edit quick filters' bug: top item shouldn't have an enabled move-up button; bottom item shouldn't have an enabled move-down button
• Walk through abruptly stops after edit-filters. Should let users know it's complete by signing off, saying something positive, yada yada

Wow. This is powerful stuff. I wonder how much of the readme stuff could be replaced by just this.

[rmcrackan]

I'm going to hold off on creating a release from this PR until you decide if you want to tweak the walk through first. It should get the first impression it deserves.

[Mbucari]

something blinked so I clicked on it, then the tutorial also took the same action. it shouldn't have been possible

You're right, that shouldn't be possible. I think I'll disable those menu items/buttons while they are flashing.

'click save'. I only did this in Classic and this is correct. Don't remember if Save button exists in Chardonnay

It does. I think you're thinking of the cancel button, which I almost entirely left out of the Chardonnay dialogues. To me it's redundant with the close window function.

"Change where liberated books are stored." ... but I don't want to.

Maybe I'll change the language to something like "Here is where you can..."

Same confusion on subsequent Settings tabs. Let user know that "Save" will take them to the next step

This is actually intentional. You can't close the settings dialog until every tab has been visited and that tab's message box has been displayed. It may feel a little heavy-handed, but it's the only way I could think of to allow the users the freedom to spend as much time as they want on each tab, but at the same time ensure that they visit every tab.

Maybe I'll programmatically change the text of that button too "Next".

my first walk through attempt reset my existing accounts, prompting for password. Could not reproduce later

VERY weird. That definitely should not be possible because scanning is performed by performing a click action on the menu item. Maybe because I only performed that part of the walkthrough if they have actually added accounts which I check by opening their account settings. Maybe having the account settings open caused an error when a different thread was also trying to access the same file? I'll look into it. Maybe I need to manually dispose of the account settings before invoking the scan.

Legacy 'edit quick filters' bug: top item shouldn't have an enabled move-up button; bottom item shouldn't have an enabled move-down button

I'll check it out.

Walk through abruptly stops after edit-filters. Should let users know it's complete by signing off, saying something positive, yada yada

Good idea. I was on the fence about this but I think you're right. One thing that's especially nice about using the step runner is that this whole process is highly extensible. Adding more tutorials as simple as adding a new method to the step runner. I thought about adding walkthroughs for the grid, but programmatically interacting with the grid view is, well, problematic.

---

Thank you so much for the detailed review! I'll work on it dressing these tomorrow morning, and hopefully we can have a release pushed by late afternoon!

[rmcrackan]

cancel button ... To me it's redundant with the close window function

Functionally, it's 100% redundant -- if you have the correct knowledge. I include them for clarity. If you change something and hit cancel, it's clear that nothing should have been saved. However, especially in many modern apps, as soon as you change something then it's saved; X-ing out just closes the box you're in. Lacking a Cancel button, it's unclear which paradigm is at play.

You could just end each of these tab steps with "\r\n\r\n(Click Save to continue)" or similar. This turns into the kind of thing you visually stop seeing after a few iterations. Which is a good thing: it doesn't take up much attention but it's still there in case it's useful.

Maybe I'll programmatically change the text of that button too "Next".

That also works

step runner is that this whole process is highly extensible

Yes! As I'm sure you've noticed throughout my code, I love creating polymorphic stuff like that too. So powerful and abstracted away from the business logic. Feels good man

I thought about adding walkthroughs for the grid, but programmatically interacting with the grid view is, well, problematic.

Yeah, would be cool. It's already a lot though. Don't want to risk burning out the user.