search

ankidroid / Anki-Android

AnkiDroid: Anki flashcards on Android. Your secret trick to achieve superhuman information retention.
GNU General Public License v3.0
8.41k stars 2.18k forks source link

ChangeManager: not undoable operation should also notifies #16941

Closed Arthur-Milchior closed 2 weeks ago

Arthur-Milchior commented 3 weeks ago

Right now, notifySubscribers is called on any undoable operation and on new day. (Funnily enough "undo" is an undoableOp too, so "undo" also notifies the subscriber) It's not yet done on undoable operations. Which means that, for example, the widget does not get notified that there its content should be updated.

So, this issue has two tasks:

If there are actions that you find that can't be undone, and you can't easily find how to create the notification, you can simply create a new issue. Otherwise, you can directly create a PR to add the notification.

Right now, the operations I know of are:

I should note that those four operations are done in the deck picker, and we hard coded the deck picker to update when those operations are done. Previously, no other views could be opened while the deck picker was on screen, so it was never necessary to notify subscribers, because the only subscriber is the deck picker, and it was already listening.

Now that we are introducing widgets that display the state of the collection, it'll be necessary to notify the observers.

I think the object sent for notification should be simply OpChanges with all fields set to true, given that it's too hard to know what actually was changed.

Honestly, I don't even know how to find all tasks that need to be updated. I assume most of those tasks are in GeneratedBackend.kt. But it contains 445 functions, and there is no way to read all of them and be certain that we won't miss any function that change the collection and is not undoable.

BrayanDSO commented 2 weeks ago

I don't think that doing an open hunt for all the backend methods that don't return OpChanges is a productive endeavor.

If there is an issue with the widgets or with something else, fix that specific issue and that's it.

Doing that for all the methods will likely lead to bugs and a lot of busywork. Also, they should return OpChanges in the backend itself, not here.

Another thing is that issues without clear finish criteria are bad. This issue will likely stay open forever because the backend methods will change and new ones will be added in the future.

david-allison commented 2 weeks ago

Read the .proto files on the Anki side to find the methods

Completely agreed with Brayan, I took on one task because it was in the issue tracker and I presumed there was a need

If the impact is:

It's not ideal, but we have very limited developer time, and we need to focus on higher impact issues

If we want to go through with this task, most changes should be done on the Anki backend to reduce busywork on our side, and that's probably not worth asking dae to consider, let alone prioritize unless we're putting in the effort to land the patches

Arthur-Milchior commented 2 weeks ago

I don't think that doing an open hunt for all the backend methods that don't return OpChanges is a productive endeavor.

Is OpChanges systematically sent to undoableOp? Because if there is any method that returns a OpChanges and is not sent to undoableOp then the notifications are not sent either. And even if the operations listed above, sync, empty cards and check database returned a OpChanges I don't think they should be cancellable. So we would still need to do some work in AnkiDroid side.

If there is an issue with the widgets or with something else, fix that specific issue and that's it.

Actually, it's an issue with the widget and sometime the deck picker.

If you do "empty cards" the new number of cards is not reflected in the deck picker either. I could of course directly refresh the deck picker in the callback. But I thought that sending a general notification would be better than hard coding a deck picker refresh, so that it would also update the widgets.

Admittedly, the lack of update after "empty cards" is not a huge bug. It's actually so small that I never realized it and I don't think we ever got any bug report about. Still, it would be nice to have, in order to improve the app quality. And similarly, I would expect that there are other actions that does not trigger the refresh, and which, ideally, should trigger it. Hence the general request. If we just wait until we find it by accident, it will remain uncorrected.

Also, given the implementation of the widget update, that is, simply subscribing to changes, the simple fix is really to use notification.

Another thing is that issues without clear finish criteria are bad. This issue will likely stay open forever because the backend methods will change and new ones will be added in the future.

Fine with closing the general issue if you want. I was hoping one day someone has the time to look at the list of methods, but understand it would not be the case.

A widget will be out of sync for 60 seconds if you're checking the database,

Given that the automated update is once every 10 minutes, then you'll have to wait up to 600 seconds and not 60. But I suspect it does not change your point. It also refresh after any review or card edition done by the user.

I'll admit that "add card" and "check media" are no big deal. It's probably pretty rare, and not the last sync you'd do before closing the application. However, I still believe that the issue with "sync" is a big deal. Because, if you've reviewed all your cards on computer, that you sync, you see that you have nothing to review, close ankidroid, and the widget shows you that you have 20 reviews, that could be quite frustrating. Especially if you have no way to make it display the right numbers appart from waiting up to 10 minutes.

most changes should be done on the Anki backend to reduce busywork on our side

This part, I admit I don't get. Let's consider the case of "sync". I don't sync we want "sync" to be undoable. This means that AnkiDroid will have to notify the subscriber directly, without relying on the undoableOp method. Whether the OpChanges is created by the backend or by AnkiDroid, we'll still have work to do. Admittedly, the backend would be able to state exactly what changed, while we could only create a OpChanges that would state what may have changed. I think that stating that, after a sync, everything should be considered as modified is acceptable, even if it requires to refresh something that could have been not refreshed.

