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

Add time spent on heatmap #62

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AdrienLemaire
Copy link

@AdrienLemaire AdrienLemaire commented Mar 6, 2020

Description

This PR follows the request of a redditor here: https://www.reddit.com/r/Anki/comments/fdsgfm/celebrating_1_year_of_anki/fjjxiaj/

It adds the "Total time spent" and "Daily time average" stats below the heatmap.

Checklist:

Please replace the space inside the brackets with an x and fill out the ellipses if the following items apply:

  • I've read and understood the contribution guidelines
    Fyi, did you know that Black recommends 88 chars over 79? https://github.com/psf/black#line-length. In addition, if you add a pyproject.toml file, your conventions will take priority over developers' own defaults when they edit your files, which saves everybody some time.
  • I've tested my changes against at least one of the following Anki builds:
    • Latest standard Anki 2.1 binary build [required for Anki-compatible 2.1 add-ons]
    • Latest alternative Anki 2.1 binary build
    • Latest Anki 2.0 binary build [required for Anki 2.0-compatible add-ons]
  • I've tested my changes on at least one of the following platforms:
    • Linux, version: Arch linux, linux 5.5.7-arch1-1
    • Windows, version:
    • macOS, version:
  • My changes potentially affect non-desktop platforms, of which I've tested:
    • AnkiMobile, version:
    • AnkiDroid, version:
    • AnkiWeb

Copy link
Owner

@glutanimate glutanimate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I could see this being a cool addition for some users! Only left a small comment for now, but will be looking this over more thoroughly soon.

Though more generally, I'm not sure if I want to add too many stats to Review Heatmap. My mission goal with the add-on definitely was to distill Anki's (otherwise fairly complicated) stats to a number of key metrics that can act as motivators.

So if we were to add new stats like these, I think it would have to be as toggle-able options. I really don't want to overwhelm users with too many stats on their home screen (and that goes especially for anything related to time spent, since Anki's way of tracking review time is renown for confusing users).

Also, to be honest, with 0.7.0 still in beta, it might be best to relegate new feature additions like these until the release has stabilized. There are still a number of core issues to fix in the date/time-related code, and I'm a bit wary about introducing any additional features until that is done. I'll have to think about it for a bit.

However, another option could be to introduce some kind of hook that would allow other add-on authors to extend the stats section with their content (maybe as a label, value, scale tuple). We'd have to think about what data to pass on to the hook for other add-ons to make the best out of it, but this might be more feasible and faster to do in the current state of the add-on than feature additions.

(and since there are likely a myriad of interesting conclusions to draw from Anki's stats, this might be the best way forward rather than adding more and more options to Review Heatmap)

zip(
(0, DAY, 3 * DAY, 10 * DAY, 30 * DAY, 100 * DAY),
compress_levels(css_colors, (0, 2, 4, 6, 9, 10)),
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there's a closing ) missing here, which currently causes:

When loading '⁨review_heatmap⁩':
⁨Traceback (most recent call last):
  File "/home/ari/Projects/AnkiDesktop21/qt/aqt/addons.py", line 208, in loadAddons
    __import__(addon.dir_name)
  File "/home/ari/Tests/Anki/review-heatmap/addons21/review_heatmap/__init__.py", line 118, in <module>
    initializeAddon()
  File "/home/ari/Tests/Anki/review-heatmap/addons21/review_heatmap/__init__.py", line 109, in initializeAddon
    from .views import initializeViews
  File "/home/ari/Tests/Anki/review-heatmap/addons21/review_heatmap/views.py", line 53, in <module>
    from .heatmap import HeatmapCreator
  File "/home/ari/Tests/Anki/review-heatmap/addons21/review_heatmap/heatmap.py", line 85
    }
    ^
SyntaxError: closing parenthesis '}' does not match opening parenthesis '(' on line 79

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry about that. I had customized an earlier version of the add-on, and had to deal with a number of conflicts. Didn't properly test this version.

@AdrienLemaire
Copy link
Author

@glutanimate totally agree with you. I only made the PR to help others who are interested in this data, but didn't really expect that you'd merge it :) Toggable and hookable options are definitely the way to go for flexibility and extendability purposes! But I don't have time to work on this, sorry about that :)

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.

2 participants