anki-geo / ultimate-geography

Geography flashcard deck for Anki
https://ankiweb.net/shared/info/2109889812
Other
819 stars 81 forks source link

Interactive Map For Experimental Deck #644

Closed helitopia closed 3 months ago

helitopia commented 3 months ago

Original issue

Hi @aplaice, @axelboc

Getting back with the PoC discussed in the original issue, integrated into experimental deck.

Please be sure to read added parts in README.MD and CONTRIBUTING.MD before getting to review the PR as it will add clarity to the reasoning and structure of the changes. Also, most introduced changes are represented by meaningful and detailed commit messages which might help in understanding of the codebase (which might need to be squashed before the merge, it is up to you).

It was a top priority to follow both explicit and implicit project conventions, while it is acknowledged that some parts might have been missed, so the feedback is much appreciated.

There is an overwhelming amount of things discovered, ideas generated, questions raised during implementation of the change. All of them will later on appear for discussion in the original issue. The goal of this PR is to focus exlcusively on existing concepts and corresponding logic leading to release of minimal viable product to get the project started.

Over the course of working on the change a number of remarkable improvements over PoC were made. In particular:

Some notable implementation details:

helitopia commented 3 months ago

CONTRIBUTING.md

I think it'd be better if we had full stops at the ends of bullet points

I think we should expand the acronym IIFE (immediately invoked function expression).

Agreed.

_interactive_map_config.js

Why are the booleans strings in getUserConfig? Is it just to allow us to be liberal in what we accept (i.e. all of "True", true, "true", "TRue" etc. being accepted)?

Almost. String property is used so that typos (e.g. interactiveEnabled: "fasle") do not make program throw error, but rather default to starting value for the property (in contrast to interactiveEnabled: fasle that does throw an error and thus makes program default to static fallback altogether)

Also, maybe if we want the config to be more easily adjustable, by the user, we should move (the equivalent of) getUserConfig into the note template (Country - Map [Experimental].html).

Absolutely agree that it is easier for the user. Moreover I haven't yet figured out the issue with media folder update on AnkiDroid - it seems that the media is not always updated when a file is changed. You need to remove the file from media folder, then force sync on Anki Desktop and only then sync your AnkiDroid. Thus the configuration change may not be easily updated on other from main devices.

Funny enough the config was initially in the note template. But half through development I refactored it to a separate script as I expected you to be less inclined to accept a lot of stuff in the note template.

(This could be by, say, passing configObj to some function defined in _interactive_map_config.js.)

Though exactly this solution (not the whole idea of configuration in note template) is flawed. The configuration is loaded separately for both front and back sides. So with such option we'd need to write configuration twice in note template - for both card sides. And every time it would have to be updated twice which is fraught with troubles due to primitive human error. The way I did it (before moving configuration to media folder) is via sessionStorage.setItem() in note template and sessionStorage.getItem() in program code.

_jsvectormap.js

Where does _jsvectormap.js come from?

That's on me. Indeed it is essential to know the version and have attribution in LICENSE.MD.

_world.js

Ideally, we'd have instructions for how to generate _world.js as well, but I guess that that might be trickier.

This will be done in the next release. Right now it is skipped consciously as the instructions will quickly become deprecated.

Currently program uses pre-formed (taken from the web) SVG file that gets converted by an ad-hoc tool into JS object stored in _jsvectormap.js. So the workflow is SVG -> JS object The problem with this workflow is that SVG files are not easily editable in context of geographic data (e.g. island shape correction, disputed border creation). Thus, to make the process of map updating flexible and pretty straightforward, with the next release it was planned to complete the workflow to GeoJson -> SVG -> JS object (where GeoJson file can be imported in any online service (e.g. geojson.io) and adjusted, before being converted to SVG) and then write detailed instructions on how the process in performed.

Tests

We'll need a test (in .github/workflows/integrity-check.yml) that checks that pipenv run brain_brew run recipes/source_to_anki_\[experimental\].yaml runs successfully.

Good point.

Infrastructure

I don't think that the deck version numbering is affected by where the BrainBrew recipe is located, though.

I meant that right now experimental deck releases might be more frequent, so dividing setup into separate recipes makes sense to avoid generating regular and extended decks every time and vice versa

