Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remember how many cards we've learned today even after re-opening the app #110

Merged
merged 9 commits into from Mar 3, 2019

Conversation

woohgit
Copy link
Contributor

@woohgit woohgit commented Feb 23, 2019

Rationale

When we close the Mnemosyne application and re-open it, it won't
remember the number of the already memorized cards. In other words, it
won't warn you if you've already learned 15 or more words.

Implementation

This PR aims to fix this issue and re-construct the
self._fact_ids_memorised from the logs which are not that hard.

We can fetch all the newly learned cards (same query as in the statistics) and all the forgotten but
learned cards today. Combining the two, we can get the number of learned cards today.

New features

  • The already memorized cards are re-constructed from the logs
  • It only warns once per day
  • Fixed long-time flaky python3.7 test

Any comments are welcome.

When we close the mnemosyne application and re-open it, it won't
remember the number of the already memorized cards. In other words, it
won't warn you if you've already learned 15 or more words.

This PR aims to fix this issue and re-construct the
`self._fact_ids_memorised` from the logs.

We can fetch all the newly learned cards and all the forgotten but
learned cards today. Combining the two, we can get the number of the
learned cards today.
@woohgit
Copy link
Contributor Author

woohgit commented Feb 23, 2019

I need to check why the database is not set up properly during the test. The path is None so it fails for sure.

======================================================================
ERROR: test_cramming.TestCrammingScheduler.test_4
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/wooh/venvv3/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/wooh/repos/mnemosyne/tests/test_cramming.py", line 160, in test_4
    self.review_controller().reset()
  File "/home/wooh/repos/mnemosyne/mnemosyne/libmnemosyne/review_controllers/SM2_controller.py", line 52, in reset
    self.scheduler().reset(new_only=new_only)
  File "/home/wooh/repos/mnemosyne/mnemosyne/libmnemosyne/schedulers/cramming.py", line 29, in reset
    SM2Mnemosyne.reset(self, new_only)
  File "/home/wooh/repos/mnemosyne/mnemosyne/libmnemosyne/schedulers/SM2_mnemosyne.py", line 147, in reset
    self._fact_ids_memorised = self.today_learned_fact_ids()
  File "/home/wooh/repos/mnemosyne/mnemosyne/libmnemosyne/schedulers/SM2_mnemosyne.py", line 679, in today_learned_fact_ids
    result = self.database().con.execute(
  File "/home/wooh/repos/mnemosyne/mnemosyne/libmnemosyne/databases/SQLite.py", line 255, in con
    self._connection = _Sqlite3(self.component_manager, self._path)
  File "/home/wooh/repos/mnemosyne/mnemosyne/libmnemosyne/databases/_sqlite3.py", line 54, in __init__
    self.connection = sqlite3.connect(path)
TypeError: expected str, bytes or os.PathLike object, not NoneType

----------------------------------------------------------------------
Ran 1 test in 0.136s

FAILED (errors=1)

@woohgit
Copy link
Contributor Author

woohgit commented Feb 23, 2019

It seems that checking that the DB is loaded or not satisfies the test:

        if not db.is_loaded():
           ...

@pbienst
Copy link
Contributor

pbienst commented Feb 24, 2019

I'm not against reconstructing the memorised ids from the log, but I would only warn once per day, otherwise it seems very obtrusive.

People already now complain about this warning...

@woohgit
Copy link
Contributor Author

woohgit commented Feb 24, 2019

I'm not against reconstructing the memorised ids from the log, but I would only warn once per day, otherwise it seems very obtrusive.

People already now complain about this warning...

oh I didn't know this. In this case I'd only warn exactly at the == 15. Instead of the >= 15

On the other hand, why not adding a new configuration option, to disable warning if that is so annoying to others? To be quite honest, I find it very useful. When I try to learn more than 15, like 30, it'll backshot after 3 weeks and my retention drops drastically.

@woohgit
Copy link
Contributor Author

woohgit commented Feb 24, 2019

@pbienst by the way I'm really grateful that you created this application. I'm using this for more than 2 years and I'm pretty satisfied with it. It helps a lot! with learning Japanese :)

@pbienst
Copy link
Contributor

pbienst commented Feb 24, 2019

My pleasure!

When saving the Settings dialog, the `reset` is alwas called, so it 
would trigger an alert every time when, because during the reset 
function, the `warned_about_too_many_cards` is reset to 
`warned_about_too_many_cards = False`. 

This fix initializes the `warned_about_too_many_cards` on the class 
level, and calling  `reset` won`t reset it back to `False`.

Also only warn when we've just reached the 15 new or forgotten words.
Add a new event type: `WARNED_TOO_MANY_CARDS` so we can query from the 
log table if the warning has been already shown or not.
- Add test coverage
Warning on start-up would be only useful if we could alert more than 
once a day. If we already displayed a warning, we should never show the 
warning again on start-up.
Python lists are unordered, so comparing them will result in a random 
result.

Using `sort()` will ensure that the elements are ordered so we can 
compare them properly. Using sort is `O(n log n)`.
@woohgit
Copy link
Contributor Author

woohgit commented Feb 25, 2019

@pbienst According to your comment, I made a few changes to the code:

  • It won't alert on start-up, because there is only one alert per day.
  • log the alert for the specific day, so we can determine if we have already alerted or not on a specific day, even if the application has been closed and re-opened
  • add a little more documentation. If you prefer, I'm happy to add more.

other:

  • fixed the flaky test for test_retain_only_child_tags in python3.7 so it won't generate flaky test results
  • add additional info for developers on Arch-based distribution

@pbienst
Copy link
Contributor

pbienst commented Feb 26, 2019

I'm almost ready to commit this, but I have a few minor remarks:

  • could you move the functions that directly use sql code, like today_leared_fact_ids and warn_too_many cards to the sqlite files, so that the scheduler code only accesses the database through its API?
  • currently, all variables in mnemosyne make a distinction between external ids "id" and internal ids "_id", so, could you also do this consistently in your code? Consistent naming will help to prevent bugs.
  • I would replace today_learned_fact_ids with _fact_ids_learned_today

Thanks!

@woohgit
Copy link
Contributor Author

woohgit commented Feb 26, 2019

  • mnemosyne

Sure, no problem. I'll get back to you when the code is cleaned up.

- Move sql queries to the database class
- Add test coverage for the new database API functions
- adjusting the variable naming to comply with the current naming scheme
@woohgit
Copy link
Contributor Author

woohgit commented Mar 2, 2019

@pbienst I made some changes according to your comments.
I tried to keep the variable naming consistent and added test coverage to the new code.
I also fixed the 2 randomly failing (flaky) tests.

Let me know if you want me to change other things.

@pbienst
Copy link
Contributor

pbienst commented Mar 3, 2019

Thanks a lot!

@pbienst pbienst merged commit dd26b63 into mnemosyne-proj:master Mar 3, 2019
@woohgit woohgit deleted the remember-today-learned-count branch March 4, 2019 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants