Update currently selected cards in Anki#134
Update currently selected cards in Anki#134tatsumoto-ren merged 13 commits intoAjatt-Tools:masterfrom
Conversation
|
This repo doesn't use tabs to indent code. |
|
My bad, should be good now. |
|
rebased against master. |
|
If you update multiple notes you will only see the output from the last one added on the OSD. This is especially bad if there's an error adding a note that's not the last one. Also all modified notes should be opened in Anki, not just the last one. |
|
Yeah, I think only one card should be updated at a time. If you have multiple cards with the same sentence (and the same audio recording), that's typically a bad practice. |
It was really due to how ankiconnect.append_media works, but I have a solution. Displaying all noteids is confusing in my opinion, so I made it that just the number of cards that were updated is displayed, errors still work.
Yes, I considered that first but I wanted the functionality to match the "update last notes" functionality, which allows it. I'll set config.card_overwrite_safeguard to 1 by default. Users still can change it, but can be discouraged with documentation. |
This is generally fine for vocab cards. I realize you don't like them, but people are going to want to make them anyway. Discouraging is fine, removing the option seems like a bit much.
I think this strikes the right balance.
Agreed, there's no value in seeing the IDs, so this is perfect. |
ankiconnect.lua
Outdated
| end | ||
|
|
||
| self.append_media = function(note_id, fields, create_media_fn, tag) | ||
| self.append_media = function(note_id, fields, create_media_fn, tag, quiet_on_success) |
There was a problem hiding this comment.
I think it might be cleaner to have this function return an error if encountered, and have the caller handle printing. You could then print things like "updated 3 cards, 1 error.: ...".
There was a problem hiding this comment.
Well, this function runs curl request asynchronously, so we either gotta wait or do this. I chose not to fiddle and change too much
There was a problem hiding this comment.
Fair enough, that's probably too big of a rewrite for a small feature like this.
ankiconnect.lua
Outdated
| self.gui_browse(string.format("nid:%s", note_id)) -- select the updated note in the card browser | ||
| h.notify(string.format("Note #%s updated.", note_id)) | ||
| if not quiet_on_success then | ||
| self.gui_browse(string.format("nid:%s", note_id)) -- select the updated note in the card browser |
There was a problem hiding this comment.
If the caller handles printing it could also handle this. It already does for multiple cards right now, so this could be consolidated to one block of code in one file.
|
I cooked this special branch https://github.com/fulltimebaka/mpvacious/tree/synchronous-updating try and see if you like it |
|
I think it looks sensible, but it might be out of scope for this PR after all. There's probably a bigger refactor it could be part of, so for now it maybe makes sense to just go with the workaround. But let's see what @tatsumoto-ren thinks. |
|
I just realized that we should probably not overwrite the sentence/sentence translation fields when updating a note. They could be from something completely unrelated, so they should be merged unless explicitly overwritten. |
Could you link the code block that needs to be changed? |
| forvo.set_output_dir(anki_media_dir) | ||
| new_data = forvo.append(new_data, stored_data) | ||
| new_data = update_sentence(new_data, stored_data) | ||
| if not overwrite then |
There was a problem hiding this comment.
join_media_fields should also join sentences if you're not overwriting.
This would also require a separator between merged fields (e.g. a newline).
We could also check if the fields are equal (or equal to the data between separators), in which case they can be skipped. I think this is only marginally useful, but I don't think it would hurt either.
There was a problem hiding this comment.
So, add sentence_field and secondary_field to the list?
There was a problem hiding this comment.
Yes, and also change this to something like (I haven't actually run this so please test it):
if not h.is_empty(field) and not string.match(h.table_get(stored_data, field, ""), h.table_get(new_data, field, "")) then
new_data[field] = h.table_get(stored_data, field, "") .. "\n" .. h.table_get(new_data, field, "")
endThere was a problem hiding this comment.
Ok, this can be proposed in a different PR
for #133