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

Interactive Map For Experimental Deck #644

Merged
merged 45 commits into from
Aug 17, 2024

Conversation

helitopia
Copy link
Collaborator

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:

  • Higher-level detailization map - while still having its flaws this change is a substantial factor in allowing to include more regions than it was initially agreed to (while also providing better and smoother UX)
  • Configuration flexibility - both for interactive map itself and its color palette
  • Logic robustness and maintainability - both achieved with the help of code being factored out into separate scripts residing in media folder instead of being inside <script> elements of the note template

Some notable implementation details:

  • Region inclusion - initially it was agreed to start with Europe. But, as mentioned above, higher-level map detailization allowed for broader range addition. Thus, all the UN regions with land mass large enough to be noticed by the user at current maximum zoom level are included (Barbados is used as the benchmark)
  • Border width - the discussed extensively large map border width issue ended up as the issue of SVG regions being defined too far from each other. Thus it was a choice between broad borders or thin borders and remaining gaps between them. The newly introduced map resolved the issue altogether
  • Performance - higher-level detailization map slightly reduces performance by bringing delay between user event and action execution. The decrease will most likely go unnoticed by those who have not used previous low-detail map and is surely not critical, but this aspect should be taken into account in case of decision to move to even more detailed map (more on this in the original issue afterwards)
  • Interactive map display - it was thought to be reasonable to always display interactive map on the question side of a card (unless specified by configuration otherwise) due to it ultimately providing more help to the user than default globe SVG icon
  • Infrastructure - as the existing source_to_anki.yaml recipe being already complex enough, the infrastructure part of the experimental deck creation was written as a separate configuration - source_to_anki_[experimental].yaml. The point is reinforced by the fact that the deck version numbering does not match its counterparts and persistent existence of the deck being under question altogether
  • Styling - to avoid stylesheet duplication which may be fraught with maintenance burden, the already existing stylesheet was reused. @import at-rule for underlying library is ignored when non-experimental deck makes use of the stylesheet, so there should be no issue with it. Although now writing the description I understand that I could make use of <style> element to import the stylesheet directly in the note template instead of using the at-rule - and probably will do so eventually (either with next release or with corrections of this PR, if there will be any)

Copy link
Collaborator

@aplaice aplaice left a comment

Choose a reason for hiding this comment

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

This looks amazing! There are a couple of minor issues that should be resolved and a couple of others that might be worth thinking about, but I think we should merge asap to allow people to play around with it!

CONTRIBUTING.md

Nitpicks:

I think it'd be better if we had full stops at the ends of bullet points, at least when these are long/full sentences (where there's a mix of short/long, I think we should default to long for consistency). AFAICT for short bullet points, we've been following the convention of having a full stop at the end of the list (last bullet point), until now, but I don't have a strong opinion either way. (Sorry — the lack of full stops was weirdly distracting when reading!)

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

_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)?

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). (This could be by, say, passing configObj to some function defined in _interactive_map_config.js.) (I think it's easier for a typical user to change the note template from within Anki rather than opening _interactive_map_config.js in a text editor, but it might not be worth the refactoring.)

_jsvectormap.js

Where does _jsvectormap.js come from? (In that I know that it's from jsvectormap but how is it generated/which version is it (currently) based off/what licence is it under?)

The former is valuable for reproducibility/maintenance purposes (if you're busy someone else might need to adjust the maps, and besides you might forget things you did yourself (I know that I do :)) — this should probably go in CONTRIBUTING.md.

License-wise, we need this to satisfy the attribution requirements of the MIT license (probably should go at the end of 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.

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.

Infrastructure

Infrastructure - as the existing source_to_anki.yaml recipe being already complex enough, the infrastructure part of the experimental deck creation was written as a separate configuration - source_to_anki_[experimental].yaml.

I think that in the medium-term this is probably the right choice. In the longer term, it might be worth combining the two, to avoid duplication, despite the complexity.

The point is reinforced by the fact that the deck version numbering does not match its counterparts

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

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).

Map colours

The discrepancy between the colours in the interactive map and the static maps is unfortunate, though I can't think of a better approach.

Using red if the user clicks the wrong region and green if they click the correct one is the natural, obvious choice. Unfortunately (as you know), we currently use red as the main highlight colour in our maps (in line with Wikipedia's/Wikimedia's style of maps). This leads to three related issues:

  1. The change in style (old static maps vs. new interactive maps) for the fully interactive Country - Map cards might be very slightly confusing for current users (obviously not an issue for new users). (IMO minor — people can get used to this.)
  2. The difference in style for the "Country - Map" and "Map - Country" cards (the former uses the interactive maps and the latter the current static maps). (Also minor.)
  3. The difference between the "Country - Map" cards that are interactive on the back side and those that have a static map. Here, there's a risk that people will think (for the back-static cards) that because the highlight is in red, they had incorrectly clicked*. (We can mention this in the README or elsewhere, but most people won't read.)

* actually, in a very narrow sense they will have incorrectly clicked, since the correct region is not represented on the interactive map...

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.

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. (as we had already discussed in the original issue). Maybe (but probably not) we should have a way of also displaying the static image map. A toggle/checkbox of some sort to switch between the static and interactive maps would probably be not-too-difficult to implement, but it would be ugly and result in additional clutter, so probably not.

  • 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...

README.md

I think we should have some sort of description of the interactivity — particularly meaning of the colours (intuitive though they are) and the presence of the static fallback for dependent terrritories and the like — in either the README or the deck description (if we were to have a separate deck description for the experimental deck).

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.)?

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.

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.

@helitopia
Copy link
Collaborator Author

helitopia commented Aug 10, 2024

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):

  • 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

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

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

