search

jcklie / anki-maobi

máobĭ (毛笔) is an Anki add-on to create cards with writing quizzes for Hanzi (Chinese characters)
MIT License
47 stars 7 forks source link

#28 - restarting quizzes #29

Closed arueckle closed 4 years ago

arueckle commented 4 years ago

What's in the PR

How to test manually

Documentation

jcklie commented 4 years ago

It looks nice, but when I try to restart during a reveal, nothing happens. Is it possible to let restart restart even during reveal?

arueckle commented 4 years ago

It is not easily possible without any side effects. I quickly tested two variants.

(1) Allow invoking character-reveal even when there is a reveal animation currently in progress. Unfortunately, I do not see any cancel-animation method in HanziWriter. I tried several things including setCharacter (which is supposed to cancel quizzes and cancel animations). HanziWriter seemingly keeps some internal state, which make the next animation flicker and cancel unexpectedly. This is easily reproducible.

(2) I also tried restarting quizzes while an animation is in progress (as suggested). This works better, likely also because for a restart the whole HanziWriter container div is re-created. Still, repeatedly using reveal character animations and then cancelling them with restart-quiz causes similar flicker effects and canceled animations. It is more difficult to reproduce.

I would probably keep it as-is in favor of stability and less unexpected behavior. If the issue basically is that the reveal animation takes too long for complex characters, we could make the animation speed configurable in the UI.

chanind commented 4 years ago

What's the bug related to flickering and messing up on the next animation? Could you open an issue in the hanzi-writer repo? Would be happy to fix the issue!

Calling writer.showCharacter() or writer.hideCharacter() should cancel any running animations. It's also not a problem to add a writer.cancelAnimation() method to HanziWriter if needed, but I'm not sure what the method should do - ex, immediately show the whole character or immediate hide the character? There's also a writer.pauseAnimation() method which will stop the currently running animation until resumed.

arueckle commented 4 years ago

Thanks for your input and for creating the HanziWriter library!

I will try to come up with a minimal example that reproduces the issue outside of Maobi and file a bug soon when I have more information about it.

arueckle commented 4 years ago

I have investigated the problem more closely. Good news, it was not related to HanziWriter! I was not able to reproduce it with a minimal example outside of Maobi. HanziWriter behaves as expected, and my guess that there might be some internal state that is not fully cleared turned out to be not true.

The bug was in our implementation for the onComplete callback that is passed to HanziWriter.animateCharacter. The callback is not only invoked after finishing the animation, but also when the animation is cancelled. We call different methods of the current HanziWriter instance after a timeout, and not properly handling cancelled animations led to unexpected behavior.

To avoid similar issues in the future, we should refactor the JS code of Maobi to better deal with asynchonous behavior and animation states. The JS part has grown quite a bit with the new features without being restructured.

I resolved the issue and have implemented the suggested changes by @jcklie. Thanks again to @chanind for triggering this investigation.

jcklie commented 4 years ago

Looks good, should we merge it now?

arueckle commented 4 years ago

Looks good, should we merge it now?

Yes, thanks!