diff --git a/CHANGELOG.md b/CHANGELOG.md index 5be6ef6..38a98cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,35 @@ detail, which is where the reasoning lives. ### Fixed +- **Opening a deck no longer rewrites it.** Studying any deck in `examples/` + straight from a clone left it modified in `git status` without a single card + having been answered, which looks like the app corrupting its own sample data + and is an easy way to get surprise diffs. + + Two separate causes, both of which had to go: + + - Every card was given an `id` when the deck was *loaded*. An id is what a + log event names a card by, so a card needs one at the moment something is + recorded against it — and a deck that is only read has nothing recorded + against it. Ids are now minted at that moment instead. Answered cards are + unaffected: the id still exists before the event that names it is written, + which is the invariant that matters. + - Saving expanded every row to its full width, so a hand-written + `question,answer,tags` deck came back as `question,answer,tags,0,0,1,,,id`. + Cards are now written only as far as the last column they actually use. + That rule already existed for the `audio` and `image` columns, for exactly + this reason; it now covers the whole row rather than the last two. + + A save that would reproduce the file byte for byte is also skipped outright. + Saving is unconditional at every call site — after every answer, after every + edit, and on the way out — which is deliberate and is what makes an + interrupted session cost nothing; it just should not mean that reading a deck + counts as writing it. + + Nothing about what a deck can contain has changed, and every deck written by + an earlier version still loads and still saves identically once a card in it + has been reviewed. + - **`FLASHTERM_SEED` now fixes the review order on every platform, not just the one you built on.** The shuffle went through `std::shuffle`, whose output the standard does not specify — only that the permutation is uniformly random — diff --git a/README.md b/README.md index 206ae55..5a0d94d 100644 --- a/README.md +++ b/README.md @@ -206,20 +206,28 @@ question,answer,tags,correct,incorrect,box,last_reviewed,due_date,id,audio,image Dates are plain `YYYY-MM-DD`, blank when a card has never been reviewed. Answers may list alternatives separated by `|`. Questions and answers containing commas or quotes are quoted normally, so decks stay greppable and editable by hand. `id` is 16 hex characters identifying the card for the review log. It is filled -in automatically the first time a deck is loaded, so hand-written decks can +in automatically the first time the card is answered, so hand-written decks can leave it off, and it stays the same when you edit the card — a fixed typo does -not orphan the card's history. +not orphan the card's history. Not on load, deliberately: opening a deck does +not change it, so a deck you only read is a deck your sync client and your +version control never see move. `audio` is a recording of the *question*, as a path relative to the deck file — so a deck and the audio directory beside it can be moved or synced as one thing. `image` is a picture for the card, resolved the same way. See [Images](#images). -Both are written only as far as the last column a card actually uses, which -means a deck with neither comes out byte for byte as earlier versions wrote it, -and syncing between a machine that has updated and one that has not does not put -the whole file in conflict. A card with a picture and no recording still writes -the empty audio column, because position is what names a field in a CSV. +**Every card is written only as far as the last column it actually uses**, so a +deck of plain `question,answer,tags` rows — which is what the examples are, and +what you get writing one by hand — is saved back in that form rather than +expanded. A card that has never been reviewed says nothing in the six columns +after its tags, and so writes none of them. + +That is also what keeps a deck byte for byte as earlier versions wrote it, so +syncing between a machine that has updated and one that has not does not put +the whole file in conflict. Trailing columns only: a card with a picture and no +recording still writes the empty audio column, because position is what names a +field in a CSV. ### Review Log Format diff --git a/src/deck.cpp b/src/deck.cpp index 56e1ebf..f9d62c3 100644 --- a/src/deck.cpp +++ b/src/deck.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include "date.h" @@ -30,28 +31,58 @@ std::string errno_message() { } // namespace std::string card_to_csv(const Flashcard& card) { - const std::string row = - escape_csv_field(card.question) + "," + escape_csv_field(card.answer) + - "," + escape_csv_field(card.tags_to_string()) + "," + - std::to_string(card.times_correct) + "," + - std::to_string(card.times_incorrect) + "," + - std::to_string(card.leitner_box) + "," + - format_date(card.last_reviewed) + "," + format_date(card.due_date) + "," + - escape_csv_field(card.id); - - // Written only as far as the last column the card actually uses. A deck with - // no audio and no pictures -- which is most decks -- then comes out byte for - // byte as every earlier version wrote it. That matters because decks are - // synced between machines as plain files: a trailing comma on every line - // would put the whole deck in conflict the first time one machine saved it - // and the other had not updated yet. + const std::string columns[] = { + escape_csv_field(card.question), + escape_csv_field(card.answer), + escape_csv_field(card.tags_to_string()), + std::to_string(card.times_correct), + std::to_string(card.times_incorrect), + std::to_string(card.leitner_box), + format_date(card.last_reviewed), + format_date(card.due_date), + escape_csv_field(card.id), + escape_csv_field(card.audio), + escape_csv_field(card.image), + }; + + // What card_from_csv fills in for a column that is not there at all. A + // column holding exactly this says nothing the reader would not have assumed + // anyway, so writing it is optional -- which is the whole basis of the trim + // below, and the reason these two lists have to stay in step. + static const char* const kAbsent[] = {"", "", "", "0", "0", "1", + "", "", "", "", ""}; + const std::size_t count = sizeof(columns) / sizeof(columns[0]); + + // Written only as far as the last column the card actually uses. A deck of + // plain question,answer,tags rows -- which is what every deck in examples/ + // is, and what anyone writing one by hand produces -- comes back out in that + // form rather than being expanded to the full eleven. Without this, opening + // such a deck and closing it rewrote every line of it. // - // A card with a picture and no recording still has to write the empty audio - // column, since position is what names a field in a CSV. - if (card.audio.empty() && card.image.empty()) return row; - const std::string with_audio = row + "," + escape_csv_field(card.audio); - if (card.image.empty()) return with_audio; - return with_audio + "," + escape_csv_field(card.image); + // It is also what keeps a deck byte for byte as an earlier version wrote it, + // which matters because decks are synced between machines as plain files: a + // trailing comma on every line would put the whole deck in conflict the + // first time one machine saved it and the other had not updated yet. The + // audio and image columns were the first to need this and are now simply the + // last two cases of the general rule. + // + // Trailing, not any: a card with a picture and no recording still writes the + // empty audio column, because position is what names a field in a CSV. + // + // Question, answer and tags are always written, even when the tags are + // empty. Three columns is the documented short form of a deck, and a + // two-column line, though it would read back correctly, is not a shape + // anything else in the project produces. + std::size_t last = 2; + for (std::size_t i = 3; i < count; ++i) { + if (columns[i] != kAbsent[i]) last = i; + } + + std::string row = columns[0]; + for (std::size_t i = 1; i <= last; ++i) { + row += "," + columns[i]; + } + return row; } bool card_from_csv(const std::string& line, Flashcard* out) { @@ -101,21 +132,57 @@ bool Deck::load() { std::ifstream file(path_); if (!file.is_open()) { + on_disk_.clear(); + on_disk_known_ = false; return false; } + + // Read whole rather than line by line, and kept, so that save() can answer + // "would writing this change the file?" against the actual bytes. Parsing + // then runs over the copy. Line-at-a-time reading cannot serve here: it + // silently drops blank lines and cannot tell whether the last line ended in + // a newline, and both of those are differences a save would write out. + std::ostringstream buffer; + buffer << file.rdbuf(); + on_disk_ = buffer.str(); + on_disk_known_ = true; + + std::istringstream lines(on_disk_); std::string line; - while (std::getline(file, line)) { + while (std::getline(lines, line)) { if (trim(line).empty()) continue; Flashcard card("", ""); if (card_from_csv(line, &card)) { cards_.push_back(card); } } - ensure_ids(); + + // Note that ids are deliberately *not* minted here; see Deck::ensure_id. + // Duplicate ids are likewise left alone, because a deck that is only read + // never uses them. import_into_deck resolves them, which is where they come + // from. return true; } bool Deck::save(std::string* error) const { + std::string content; + for (const auto& card : cards_) { + content += card_to_csv(card); + content += "\n"; + } + + // A write that would reproduce the file exactly is not a save. Every call + // site saves unconditionally -- after each answer, after each edit, and on + // the way out -- which is deliberate and is what makes an interrupted + // session cost nothing. The price was that opening a deck and closing it + // rewrote it, and for the decks in examples/ that meant a fresh clone could + // not be studied without the repository showing modified files. + // + // Cheaper than it looks, and cheaper than the write it replaces: the + // comparison is against a string already in memory, and the saves that do + // nothing are exactly the ones where the deck is large and untouched. + if (on_disk_known_ && content == on_disk_) return true; + const std::string tmp_path = path_ + ".tmp"; { @@ -124,9 +191,7 @@ bool Deck::save(std::string* error) const { if (error) *error = "cannot write " + tmp_path + ": " + errno_message(); return false; } - for (const auto& card : cards_) { - file << card_to_csv(card) << "\n"; - } + file << content; file.flush(); if (!file) { if (error) *error = "failed writing " + tmp_path; @@ -141,6 +206,10 @@ bool Deck::save(std::string* error) const { std::remove(tmp_path.c_str()); return false; } + + // Only now, because a failed write leaves the file as it was. + on_disk_ = content; + on_disk_known_ = true; return true; } @@ -161,6 +230,22 @@ void Deck::ensure_ids() { } } +const std::string& Deck::ensure_id(Flashcard& card) { + if (!card.id.empty()) return card.id; + + // Collected per call rather than kept as a member: this runs when a card is + // answered for the first time since the log existed, which is once per card + // in the life of a deck and never in a loop. + std::set taken; + for (const auto& other : cards_) { + if (!other.id.empty()) taken.insert(other.id); + } + do { + card.id = generate_id(); + } while (!taken.insert(card.id).second); + return card.id; +} + bool Deck::remove(std::size_t index) { if (index >= cards_.size()) return false; cards_.erase(cards_.begin() + static_cast(index)); diff --git a/src/deck.h b/src/deck.h index 08c628c..95bff8a 100644 --- a/src/deck.h +++ b/src/deck.h @@ -47,6 +47,12 @@ class Deck { // Writes to a temporary file and renames it into place, so an interrupted // or failed write can never leave a truncated deck behind. + // + // A write that would reproduce the file byte for byte is skipped and + // reported as success. Saving is unconditional at every call site -- after + // every answer, every edit, and on the way out -- which is what makes an + // interrupted session cost nothing; the cost is that merely opening a deck + // used to rewrite it. See the comment on the check itself. bool save(std::string* error = nullptr) const; const std::string& path() const { return path_; } @@ -106,12 +112,36 @@ class Deck { // log existed, and duplicated ids arrive from re-importing an export into // the deck it came from — two cards sharing one history would make the log // ambiguous, so the newcomer is renamed rather than the incumbent. + // + // Deliberately not called by load(): see ensure_id. void ensure_ids(); + // Gives one card an id if it has none, unique against the rest of the deck, + // and returns it. + // + // This exists so that load() does not have to mint ids for a whole deck just + // to open it. An id is what a log event names a card by, so a card needs one + // at the moment something is about to be recorded against it and not before + // — and a deck that is only read has nothing recorded against it at all. + // Minting on load meant that opening a deck changed it, which made studying + // a deck checked into a repository show up as a source change. + const std::string& ensure_id(Flashcard& card); + private: std::string path_; std::vector cards_; EventLog log_; + + // What the file holds as of the last successful read or write, so that + // save() can tell a real change from a no-op. Mutable because save() is + // const to its callers and stays that way: what it records here is a fact + // about the file rather than about the deck. + // + // The flag is not the same question as an empty string: a deck file that + // does not exist yet still has to be created the first time it is saved, + // and an empty deck serialises to nothing. + mutable std::string on_disk_; + mutable bool on_disk_known_ = false; }; // Appends the cards in `path` to `deck`, preserving review statistics when diff --git a/src/generate.cpp b/src/generate.cpp index 5509d46..bd15031 100644 --- a/src/generate.cpp +++ b/src/generate.cpp @@ -88,9 +88,10 @@ GenerateResult generate_audio(Deck& deck, const std::string& voice, bool force, return result; } - // A card is named by its id, so every card needs one before anything can be - // written. Loading a deck normally mints them; a deck loaded and immediately - // generated from may not have been through that yet. + // A recording is named after the card's id, so every card needs one before + // anything can be written. Loading a deck no longer mints them -- reading a + // deck leaves it alone, see Deck::ensure_id -- so this is where a whole + // deck's worth gets minted, and the deck is about to be written anyway. deck.ensure_ids(); const std::string directory = deck.resolve(kAudioDirectory); diff --git a/src/review.cpp b/src/review.cpp index c13dacb..21462b6 100644 --- a/src/review.cpp +++ b/src/review.cpp @@ -568,12 +568,16 @@ std::string log_event(Deck& deck, const ReviewEvent& event, bool* warned) { return event.id; } -std::string log_answer(Deck& deck, const Flashcard& card, bool reversed, +// Takes the card by mutable reference because writing an event is the moment +// the card needs an id: a deck written before the log existed has none, and +// ensure_id mints one here rather than on load so that reading a deck leaves +// it alone. +std::string log_answer(Deck& deck, Flashcard& card, bool reversed, Outcome outcome, const AnswerResult& result, bool* warned) { ReviewEvent event; event.id = generate_id(); - event.card_id = card.id; + event.card_id = deck.ensure_id(card); event.timestamp = now_timestamp(); event.direction = reversed ? 'r' : 'n'; event.outcome = outcome; @@ -582,11 +586,13 @@ std::string log_answer(Deck& deck, const Flashcard& card, bool reversed, return log_event(deck, event, warned); } -void log_undo(Deck& deck, const Flashcard& card, const std::string& answer_id, +// The card has an id by now, having just been answered; ensure_id is asked +// anyway rather than assuming the order of two calls in another function. +void log_undo(Deck& deck, Flashcard& card, const std::string& answer_id, bool* warned) { ReviewEvent event; event.id = generate_id(); - event.card_id = card.id; + event.card_id = deck.ensure_id(card); event.timestamp = now_timestamp(); event.undoes = answer_id; log_event(deck, event, warned); diff --git a/tests/golden/cases/eof-mid-session/expected b/tests/golden/cases/eof-mid-session/expected index 35eeff7..38b7be5 100644 --- a/tests/golden/cases/eof-mid-session/expected +++ b/tests/golden/cases/eof-mid-session/expected @@ -20,4 +20,4 @@ Input stream closed/EOF. Flashcards saved. Goodbye! --- exit status --- 0 --- file deck.txt --- -Bonjour,Hello,french,0,0,1,,, +Bonjour,Hello,french diff --git a/tests/golden/cases/export-and-reimport/expected b/tests/golden/cases/export-and-reimport/expected index 3593571..e817098 100644 --- a/tests/golden/cases/export-and-reimport/expected +++ b/tests/golden/cases/export-and-reimport/expected @@ -53,5 +53,5 @@ Merci,Thanks|Thank you,french,0,0,1,,, Bonjour,Hello,french;greeting,0,0,1,,, Merci,Thanks|Thank you,french,0,0,1,,, --- file out.csv --- -Bonjour,Hello,french;greeting,0,0,1,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/golden/cases/manage-list-and-search/expected b/tests/golden/cases/manage-list-and-search/expected index 3bc8d3f..d59a07c 100644 --- a/tests/golden/cases/manage-list-and-search/expected +++ b/tests/golden/cases/manage-list-and-search/expected @@ -65,5 +65,5 @@ Nothing matches "zzz". --- exit status --- 0 --- file deck.txt --- -Bonjour,Hello,french;greeting,0,0,1,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/golden/cases/progress/expected b/tests/golden/cases/progress/expected index 4d65242..b661f9e 100644 --- a/tests/golden/cases/progress/expected +++ b/tests/golden/cases/progress/expected @@ -53,5 +53,5 @@ Merci 0 0 0.00 1 new --- exit status --- 0 --- file deck.txt --- -Bonjour,Hello,french;greeting,0,0,1,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/golden/cases/review-quit-midway/expected b/tests/golden/cases/review-quit-midway/expected index 3767b07..7c2bc27 100644 --- a/tests/golden/cases/review-quit-midway/expected +++ b/tests/golden/cases/review-quit-midway/expected @@ -62,4 +62,4 @@ Your answer: ================================================== --- exit status --- 0 --- file deck.txt --- -Bonjour,Hello,french,0,0,1,,, +Bonjour,Hello,french diff --git a/tests/golden/cases/tags/expected b/tests/golden/cases/tags/expected index bb051fa..14809b3 100644 --- a/tests/golden/cases/tags/expected +++ b/tests/golden/cases/tags/expected @@ -31,5 +31,5 @@ Loaded 2 flashcards from deck.txt --- exit status --- 0 --- file deck.txt --- -Bonjour,Hello,french;greeting,0,0,1,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/tests.cpp b/tests/tests.cpp index 4e4b8c2..1024a8f 100644 --- a/tests/tests.cpp +++ b/tests/tests.cpp @@ -57,6 +57,16 @@ std::string temp_path(const std::string& name) { return "build/test-" + name; } +// Whole-file, bytes as they are: a test about whether a file was rewritten +// cannot read it a line at a time, since that is exactly what hides a +// difference in trailing newlines. +std::string file_contents(const std::string& path) { + std::ifstream file(path); + std::ostringstream buffer; + buffer << file.rdbuf(); + return buffer.str(); +} + void test_trim_and_case() { EXPECT_EQ(trim(" hello "), std::string("hello")); EXPECT_EQ(trim("\t\r\n"), std::string("")); @@ -1977,7 +1987,9 @@ void test_card_ids() { EXPECT_EQ(log, path + ".log"); - // A deck written before ids existed gets them on load, and they stick. + // A deck written before ids existed keeps its cards id-less on load, because + // reading a deck does not change it. One is minted when something is about + // to be recorded against the card, and then it sticks. { std::ofstream file(path); file << "Q1,A1,tag,3,1,4,2026-08-01,2026-08-15\n" @@ -1986,11 +1998,17 @@ void test_card_ids() { Deck deck(path); EXPECT_TRUE(deck.load()); EXPECT_EQ(deck.size(), size_t{2}); - EXPECT_TRUE(!deck.cards()[0].id.empty()); - EXPECT_TRUE(!deck.cards()[1].id.empty()); - EXPECT_TRUE(deck.cards()[0].id != deck.cards()[1].id); + EXPECT_TRUE(deck.cards()[0].id.empty()); + EXPECT_TRUE(deck.cards()[1].id.empty()); + + const std::string first_id = deck.ensure_id(deck.cards()[0]); + EXPECT_TRUE(!first_id.empty()); + // Asking again returns the same one rather than minting a second. + EXPECT_EQ(deck.ensure_id(deck.cards()[0]), first_id); + // Its neighbour is left alone until it is asked for, and then differs. + EXPECT_TRUE(deck.cards()[1].id.empty()); + EXPECT_TRUE(deck.ensure_id(deck.cards()[1]) != first_id); - const std::string first_id = deck.cards()[0].id; EXPECT_TRUE(deck.save()); Deck reloaded(path); EXPECT_TRUE(reloaded.load()); @@ -2026,6 +2044,69 @@ void test_card_ids() { std::remove(exported.c_str()); } +// Opening a deck and closing it must leave the file exactly as it was. The +// decks in examples/ are the reason: they are checked in, they are written in +// the short three-column form, and studying one used to rewrite every row of +// it into the full nine -- so a fresh clone could not be tried out without the +// repository showing modified files. +void test_save_leaves_an_unchanged_deck_alone() { + const std::string path = temp_path("unchanged.txt"); + std::remove(path.c_str()); + + // The short form, exactly as a deck in examples/ is written. + const std::string original = + "la biblioteca,library,spanish;nouns\n" + "el árbol,tree,spanish;nouns\n"; + { + std::ofstream file(path); + file << original; + } + + Deck deck(path); + EXPECT_TRUE(deck.load()); + EXPECT_EQ(deck.size(), size_t{2}); + // Saving reports success without having touched the file. + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), original); + + // Repeatedly, since every answer and every menu exit saves. + EXPECT_TRUE(deck.save()); + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), original); + + // A real change is still written -- and only as far as the column that + // carries it, so the card that did not change keeps its short row and the + // one that did grows by exactly the three columns it now needs. + deck.cards()[0].leitner_box = 3; + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), + std::string("la biblioteca,library,spanish;nouns,0,0,3\n" + "el árbol,tree,spanish;nouns\n")); + + // ...and having written it, the deck knows it is current again. + const std::string written = file_contents(path); + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), written); + + // A deck file that does not exist yet is created rather than skipped, even + // when the deck is empty and so serialises to nothing at all. The flag and + // an empty string are not the same question. + const std::string fresh = temp_path("unchanged-new.txt"); + std::remove(fresh.c_str()); + Deck new_deck(fresh); + EXPECT_TRUE(!new_deck.load()); // no such file, which is not an error + EXPECT_TRUE(new_deck.save()); + { + std::ifstream check(fresh); + EXPECT_TRUE(check.is_open()); + } + + std::remove(path.c_str()); + std::remove(fresh.c_str()); + std::remove((path + ".log").c_str()); + std::remove((fresh + ".log").c_str()); +} + void test_wrap() { // Short text is one line, and empty text is still one line so a frame drawn // from the result is never zero rows tall. @@ -2291,6 +2372,7 @@ int main() { test_shipped_example_decks(); test_summarize(); test_card_ids(); + test_save_leaves_an_unchanged_deck_alone(); test_wrap(); test_partial_answers(); test_partial_events();