mustang-im / mustang

Mustang - New full-featured desktop email, chat and video conference client
https://mustang.im
Other
5 stars 0 forks source link

FastList: Changing the list doesn't immediately update the displayed items #66

Open benbucksch opened 3 weeks ago

benbucksch commented 3 weeks ago

FastList.svelte line:

  $: showItems = $items.getIndexRange(scrollPos, showRows) as T[];

should observe items and run whenever items are added or removed, therefore setting showItems to the new values, which then invalidates {#each showItems as item}, which should re-create the displayed rows.

But it doesn't work.

Reproduction:

benbucksch commented 3 weeks ago

@NeilRashbrook Can you see my bug here? I understand there's a lot of Svelte special logic involved here, but maybe you see the problem anyway.

NeilRashbrook commented 3 weeks ago

I tried to follow the execution from deleteIt in SavedSearchFolder.ts.

savedSearchFolders is an ArrayColl. It has one observer, the AdditionCollectionDedupObserver beloning to allAccountsAccount.rootFolders. This then has several observers, all TransformedCollectionObservers for SortedCollection. Unfortunately none of these collections themselves have any observers.

Next I looked at when the SortedCollection gets created. Well, it turns out that the folder list isn't actually a FastList, it's a FastTree. And that references its items on line 32:

https://github.com/mustang-im/mustang/blob/ea8f9389d64326dbdcf9e2b5655220328e35d47c/app/frontend/Shared/FastTree.svelte#L32

Double-checking the generated code, I noticed that Svelte wasn't subscribing to items, although the generated code for FastList does subscribe to its items. Why? Well, there's no auto-subscription in FastTree.svelte! That's right, you're missing a $ before items, and that line of code should read like this:

  $: showItems = new ArrayColl($items);

Now when you delete the saved search, the folder list updates.

But it promptly crashes the app if this was the last saved search, or if you misclick and hit the "..." of Drafts instead; this is because AllFolders.ts doesn't override the Folder.ts function disableChangeSpecial. I assume that the override simply needs to return a string along the lines of "You cannot change an All Accounts special folder."; I guess EWS will also want an override (you can't change the specialness of EWS folders at all; in theory you can create special folders but not of any of the email types.).

benbucksch commented 3 weeks ago

$: showItems = new ArrayColl($items);

Thanks for finding this. I commited that change, on your behalf.

AllFolders.ts doesn't override the Folder.ts function disableChangeSpecial. I assume that the override simply needs to return a string along the lines of "You cannot change an All Accounts special folder."; I guess EWS will also want an override (you can't change the specialness of EWS folders at all

Can you please make 2 PRs for that?


I also have the problem in the message list, which is a FastList, not a FastTree. So, that must be yet another bug, then, not the same root cause.

Initial description:

Delete a few emails. The first one works, but subsequent ones won't update the list of messages properly.

It may be a little more difficult to reproduce, but you'll see it when deleting multiple emails in short succession.

This one is actually the most serious, because it may make me delete the wrong email! Because the FastList display and selection don't reflect the right state. Could you also look into this bug, please?

NeilRashbrook commented 3 weeks ago

So far the only thing I've been able to reproduce sometimes is that deleting a message sometimes makes the selection jump two messages.

NeilRashbrook commented 3 weeks ago

... which happens because the message deletion function selects the next message based on their order in the folder rather than their order on screen.

Message list is otherwise still updating for me.

benbucksch commented 3 weeks ago

@NeilRashbrook OK, that's good to hear. Do you have a suggestion how to fix the selection in FastList itself? (Instead of letting the message delete function select another message.)

Desired result: If only 1 item is selected, and the selected items is removed from the list, select the item that was in its place in the list on screen.

I know this is going to be tough. I already tried and didn't manage. Maybe you have a better idea from another angle. If you have a nice idea, please make a PR. If you have multiple ideas, you can also make multiple alternative PRs.

NeilRashbrook commented 2 weeks ago

By my reading of the code, FastList already tries to do this: https://github.com/mustang-im/mustang/blob/master/app/frontend/Shared/FastList.svelte#L268

NeilRashbrook commented 2 weeks ago

Or was that what you were referring to in your edit?

benbucksch commented 2 weeks ago

Yes, indeed, I tried. But as you can see in the app, it doesn't work, and I don't know why.