@aplaice
Copy link
Collaborator

aplaice commented Aug 12, 2024

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
Copy link
Collaborator

axelboc commented Aug 13, 2024

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).

helitopia and others added 26 commits August 13, 2024 14:36
- Note that this commit is atomic part of the previous one and the sole purpose of separation is readability of mainstream changes of the former
- For maximum flexibility introduced ability to disable correctly selected green region highlighting via card template
- JS code moved out into separate files for front and back card templates
- Each file is wrapped into IIFE to avoid re-declaring variables when switching between cards (Anki uses persistent webview)
- In case of interactive map trouble to prevent user from need to reimport normal UG deck implemented logic to make it possible to disable interactive map
- In case of being disabled it gets replaced with static fallback as if the deck is of Extended type
- Foreseeing possible issues with dragging compatibility on smartphones (e.g. accidental click on the region) implemented a switch to disable automatic card turn to answer side
- Defaulting to static fallback image when card has empty "Region code" field
- Switch from explicit constructors for type conversions to implicit idiomatic operator usages (e.g. +<string> to get number value)
- Prior to the change if something went wrong with script loading nothing would be shown to the user on question or answer
- Described commit implements defensive tactic so that if script fails to load static fallback is shown anyway
- To maintain flexibility status the described commit implements switch to control display of interactive map on mobile agents (including AnkiDroid and AnkiMobile)
- The use case of the option might be too small device dimensions or simple preference of static map when reviewing on smartphone
- Tooltip on answer card side is disabled by default as it is assumed to disrupt remembering process (e.g. first card you learn England, but select Germany and tooltip shows its name, then the next card is Germany and you will easily guess it)
- Mobile hack of hidden tooltip to make dragging work is removed as it seems that some other factor caused the dragging issue
- Merge scripts for both card sides with intention to factoring out common parts and reducing code size
- Improve code readability by moving out a lot of logic into separate methods, each having documentation written for it
- Move out the logic to separate isolated function
- Document the specifics of the moved function
- Minor unrelated code corrections
- Put all the configuration properties in common object
- Shorten constant object properties names
- One of the earlier commit erased the mobile hack for dragging on mobile to work presuming that the tooltip is not the cause of the issue
- The assumption was wrong and has arisen due to incorrect mobile media folder update
- The described commit reverts the hack removal and integrates it with tooltip switch to work correctly
- In order to reduce symbols and complexity of code - shorthand for sessionStorage is implemented to either retrieve string or boolean value. Appropriate changes are made to switch from implicit boolean casting to the new shorthand
- In order to aid readability, map initialization conditions are moved out to separate functions, each given descriptive name and documentation comment
- Prior to the change any positive integer yielded true
- Described commit correct function body to correspond to the behaviour written in function documentation comment
- In order to improve fault-tolerance of the program add a logic to disable interactive map in case there is code error thrown
- Note, that interactiveMapMode is first enabled for map initialization to work correctly and only if error occurs it is disabled (in contrast to enabling the mode after initialization occurs which won't work)
- Resolving logic of whether map mode is enabled is moved out into separate function with readability purpose
- As experimental deck does not require much customized styling that differs from any other deck, there is no sense in storing separate stylesheet - thus the merge
- Note that non-experimental decks are not affected by the merge as:
   * `@import url("_jsvectormap.min.css");` at-rule is ignored due to stylesheet _jsvectormap.min.css not existing
   * Moreover, even assuming the user installs experimental deck and then downgrades to regular one, leaving _jsvectormap.min.css in their collection folder, the rules in that stylesheet are isolated and applicable to service classes assigned to the map element (which is absent on regular decks)
   * `.value--map` rule is not applied as there are no elements with this class (assuming future commits will be mindful of this class already existing and won't introduce it into the decks)
- In order to increase maintainability of the code, move out all the "magic" strings being used in the code into one place
- Standard map that comes with the jvm library is losing significant land details
- In order to increase UX, integrate additional map with much more land mass details
- Note, that as a result of performance considerations described commit as well introduces possibility to switch to low-detail map as some users may not possess enough available computing resources, specifically smartphones.
- Also note that it is conscious decision to include both map initializations in card templates as it did not show to slow performance significantly. Further commits may improve existing logic
@axelboc
Copy link
Collaborator

axelboc commented Aug 13, 2024

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
Copy link
Collaborator Author

helitopia commented Aug 13, 2024

Hi @aplaice, @axelboc

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

Done:

  • name collision avoidance via file renaming
  • full stops, IIFE clarification in docs
  • _jsvectormap.js version and license attribution
  • pipenv run brain_brew run recipes/source_to_anki_[experimental].yaml check
  • user configuration move to note template
  • UUID change in note model

Skipped:

  • jsvectormap stylesheet import directly in the note template (via <link> element)
    • the solution is invalid as this way library stylesheet appears later (than style.css) in the cascade thus overriding some of the map properties (e.g. height) and making map appear "broken". Not a major issue, as was said - @import in style.css is activated for experimental deck only, being ignored otherwise. The only possible problem is future contributors confusion (along the lines "why have @import that refers to non-existing stylesheet for regular deck). This might be eliminated by adding implementation notice in documentation later on.
  • user guide in README.md and deck description - as per @axelboc roadmap (see details below)

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
Copy link
Collaborator

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! ^^

LICENSE.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +2 to +3
sessionStorage.setItem("userConfig",
JSON.stringify({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what @aplaice and I had in mind was to somehow hide away those two lines. You could expose a defineInteractiveMapConfig but that would mean importing yet another script at the top of the template...

<script src="_ug-interactive_map_setup.js"></script>
<script>
  defineInteractiveMapConfig({
      commonFeatures: { ... },
      commonColors: { ... },
  })
</script>
// _ug-interactive_map_setup.js
(function () {
  function defineInteractiveMapConfig(userConfig) {
    sessionStorage.setItem("userConfig", JSON.stringify(userConfig));
  }
}());

Not great... An alternative might be to expose the config on the global scope:

<script>
  window.INTERACTIVE_MAP_CONFIG = {
    commonFeatures: { ... },
    commonColors: { ... },
  };
</script>
// _ug-interactive_map_config.js
(function () {
  // Retrieve and store user config in `sessionStorage` if present (i.e. on front side only)
  if ('INTERACTIVE_MAP_CONFIG' in window) {
      sessionStorage.setItem("userConfig", JSON.stringify(window.INTERACTIVE_MAP_CONFIG));
  }

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

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

<script id="ugInteractiveMapConfig" type="application/json">
  {
    "commonFeatures": { ... },
    "commonColors": { ... }
  }
</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`
  }
}());

Copy link
Collaborator

@aplaice aplaice Aug 16, 2024

Choose a reason for hiding this comment

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

I think what @aplaice and I had in mind was to somehow hide away those two lines. You could expose a defineInteractiveMapConfig but that would mean importing yet another script at the top of the template...

I had meant not including the getMapConfig, filterObj and other functions in the template and leaving them in _ug-interactive_map_config.js. It would be slightly better not to have the two lines of stringifying and setting session storage in the template, but it should be clear to both "laypeople" and programmers that they shouldn't be touched, and they're not much "weight-wise".

I do like the second approach, though (both from a "cleanness" perspective — it makes it clear that the configuration is configuration and not code — and from a "less risk of the user messing things up" one), and I think it should work on AnkiDroid since we'll still be setting the sessionStorage on the front card, just in a different place?

(But I think that the status quo is fine!)

@axelboc
Copy link
Collaborator

axelboc commented Aug 16, 2024

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
Copy link
Collaborator Author

helitopia commented Aug 16, 2024

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
Copy link
Collaborator

aplaice commented Aug 16, 2024

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
Copy link
Collaborator

axelboc commented Aug 17, 2024

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 and others added 6 commits August 17, 2024 13:26
- In order to make customizable part of program configuration more user-friendly - move the mentioned configuration into note template to be able to open it from any device and easily modify the available properties
@helitopia
Copy link
Collaborator Author

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 axelboc merged commit 84edac8 into anki-geo:master Aug 17, 2024
1 check passed
@axelboc
Copy link
Collaborator

axelboc commented Aug 17, 2024

Excellent, thanks!

@aplaice
Copy link
Collaborator

aplaice commented Aug 17, 2024

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
Copy link
Collaborator

axelboc commented Aug 17, 2024

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
Copy link
Collaborator Author

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

@axelboc axelboc added the structure Templates, tags, generated decks, etc. label Aug 18, 2024
@axelboc axelboc added this to the v5.3 milestone Aug 18, 2024
@aplaice
Copy link
Collaborator

aplaice commented Aug 18, 2024

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
Copy link
Collaborator Author

helitopia commented Aug 19, 2024

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
Copy link
Collaborator

aplaice commented Aug 19, 2024

For now, I've opened a discussion here:

#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
Copy link
Collaborator Author

helitopia commented Aug 19, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
structure Templates, tags, generated decks, etc.
Development

Successfully merging this pull request may close these issues.

4 participants