Also, regarding numbering, I'm not sure whether or not we should have separate numbering for the experimental deck — it depends on the length of the transition into the main deck. I think we have two main options:

  1. Use the same numbering as for the main decks, and if we have more frequent releases for the experimental deck use something like 5.2.1, 5.2.2 etc.
  2. Use 0.x for the first several experimental deck releases (and optionally migrate to the main deck versioning eventually (or not if we fully integrate everything before then).

The thing is I am not sure what is your plan on when (and if) experimental deck becomes "stable" enough. Will it be in such case merged into extended version? If so, then it makes sense for the second option (0.x) up until the merge (and usual versioning of extended deck afterwards).

If not, then still, it would probably be confusing to users when one deck variant has only version 5.2 and the other 5.2.1. Adding more to the confusion is whether content changes (2nd digit in version number update) should reset interactive map sub-version (3rd digit in version number). So whether content update in deck version 5.2.2 should produce 5.3.1 or 5.3.2. So, as for me, the option of versioning starting from 0.x and following the convention breaking change - ceiling(currentVersion) (e.g. 0.2 -> 1.0); non-breaking change - +0.1 (e.g. 0.2 -> 0.3) seems reasonable to consider.

Map colours

The difference between the "Country - Map" cards that are interactive on the back side and those that have a static map.

I would not worry about this issue too much as the plan is to eventually provide interactive map for every region (including water bodies). So, assuming the plan succeeds, the issue of discrepancy on the back side of a card between "correct interactive" and "correct static" is completely eliminated.

Although, the issue of discrepancy of coloring between front card side static map and back card side interactive map remains (albeit minor)

I suggest releasing it "as is" right now and after some time (or after the complete switch of back card side to interactive map) to gather user feedback (e.g. via Google Forms or Reddit post) for this and possibly other features we have concerns about / are interested in developing further.

The other option is to configure default setup so that both correctly and incorrectly selected regions are highlighted in red (i.e. static map red). And the user can make conscious decision to change configuration to highlight correctly selected region in green understanding the consequences (that is, possible confusion). Though this option assumes less benefit of the green highlight (thus disabled by default), than confusion, which might not be precise. IMO not optimal, we can go with the first solution and if the feedback of the majority is negative of this feature - only then employ this option.

Tell me what you think of it in case some changes in the codebase are needed.

Globally switch to green as highlight colour (in the static land maps).

Relatively simple to do — we have access to the original SVG images.

However, I'd be _very) hesitant, since I'm not sure if it would look nice (the Wikimedia contributors from the Map Workshop chose the colour scheme for a reason and I generally trust their taste/choices).

Agreed - changing static maps to green is both questionable and requires a bit of work.

Minor annoyances/thoughts

  • The zoom resets (i.e. you abruptly zoom out and then zoom in) when switching to the back card.
  • In exchange for the interactive map we lose the "disputed borders/territories" information etc.

These are partially the issues that motivated the suggestion of creation of separate jsvectormap library fork (in case original library maintainer not responding to PRs) - mentioned in one of the original issue's comments:

As an aside, the library lacks a lot of useful features / contains certain bugs.

Since the library is intended to be stored locally (including SVG maps) one of the solutions is to include a fork (instead of master version) with agreed critically needed features/bug fixes added (if any) in case of their delayed acceptance into original codebase. The idea is further solidified by the next section.

The work on them will be started in the order of their priority out of all the remaining items.

The Mercator projection (which I think is what's used now) isn't great. The Winkel Tripel that we use for the static maps might have been better, but it's also not great away from the central longitude (e.g. the Americas/Oceania for 15 °). Switching the central longitude depending on country would provide hints...

You mean Mercator projection's downside of size and distance distortion? If so, yes, it exists. I've seen in the library codebase options for other projections. Eventually will have look at it, but for now it is low on priority list.

README.md

I think we should have some sort of description of the interactivity

Agreed, user guide as a separate section of README.MD with images and GIFs to interest the users and explain how everything works would be great.

in either the README or the deck description

Why not have in both ;)

Naming

To avoid file-collisions (as unlikely as they are), I think we should prefix the _... files with _ug (e.g. _ug-world.js etc.)?

Great catch, haven't thought about it before.

Note models

The UUID in src/note_models/Ultimate_Geography_[Experimental].yaml should be different from that in src/note_models/Ultimate_Geography_[Extended].yaml.

I see that UUID resembles MD5 hash with hyphens in certain places. Would it be okay to set UUID of src/note_models/Ultimate_Geography_[Experimental].yaml to MD5 hash of deck name with replicated hyphens in corresponding places? (i.e. Ultimate Geography [Experimental] - 2e98b530-7583-5551-1fd7-51e6969d58ed)

What about headers_from_yaml_part.crowdanki_uuid in recipes/source_to_anki_experimental.yaml?

By the way, I am not sure whether the process of upgrading from regular/extended deck to experimental is straightforward. Do we need separate instruction list for it?

Squash vs. merge

No strong opinion, here. Your commits seem to be clean (we sometimes have contributions that don't, so squashing is useful), with extensive descriptions, so it might be worth keeping the history in the repo.

Good to know as commits are expected to be indeed helpful to understand the reasoning of the written logic.

To summarize:

Changes to be included in the current release (before this PR merge):

Changes to be included in the next release(s):

Please be free to remind me of any items I have missed / forgotten

aplaice commented 3 months ago

Thanks very much for the reply! (For brevity, I've mostly skipped the parts where I was just writing "I agree"/"Makes sense" with no further comment :))

Almost. String property is used so that typos (e.g. interactiveEnabled: "fasle") do not make program throw error, but rather default to starting value for the property (in contrast to interactiveEnabled: fasle that does throw an error and thus makes program default to static fallback altogether)

That makes sense and is a great idea! (Making the parts of the code intended to be adjusted by the user less brittle.)

Funny enough the config was initially in the note template. But half through development I refactored it to a separate script as I expected you to be less inclined to accept a lot of stuff in the note template.

Could we just move the parts that are really intended to be configured (i.e. probably just getUserConfig (or equivalent) without the "plumbing"), to avoid overloading the user/make it less likely they'll edit something they shouldn't? (But if that'd overcomplicate things, placing all of what's now _interactive_map_config.js in the note template would also be OK, I think.)

Though exactly this solution (not the whole idea of configuration in note template) is flawed. The configuration is loaded separately for both front and back sides. So with such option we'd need to write configuration twice in note template - for both card sides. And every time it would have to be updated twice which is fraught with troubles due to primitive human error. The way I did it (before moving configuration to media folder) is via sessionStorage.setItem() in note template and sessionStorage.getItem() in program code.

Thanks for explaining! (I suspect that one could probably work around this using {{FrontSide}} (to include the front side on the back side, automatically), with some "magic" to hide the elements that aren't necessary, and adjust those that need to be changed but it's more complicated than your approach.)

Versions and releases

I meant that right now experimental deck releases might be more frequent, so dividing setup into separate recipes makes sense to avoid generating regular and extended decks every time and vice versa

Ah, yes, you're right!

The thing is I am not sure what is your plan on when (and if) experimental deck becomes "stable" enough. Will it be in such case merged into extended version?

Yes, that's the intention! (With possibly a fully-static variant deck — i.e. the interactive deck would become the "main" extended deck and the current extended version would become a variant.)

If not, then still, it would probably be confusing to users when one deck variant has only version 5.2 and the other 5.2.1. Adding more to the confusion is whether content changes (2nd digit in version number update) should reset interactive map sub-version (3rd digit in version number). So whether content update in deck version 5.2.2 should produce 5.3.1 or 5.3.2. So, as for me, the option of versioning starting from 0.x and following the convention breaking change - ceiling(currentVersion) (e.g. 0.2 -> 1.0); non-breaking change - +0.1 (e.g. 0.2 -> 0.3) seems reasonable to consider.

I was more thinking about the potential longer-term case if we don't merge and we have both "content" changes (new notes/languages etc.), and changes to the interactive map, and users of the interactive deck might want to know off which "main" deck version the interactive version is based.

However, we can worry about this in the future (switching from 0.x to 5.2 etc. is straightforward).

Map colours

I suggest releasing it "as is" right now and after some time (or after the complete switch of back card side to interactive map) to gather user feedback (e.g. via Google Forms or Reddit post) for this and possibly other features we have concerns about / are interested in developing further.

Yes, definitely, IMO we should ask for feedback asap.

The other option is to configure default setup so that both correctly and incorrectly selected regions are highlighted in red (i.e. static map red). And the user can make conscious decision to change configuration to highlight correctly selected region in green understanding the consequences (that is, possible confusion). Though this option assumes less benefit of the green highlight (thus disabled by default), than confusion, which might not be precise. IMO not optimal, we can go with the first solution and if the feedback of the majority is negative of this feature - only then employ this option.

I agree that having colour-coding of correctness of response is preferable.


Minor annoyances/thoughts

You mean Mercator projection's downside of size and distance distortion?

Yes, exactly.

If so, yes, it exists. I've seen in the library codebase options for other projections. Eventually will have look at it, but for now it is low on priority list.

I expect that it should be relatively straightforward (a matter of choosing a projection when converting from GeoJSON to SVG), but I agree that it's low priority for now.

Note models

I see that UUID resembles MD5 hash with hyphens in certain places. Would it be okay to set UUID of src/note_models/Ultimate_Geography_[Experimental].yaml to MD5 hash of deck name with replicated hyphens in corresponding places? (i.e. Ultimate Geography [Experimental] - 2e98b530-7583-5551-1fd7-51e6969d58ed)

That would work (but so would any (semi-)random UUID)!

What about headers_from_yaml_part.crowdanki_uuid in recipes/source_to_anki_experimental.yaml?

I think that they should stay the same as those in recipes/source_to_anki.yaml (we haven't yet changed the deck description* or the deck name, so the "deck object" as such, should stay the same (they're also the same for standard vs. extended)).

* if we do have (a) separate deck description(s) for the interactive deck, then these UUIDs should also be changed.

By the way, I am not sure whether the process of upgrading from regular/extended deck to experimental is straightforward. Do we need separate instruction list for it?

It's pretty much the same as for upgrading from standard to extended:

https://github.com/anki-geo/ultimate-geography/blob/master/README.md#levelling-up-from-standard-to-extended

i.e. for upgrading from standard to interactive you'll have to make sure to switch the order of cards (like for standard->extended), and for extended->interactive you won't need to do anything. (I've just tested extended->interactive, and it does match expectations.)

However, some instructions might be useful — possibly a set of tables in the AUG Wiki, along the lines of:

## Standard -> Interactive ### Cards | Standard | Interactive | | --------------- | ------------------ | | Country - Capital | Country - Capital | | Capital - Country | Capital - Country | | Flag - Country | **Flag - Country** | | Map - Country | **Map - Country** | where bold means that that value has to be adjusted. ### Notes ...

?

(This can wait for after the merge, though — the (expected) upgrade path from Extended to Interactive/Experimental is pretty much frictionless, so a guide isn't essential.)

Squash vs. merge

Good to know as commits are expected to be indeed helpful to understand the reasoning of the written logic.

Most people don't write helpful commit messages, so "expected" is too strong a word: rather "very gratefully received". :)


To summarize:

Changes to be included in the current release (before this PR merge):

  • jsvectormap stylesheet import directly in the note template
  • full stops, IIFE clarification in docs
  • config move / duplication into note template
  • _jsvectormap.js version and license attibution
  • pipenv run brain_brew run recipes/source_to_anki_\[experimental\].yaml check
  • name collision avoidance via file renaming
  • user guide in README.md and deck description
  • UUID change in note model

Yes, I agree!

Changes to be included in the next release(s):

  • _world.js formation instructions
  • zoom resets library fix
  • disputed borders / territories fix
  • available map projections analysis

Yeah, these can wait!


User-testing

One slight issue (I've only now realised) in terms of user-testing is that BrainBrew/CrowdAnki/AUG intentionally work in the way that changing/switching note templates results in the user's existing notes and cards being updated to the new templates, including the user's progress.

For actually upgrading (as opposed to testing), this is very much a good thing, since the user likely doesn't want to start from scratch.

However, for testing this unfortunately means that the most obvious approach of importing into one's current Anki profile, testing and deleting the newly imported cards, does not work.

Testing in a new profile obviously does work. However, this is tricky when one wants to test on mobile — and we do want third-party mobile testing, for iOS in particular!

Upgrading and then downgrading (i.e. import interactive and then import extended) also works (in that there's no data loss/progress loss, unless the user had edited their notes/note templates, in which case they do lose their edit (but they'd lose them anyway when upgrading)). However, this is finicky (multiple imports/multiple potentially scary pop-ups) and (if the user mostly has mature cards) doesn't give easy access to testing the cards of interest (they have to filter for card:"Country - Map" and uncheck "Reschedule cards based on my answers in this deck").

One idea might be to create a purely "testing" deck where all the notes would have different GUIDs, so the user could safely import, test and delete the "testing" deck. This can wait for after merging, though (these changes shouldn't be in the committed code). (It should be a matter of clearing the GUIDs in src/data/guid.csv (BrainBrew will regenerate them with random values), changing the deck names and UUIDs in recipes/source_to_anki_experimental.yaml, generating the Experimental decks and then cleaning out the unstaged changes. (Since this would be purely for testing, the GUIDs/UUIDs don't have to be persistent.))

axelboc commented 3 months ago

Documentation

I think we should keep documentation to a minimum while the deck is still experimental to avoid unnecessary PRs. Notably, I'm wondering whether the proposed changes to CONTRIBUTING should be moved into a wiki page so we can edit them more freely. This way, we would have everything in one place: file structure, configuration, colour choices, but also how to beta test as suggested by @aplaice.

Very minor, but I find that some implementation details, like the IIFE, would be better off as comments in the code to avoid cluttering the main documentation.

In the README, I would not put much more for now for the same reason. Given that we've had a few users not finding info on the extended version, I'm thinking it might be good to have a dedicated sub-section for it under Features. We could then have a paragraph describing the experimental deck with a link to the wiki page. Doesn't have to be done in this PR though; the current sentence can be merged as is, I think.

Map colours

One aspect we haven't discussed yet is accessibility, and more specifically colour blindness. In fact, red/green is a well-known troublesome combination. I reckon we should keep the same red tint as the static maps for "correct answer" and find some sort of distinctive colour + pattern combination for "wrong answer".

For the colour, maybe a shade of purple? For the pattern, I'm thinking maybe dots, crosses or zigzags, since diagonal bars are already used for disputed territories. Here's a tool that can generate a CSS-only zigzag pattern, for instance:

image

Configuration

I agree with @aplaice about moving the user-facing config into the template. Not sure I understood the double-load issue but local storage seems like a good solution and hiding this complexity away into a defineConfig function of sorts seems clean-enough.

Not sure about the boolean strings. After all, it doesn't really prevent users from typing fasle without quotes... Personally, when I make a typo in my code, I prefer when things crash early and completely, so I have no problem with the described behaviour when mistyping a boolean in the configuration object. It's the risk we take from exposing configuration to users, but I think that we will actually reduce the risk of users making mistakes by avoiding hidden parsing logic and sticking with primitive booleans.

Caching files in media folder

Moreover I haven't yet figured out the issue with media folder update on AnkiDroid - it seems that the media is not always updated when a file is changed. You need to remove the file from media folder, then force sync on Anki Desktop and only then sync your AnkiDroid. Thus the configuration change may not be easily updated on other from main devices.

This is quite a problem if it affects end users. The easiest solution I can think of is a web development technique called cache busting, which consists in appending a hash of the file's content to its filename – e.g. _interactive_map_init_2d7fe.js.

Roadmap

Here's what I'm thinking:

  1. Fix the important changes discussed (UUIDs, colours, tests, etc.)
  2. Merge this brilliant MVP.
  3. Let repo contributors and "watchers" test the new deck and report issues.
  4. Increment and improve things as discussed (projection, zoom reset, etc.) and according to contributors/watchers feedback.
  5. Find more beta testers amongst power users — I think the best way would be to publish a release of the main deck with a section dedicated to the experimental deck in the release notes and in the deck description (with links to the wiki page).
  6. After a few months and once we're happy with the state of development, make the deck stable and publish a new release — might be a good idea to keep a recipe to generate the old extended deck (and a mention of how to generate it in the README).
axelboc commented 3 months ago

I've rebased from the GitHub UI after merging #639, I hope it won't cause an issue. I've also edited my previous comment with an idea to solve the caching issue with files in media folder.

helitopia commented 3 months ago

Hi @aplaice, @axelboc

Getting back with the subset of the agreed changes. Here is a brief summary (see commits for details):

Done:

Skipped:

First of all, I would like to thank you all for your time and effort in reading, reviewing and discussing all the details. I appreciate the enormous support!

Now to the comments.

@aplaice:

_interactive_map_config.js

Could we just move the parts that are really intended to be configured (i.e. probably just getUserConfig (or equivalent) without the "plumbing")

Of course, none of the "plumbing" is included.

Versions and releases

Yes, that's the intention! (With possibly a fully-static variant deck — i.e. the interactive deck would become the "main" extended deck and the current extended version would become a variant.)

I suppose the plan is to gradually remove static images from experimental deck when it becomes "stable"? I may miss some details, but leaving static images as fallback in experimental deck and then "stabilizing" it (i.e. making it new extended) makes the deck everything the extended one now represents + interactive map. So why exactly having "old extended" one as a variant?

I was more thinking about the potential longer-term case if we don't merge and we have both "content" changes (new notes/languages etc.), and changes to the interactive map, and users of the interactive deck might want to know off which "main" deck version the interactive version is based.

Thank you for clarifying. Now that I think of it again the proposed versioning system actually makes sense.

User-testing

However, this is tricky when one wants to test on mobile — and we do want third-party mobile testing, for iOS in particular!

One other option that I used when testing on AnkiDroid is to create separate AnkiWeb account, link it to testing Anki Desktop profile and then sync AnkiDroid with the account (by force downloading the changes). Such solution is basically an ad-hoc AnkiDroid profile change. Though not sure whether possible on AnkiMobile.

@axelboc:

Documentation

I think we should keep documentation to a minimum while the deck is still experimental to avoid unnecessary PRs. Notably, I'm wondering whether the proposed changes to CONTRIBUTING should be moved into a wiki page so we can edit them more freely. This way, we would have everything in one place: file structure, configuration, color choices, but also how to beta test as suggested by @aplaice.

Moving to wiki is a great solution for PRs minimization as documentation for the interactive map is expected to be updated frequently, with each incremental improvement.

Also, I have quite an exhaustive smoke testing checklist, which I followed before creating PR (and then after introducing minor changes) that might be valuable to place into wiki as well (due to gradually increasing complexity of program logic the need for some validation arises).

Very minor, but I find that some implementation details, like the IIFE, would be better off as comments in the code to avoid cluttering the main documentation.

Most definitely agree as I imagine it to be easier to have the explanation exactly in place that it clarifies.

Given that we've had a few users not finding info on the extended version, I'm thinking it might be good to have a dedicated sub-section for it under Features. We could then have a paragraph describing the experimental deck with a link to the wiki page.

If I understand correctly the idea is about having "teasers" of extended and experimental decks in "Features" section with wiki links for advanced details, right?

Map colors

One aspect we haven't discussed yet is accessibility, and more specifically color blindness. In fact, red/green is a well-known troublesome combination.

To be honest it was the reason of my initial motivation to move color properties into user configuration - so that users with special needs are able to configure the colors correspondingly (and those who want deeper customization as well).

I reckon we should keep the same red tint as the static maps for "correct answer" and find some sort of distinctive color + pattern combination for "wrong answer".

For the color, maybe a shade of purple? For the pattern, I'm thinking maybe dots, crosses or zigzags, since diagonal bars are already used for disputed territories.

This is brilliant idea (aside from the necessity to introduce appropriate logic into underlying library)! Maybe it is even unnecessary to deviate from current static map style - and simply use red tint + pattern combination for "wrong answer"?

Configuration

Not sure I understood the double-load issue but local storage seems like a good solution and hiding this complexity away into a defineConfig function of sorts seems clean-enough.

That is on me, missed to provide one important detail. AnkiDroid uses separate global scopes for question and answer card sides, meaning that moving the getUserConfig() function into <script> element of the note template as follows (i.e. into question card side):

<script>
  function getUserConfig() {
    return {
      commonFeatures: {
        interactiveEnabled: "true",
        ...
  }
</script>
<div class="value value--top">{{Country}}</div>
<hr>
...

--

<div class="value value--top">{{Country}}</div>
...

won't work as the function is not visible to the back card side global scope. Thus the notion of doubling configuration:

<script>
  function getUserConfig() {
        ...
  }
</script>
<div class="value value--top">{{Country}}</div>
<hr>
...

--

<script>
  function getUserConfig() {
        ...
  }
</script>
<div class="value value--top">{{Country}}</div>
...

Aside from AnkiDroid concern this solution would have been the preferred one.

Not sure about the boolean strings. After all, it doesn't really prevent users from typing fasle without quotes... Personally, when I make a typo in my code, I prefer when things crash early and completely, so I have no problem with the described behaviour when mistyping a boolean in the configuration object. It's the risk we take from exposing configuration to users, but I think that we will actually reduce the risk of users making mistakes by avoiding hidden parsing logic and sticking with primitive booleans.

As described in CONTRIBUTING.MD, the initial implementation was intended to be fail-safe, thus such a reasoning. Although I agree that after writing proper documentation we may pass the responsibility of correct configuration onto the user. Plus it would remove those nasty filtering functions in _ug-interactive_map_config.js.

Caching files in media folder

This is quite a problem if it affects end users. The easiest solution I can think of is a web development technique called cache busting, which consists in appending a hash of the file's content to its filename – e.g. _interactive_map_init_2d7fe.js.

Yes, it is. Will keep in mind the mentioned technique. Although the disadvantage I see is that with time and a series of updates user media folder will accumulate significant amount of older unused files (which are not removed by "Check media" option, due to those files being considered intrinsic as they are prefixed with underscore). They do not weigh much (at least now), but still.

I am yet to investigate the logic of media update and possible options. Will get back with results later on.

Roadmap

Mostly agree with the proposed plan. Just a few questions:

  1. Let repo contributors and "watchers" test the new deck and report issues.

I assume this step does not include the official public release with announcement? It is more like a beta-testing stage for "closely involved" only?

might be a good idea to keep a recipe to generate the old extended deck (and a mention of how to generate it in the README).

Same question about preservation of static images in experimental deck after making it "new extended" one as to @aplaice above. Although from what you tell I figure that the "old extended" is planned to be discontinued from releases, while leaving an instruction to generate it in case some old school folks will want to.

josealberto4444 commented 3 months ago

I'm a bit late, but I'll write my opinion here regarding some topics discussed. If they are not still relevant, I'm sorry! =P

Map colours

Possible solutions:

1. Globally switch to green as highlight colour (in the static land maps).
   Relatively simple to do — we have access to the original SVG images.
   However, I'd be _very) hesitant, since I'm not sure if it would look nice (the Wikimedia contributors from the Map Workshop chose the colour scheme for a reason and I generally trust their taste/choices).

2. Chose another equally-intuitive colour scheme where red is correct and ??? is incorrect.  (No idea what...)

3. Use some other indicators of correct/incorrect (borders?? frames?? Again no idea that wouldn't look ugly...)

4. Switch back to using the "globe" icon for the notes where the back is static.
   IMO not a great idea as it provides a hint to the answer and as you noted, the interactive map is useful anyway.

Unless we have good ideas for 2 or 3 I think we should leave the green/red as they are.

Maybe black for incorrect and red for correct. Black is usually supposed to be worse than red, and by the time the user sees an interactive map, they should have seen a static Map - Country card and maybe associate red with correct. Just an idea, but I'm not sure about this at all. :3

One aspect we haven't discussed yet is accessibility, and more specifically colour blindness. In fact, red/green is a well-known troublesome combination. I reckon we should keep the same red tint as the static maps for "correct answer" and find some sort of distinctive colour + pattern combination for "wrong answer".

Black and red would work for this as well. In case of red-blindness, it'd be seen as black and some shade of grey (same color as the one seen in the Map - Country), and in case of green- or blue-blindness (or any combination) it doesn't change anything. =)

Squash vs. merge

No strong opinion, here. Your commits seem to be clean (we sometimes have contributions that don't, so squashing is useful), with extensive descriptions, so it might be worth keeping the history in the repo.

Same opinion here. I think that's the best advantage of using a VCS. ^^


Thanks for all the effort to you all! ^^

axelboc commented 3 months ago

If I understand correctly the idea is about having "teasers" of extended and experimental decks in "Features" section with wiki links for advanced details, right?

Yes, just a simple paragraph for each, extended and experimental, explaining what's different about them, maybe with some screenshots showing the extra templates in action, like for the standard deck.

Maybe it is even unnecessary to deviate from current static map style - and simply use red tint + pattern combination for "wrong answer"?

Worth a try! Same with @josealberto4444's suggestion of using black. The config will let us try different combinations easily, which is awesome.

Although the disadvantage I see is that with time and a series of updates user media folder will accumulate significant amount of older unused files (which are not removed by "Check media" option, due to those files being considered intrinsic as they are prefixed with underscore).

Oh yeah, that's not great if the files start to accumulate.

I assume this step does not include the official public release with announcement? It is more like a beta-testing stage for "closely involved" only?

Exactly, people who are subscribed to PR-related events and will know when this MVP is merged.

Same question about preservation of static images in experimental deck after making it "new extended" one as to @aplaice above. Although from what you tell I figure that the "old extended" is planned to be discontinued from releases, while leaving an instruction to generate it in case some old school folks will want to.

Exactly, it doesn't cost anything to keep the original Country - Map template along with a Brain Brew recipe. But the release notes won't include ZIP files for this "old extended" deck; only for the new, interactive one.

Concerning the static images, won't they have to stay anyway for the Map - Country template? Or were you thinking of putting an interactive map on the front side of this template as well?

helitopia commented 3 months ago

Force pushed suggested changes (instead of adding new commit with miscellaneous edits) to keep commit history cleaner.

Yet another option, perhaps cleaner, is to define the config as JSON directly in the template

This option appears the cleanest of the selection.

I think what @aplaice and I had in mind was to somehow hide away those two lines.

Is it worth implementing though? Hiding these two lines won't make much difference, as there are infinitely many ways to break the code, but adding this change will require additional hack in the back-end (config script):

// _ug-interactive_map_config.js
(function () {
  // Retrieve and store user config in `sessionStorage` if present (i.e. on front side only)
  const configElem = document.querySelector('#ugInteractiveMapConfig')
  if (configElem) {
      sessionStorage.setItem("userConfig", configElem.innerHTML);
  }

  function getConfig() {
    ... // read from `sessionStorage`
  }
}());

IMO status quo is the most concise it can be. (Edit: meaning balanced amount of code in both front template and configuration script)

Or there is some other significant reason to do so I am missing?

aplaice commented 3 months ago

Thanks everyone for the comments (sorry I was unavailable for a couple of days) and above all thanks so much @helitopia for your continued updates!

IMO we should merge now to avoid bike-shedding! (@axelboc I leave the last word to you, as always, though! :))

How do we make the deck easiest for testing? Do we create a draft/beta release ("pre-release") (with just the experimental deck (?))? (The main disadvantage is excessive notification for people watching releases but not other activity. OTOH forcing people to build the deck themselves is a barrier to testing. We sort-of have a rudimentary quick-release system here with artifacts published here, but it's not in the main repo due to converns about CI having access to the "write repo" permission.)


Bikeshedding below :)

Colours/patterns

Fix the important changes discussed ([...], colours, [...], etc.)

I fully agree with @axelboc that we should care about accessibility and all the ideas (red vs. black, red vs. zigzag purple, red vs. zigzag red) are great, but I don't think we've managed to settle on anything, so maybe let's just stick with red/green for now, with the plan that we will switch to something colour-blind-friendly in the next beta, and in the meantime we (and any other testers) can all play around with all the alternatives?

Versions and releases

So why exactly having "old extended" one as a variant?

My main motivations was:

  1. For people stuck on old versions of Anki(Droid|Mobile) which might malfunction even with intended graceful degradation.

  2. For people who don't fully trust us. IMO the fully local approach we have here is secure, trustworthy and verifiable. AUG also generally has a positive reputation, so most people will likely just take our word for it, and those that don't could verify that the deck does what it's supposed to. However, at the end of the day, from the point of our users, we are random people on the internet and most people won't have the wherewithal to inspect/understand the code. In contrast, checking that the deck just contains image files is a far lower burden. (Obviously, image libraries also have bugs, but JS exploits are far more common and even without a sandbox escape, malicious JS can do some (limited) damage.)

(2 is more important than 1.)

(This is a decision for the future, though, and it wouldn't be just up to me to decide :))

(It also depends on how many variants we have, and how we display them in terms of releases — having them on the main release pages might end up overwhelming.)

(If we end up with many variants we might want to have some sort of simple github pages webpage which would allow users to pick version, language, variant etc.)

User-testing

One other option that I used when testing on AnkiDroid is to create separate AnkiWeb account, link it to testing Anki Desktop profile and then sync AnkiDroid with the account (by force downloading the changes). Such solution is basically an ad-hoc AnkiDroid profile change.

That works, but it's not very convenient, though :(

Though not sure whether possible on AnkiMobile.

Actually, I've just now realised that AnkiMobile (even on very old versions!) allows for multiple profiles, out-of-the-box! Sorry for not remembering earlier!

Minimising config code in template

IMO status quo is the most concise it can be.

I think the key goal was making the template (as opposed to the back-end _ug-interactive_map_config.js code) as concise as possible, but OTOH you are indeed right that there are still "infinitely many ways to break the code" :) even if it's just JSON, so it might not be worth fiddling with this further, especially given that you've tested the current version, that the JS code isn't the only part of the template that might be confusing to a layperson (even the current extended/standard templates contain such bits) and that we don't want to bikeshed. :)

axelboc commented 3 months ago

IMO we should merge now to avoid bike-shedding! (@axelboc I leave the last word to you, as always, though! :))

The only remaining thing, if that's okay, would be to move the documentation out of CONTRIBUTING and into a wiki page for the time being, and to link to it from the README.

I've created a page and you should be able to edit it @helitopia.

helitopia commented 3 months ago

Again, force pushed for the sake of clarity of commit history. Sorry if it makes review more difficult.

For now linked documentation page via experimental deck mention in README, but, as was discussed, later on it is probably a good idea to create "teasers" of both extended and experimental decks in Features section with appropriate links to documentation for further study.

axelboc commented 3 months ago

Excellent, thanks!

aplaice commented 3 months ago

I've added a page to the Wiki describing switching deck/note type:

https://github.com/anki-geo/ultimate-geography/wiki/Switching-deck-type#specific-guides

I hope it's clear! (The descriptive section is probably unnecessary — I've moved it to the bottom, so that people who are interested can read it, but it doesn't distract those who aren't.)


I've also updated the workflow that publishes built decks based on the latest commit to also publish the experimental decks, here. (I'm not convinced that we shouldn't just do a normal draft release, though.)

axelboc commented 3 months ago

Fantastic work @aplaice, this will be super helpful, I think! Feel free to link to it from the README or anywhere else of course!

helitopia commented 3 months ago

I've also updated the workflow that publishes built decks based on the latest commit to also publish the experimental decks, here.

Well done!

One idea might be to create a purely "testing" deck where all the notes would have different GUIDs, so the user could safely import, test and delete the "testing" deck.

Is it about time to do so? Or since you have written a tabular guide for all combinations of upgrading/downgrading this would be unnecessary? @aplaice

aplaice commented 3 months ago

Temporary decks

Is it about time to do so? Or since you have written a tabular guide for all combinations of upgrading/downgrading this would be unnecessary? @aplaice

I'm not sure. The instructions should hopefully make the process smoother, but people might still not want to play around with their main AUG deck in case they mess up the review history (if they take care, then this shouldn't be an issue, but still).

Using separate profiles for testing is probably the sanest approach, but it's tricky on AnkiDroid. (It does work on both Anki desktop and AnkiMobile, though.) (On Android, you can clear everything out, sync with a different account, and then sync with your original account, but if you have a large collection that's rather slow. I believe you can also use parallel AnkiDroid builds to simulate profiles, but it's finicky.)

In case we do want temporary decks, the following bash script will generate them:

#!/bin/sh

# We can't do anything smarter here (we have to insert the right
# number of commas ourselves), since some guids contain commas.
sed '2,$ s/,.*/,,,,,,,,,,,,,/g'  -i src/data/guid.csv

# The following is inefficient (rewriting the same file n times) but results in cleaner-looking code.
# We can't just use one uuidgen invocation with a more general regexp, because we'd have the same UUID for all 4 decks.
for uuid in 43c5ba66-9a65-11e8-90c9-a0481cc15658 cb4d32ee-12ed-9960-1841-28c09449ded0 75bfcdb5-0ff3-4038-83cb-3e6ed974f439 6c995ee1-4b62-4019-a033-de0ef8651c83
do
    sed 's/crowdanki_uuid: '"$uuid"'/crowdanki_uuid: '"$(uuidgen -r)"/ -i recipes/source_to_anki_\[experimental\].yaml 
done

sed -r 's/name: Ultimate Geography( \[[A-Z][A-Z]\]|)$/name: Ultimate Geography\1 experimental interactive/' -i recipes/source_to_anki_\[experimental\].yaml

pipenv run build_experimental

Patterns in regions

Unfortunately, I haven't found a simple way to add patterns (zig-zags etc.) to highlight selected regions. The generated map is an SVG so CSS applies differently to it than to normal HTML (i.e. @axelboc's site regrettably won't work out of the box).

AFAICT the normal way of adding more complex styling (anything more complicated than a single colour, is by using fill: url(OPTIONAL_URI_THEORETICALLY_POSSIBLE#myId) with a corresponding definition elsewhere (e.g. <linearGradient id="myId">...</linearGradient>). In principle, OPTIONAL_URI_THEORETICALLY_POSSIBLE could be anything (even a remote webpage), but no web engine seems to support that (and Anki definitely doesn't), and it appears that in practice, the reference has to be within the same SVG element. (Neither referring to a different file in the same directory (fill: url(foo.svg#myId)) nor even referring to a different SVG element in the same HTML page (having another SVG element in our Country - Map template), works). (Not even using a data:... URI seems to work...)

This means that I think we'd have to inject the pattern definition into the SVG element generated by jsvectormap, (and refer to it simply via fill: url(#myId)) which should be doable, but is also rather hacky... (I haven't looked at the internals of jsvectormap, so I might be wrong about this being the only approach.)

helitopia commented 3 months ago

This means that I think we'd have to inject the pattern definition into the SVG element generated by jsvectormap

Thank you for the investigation! As I mentioned in the comment:

This is brilliant idea (aside from the necessity to introduce appropriate logic into underlying library)

I assume that the ability to set selected region highlight to a pattern is a useful feature so it is a good idea to mainstream (or include in the fork if maintainer is not reachable) implemented functionality into jsvectormap codebase, instead of creating ad hoc solution in our codebase.

But before actually getting to the proper implementation I think ad hoc solution you described will still be required first just to create a PoC of pattern highlight feature (instead of solid color) so that we are able to gauge the applicability of the idea (i.e. if it looks fine on the map).

aplaice commented 3 months ago

For now, I've opened a discussion here:

https://github.com/anki-geo/ultimate-geography/discussions/649

Please let me know if something is missing/wrong/too verbose! (@axelboc/anyone else with admin rights also please feel free to edit directly.)

I've arbitrarily (for ease of differentiation) named the deck v0.0.

I used a "parallel" deck (with separate GUIDs/UUIDs) — it doesn't really hurt unless somebody wants to immediately upgrade to the new interactive version (and if people do want that then we should probably just do a release).

We could also create a release, but maybe for the next iteration? (It seemed we couldn't really reach consensus on whether we wanted one now.)

helitopia commented 3 months ago

Amazing description, thank you!

but maybe for the next iteration?

By the way, I am currently working on summarizing and describing the remaining items on the agenda, so will come back with updates in the original issue soon.

I used a "parallel" deck (with separate GUIDs/UUIDs) — it doesn't really hurt unless somebody wants to immediately upgrade to the new interactive version (and if people do want that then we should probably just do a release).

Nevertheless, for now, I think it would be great to include a link (or artifact directly) to the "non-parallel" deck (with the same GUIDs/UUIDs) so that those who want can easily upgrade from the discussion page.

Moreover, IMO (although I am in no way pushing it) those who upgrade are potentially more likely to discover issues / have ideas as they will consistently use the map in contrast to importing the deck separately, playing around with it for a day and forgetting about it (or removing altogether).