I've not touched Anki code for sometime. I'd be interested in rediscovering it. But even if I were doing the work in anki's backend, I still suspect that we would have almost the same amount of work to do in AnkiDroid.

Completely agreed with Brayan, I took on one task because it was in the issue tracker and I presumed there was a need

Sorry for the lack of clarity. I gave the link to the general bug in the subcases because in order to ensure anyone interested could have context. I admit I had not considered doing any work in the backend, so I thought most would be simple-ish for new contributors, which is also why I splitted them in small tasks for them.

david-allison commented 2 weeks ago

Let's consider the case of "sync". I don't sync we want "sync" to be undoable. This means that AnkiDroid will have to notify the subscriber directly, without relying on the undoableOp method. Whether the OpChanges is created by the backend or by AnkiDroid, we'll still have work to do.

I would expect:

If we know we have bugs in the refresh, we can change the 600 second refresh to 60 seconds, and we can prioritise issues as performance issues

Arthur-Milchior commented 2 weeks ago

I admit David that I fail to understand what you are stating.

Regarding sync, given that #16958 already exists, I suggest we move the discussion there if there is anything more to discuss. Right now, I agree with your two expectations. But I still don't understand how they relate to the request that most changes are done in back-end. While the backend already returns after sync some type that provides useful information (even if it's not OpChanges)

Let's consider the case of "sync". I don't sync we want "sync" to be undoable. This means that AnkiDroid will have to notify the subscriber directly, without relying on the undoableOp method. Whether the OpChanges is created by the backend or by AnkiDroid, we'll still have work to do.

I would expect:

  • Anki Desktop has multiple windows, and should have code in place to notify these windows on changes.

Certainly

  • A change does not need to be undoable. OpChanges is the data which represents a change, I don't believe this has to be an undoable change (although our consumer is undoableOp, it doesn't have any direct relevance to the undo queue).

Unless I seriously misunderstand the way Anki works, the way OpChanges are currently created, is by:

If you're not currently in an undoable operation, trying to get the OpChanges will fail. https://github.com/ankitects/anki/blob/be2f013cb7a88bab869bb3428aff18dc58a72fe5/rslib/src/undo/mod.rs#L129

Of course, you're right that technically, nothing could stop the backend from creating a OpChanges even without creating a undoable operation. The back-end could do the exact same thing as what I did in #16958 where I just created a OpChanges from scratch. Still, this seems pretty pointless. I assumed that when Brayan wrote above that the back-end should be in charge of creating the OpChanges, it means that we should use the architecture that already exists in the back-end. Because, otherwise, there is really no advantage of doing it in the back-end in the first place.

Note that, since there is already a PR for the case of sync, I guess we should move the conversation there if you still expect that the work should be done in back-end.

If we know we have bugs in the refresh, we can change the 600 second refresh to 60 seconds, and we can prioritise issues as performance issues

My best guess about the last part of this sentence is that you mean that the developers should prioritize performance issue. In the absolute sense, I agree that it's nice thing to attack. However, given how simple some of the fix are (even taking into account the time to rewrite the PR the way you wanted it) I still expect that it's a good ration of user experience/developer time.

If I were asking to do big rewrite and architectural change, I'd agree it would not be worth it.

However, I'm not sure how this interpretation would relate to the first part of the sentence. So I'd be fine if you could help me understand.

Finally, given that you're probably right that it's not worth our time to search for all operations that update the collection to send notification, I'd be fine closing this issue. I believe the most important change is Sync, as it's, hopefully, done many time a day, and once it's done, we've done the biggest part of the work

david-allison commented 2 weeks ago

Unless I seriously misunderstand the way Anki works, the way OpChanges are currently created, is by: ...

From the lens of AnkiDroid:

https://github.com/ankidroid/Anki-Android/blob/79b7a6e8bd428df9eef43ec852106d98cf5cb8f5/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt#L94-L105

'undo' and 'changes' are decoupled, although we use undoableOp as the name, this is only a name

Of course, you're right that technically, nothing could stop the backend from creating a OpChanges even without creating a undoable operation. The back-end could do the exact same thing as what I did in https://github.com/ankidroid/Anki-Android/pull/16958 where I just created a OpChanges from scratch. Still, this seems pretty pointless.

Notifying us of what has changed seems like it should be the responsibility of the backend

If the backend changes its implementation, we need to be actively aware of the code changes made so we can update our implementation. Realistically we can't automatically test for this, our implementation is in a different repo from the original code, and it leaves us open to a preventable bug.

In the case of sync, which can theoretically change everything, an 'everything changed' message makes sense to be fired from our side: a change to the backend would not impact this function

However, given how simple some of the fix are (even taking into account the time to rewrite the PR the way you wanted it) I still expect that it's a good ration of user experience/developer time.

I'm saying:

Arthur-Milchior commented 2 weeks ago

@mikehardy I allowed myself to remove the tag. As far as I understand, this issue will be closed as there seems there is nothing for ankidroid to do. I kept it open because there are related discussion that seemed interesting. But I don't think there is more to say here