Faster card creation#130
Conversation
|
Hi! Before making a large PR, It would be awesome if you reached out through our community. Thanks for the changes, I'll review them. |
helpers.lua
Outdated
| this.max_n = function(note_ids, n) | ||
| table.sort(note_ids) | ||
| return {table.unpack(note_ids, math.max(#note_ids - n + 1, 1), #note_ids)} | ||
| end |
There was a problem hiding this comment.
This name is hard to understand. Apparently, this function should be named something like take_last_n_added_notes.
platform/win.lua
Outdated
| table.concat { '@', curl_tmpfile_path } | ||
| --table.concat { '@', curl_tmpfile_path } | ||
| } | ||
| table.insert(args, request_json) |
There was a problem hiding this comment.
Looks like this change was made by mistake.
There was a problem hiding this comment.
Apologies, I don't quite remember the intention of this change either. I'll remove it and verify.
There was a problem hiding this comment.
I actually believe this was actually to prevent overwriting due to asynchronous calls? When updating multiple cards, only the last updateNoteFields goes through. I'm not 100% sure on this as I never got to the bottom of it, but I suspect it has something to do with using a temp txt file + mp.command_native_async. When I changed it to include the request_json at runtime (as the nix.lua also does), multiple card updates works again. Is there a reason for using a temporary text file for the curl commands?
There was a problem hiding this comment.
Is there a reason for using a temporary text file for the curl commands?
Curl on windows doesn't work reliably when json is passed as a command line argument. Using a temporary file is a widely used solution. We also ran into the issue in the past and had to adopt the fix.
When updating multiple cards, only the last updateNoteFields goes through.
Then we need to use a unique filename every time.
Good to know, thanks for the heads up. I didn't see any contact in the mpvacious repo yesterday but I found the telegram on the main site today. I'll try to make an account tonight. |
|
I can't update notes anymore using the 'm' key. After I select the subtitle lines I want, the 'm' button just doesn't do anything.
In the console it says |
helpers.lua
Outdated
| end | ||
| this.max_n = function(note_ids, n) | ||
| table.sort(note_ids) | ||
| return { table.unpack(note_ids, math.max(#note_ids - n + 1, 1), #note_ids) } |
There was a problem hiding this comment.
Callingtable.unpack causes an error: attempt to call field 'unpack' (a nil value). To fix the error, change table.unpack to this.unpack.
use this.unpack in place of table.unpack remove extraneous observer revert win.lua changes
|
Unfortunately the multiple card update looks to be broken now. I made a few cards tonight with the regular quick card creation (with just g[0-9]), so that seems to work fine at least. Would you prefer me to close the PR and work on integration or leave it open as draft? |
I think it's better to leave it open for now. Additionally, the users need to understand how to use the new functionality. So we need to add the appropriate documentation to |
|
Got it, I'll add that over this week too. Currently I plan to:
|
435009b to
b13ae4f
Compare
b13ae4f to
0c31bd1
Compare
|
@tatsumoto-ren Hi Tatsumoto, can you review the short tutorial I wrote in the README? |
subtitles/observer.lua
Outdated
| end | ||
|
|
||
| self.collect = function() | ||
| self.collect = function(n_lines) |
There was a problem hiding this comment.
This function now does two different things depending on the argument passed. In such cases it's better to have two functions instead of one.
subs2srs.lua
Outdated
| local n_lines | ||
| local n_cards = 1 |
There was a problem hiding this comment.
These two values are logically tied together, so it would be appropriate to encapsulate them in one object.
|
Thanks for making the README.
It doesn't work with secondary subtitles, right?
I'll test it. |
Yes, I didn't add support for secondary subtitles. Let me know if this is a problem. |
|
Also, I saw you reverted the request_json change. Were you able to take a look at the earlier comment regarding it? |
Users will likely find this confusing. |
|
I wrote a workaround for the async curl calls issue. But I don't use Windows so can't test it. |
|
Thanks, I'll give it a shot. What's the benefit of using these files for the curl command instead of passing in the request directly in the lua table? I'll add secondary subtitle support too. |
It's to avoid the issue with failing curl requests on windows. |
subs2srs.lua
Outdated
| local last_note_ids = ankiconnect.get_last_note_ids(n_cards or 1) | ||
| local last_note_ids = ankiconnect.get_last_note_ids(n_cards) |
There was a problem hiding this comment.
What's the purpose of the or 1 expression?
There was a problem hiding this comment.
I don't quite remember why I had that to be honest lol. Likely some sort of safeguard in case n_cards is nil or 0, but using the quick_creation_opts guarantees a valid value now.
|
Hi @tatsumoto-ren , sorry for the delay. I got around to testing the new curl request and secondary sub additions. Seems to be all working now. I had to modify some of the curl logic as async calls for something like grabbing the last n notes would not work. |
cool! |
* add quick note update * card selection feature * change number of cards updated in main menu * format * fixup unpack and extraneous code use this.unpack in place of table.unpack remove extraneous observer revert win.lua changes * rename max_n to get_last_n_added_notes * insert curl request directly into lua table * add quick card creation tutorial, rename script-binding * clearer differentation between card/line selection menu * lower bound num cards to 1 * fix case with empty current sub * fix request_json * use a random tmp file for each curl request * refactor diag collection and bundle quick creation opts * secondary sub support for quick card creation * format * get cards/lines typo fix * fix windows curl request * fix card creation --------- Co-authored-by: Ren Tatsumoto <tatsu@autistici.org>
* add quick note update * card selection feature * change number of cards updated in main menu * format * fixup unpack and extraneous code use this.unpack in place of table.unpack remove extraneous observer revert win.lua changes * rename max_n to get_last_n_added_notes * insert curl request directly into lua table * add quick card creation tutorial, rename script-binding * clearer differentation between card/line selection menu * lower bound num cards to 1 * fix case with empty current sub * fix request_json * use a random tmp file for each curl request * refactor diag collection and bundle quick creation opts * secondary sub support for quick card creation * format * get cards/lines typo fix * fix windows curl request * fix card creation --------- Co-authored-by: Ren Tatsumoto <tatsu@autistici.org>
* add quick note update * card selection feature * change number of cards updated in main menu * format * fixup unpack and extraneous code use this.unpack in place of table.unpack remove extraneous observer revert win.lua changes * rename max_n to get_last_n_added_notes * insert curl request directly into lua table * add quick card creation tutorial, rename script-binding * clearer differentation between card/line selection menu * lower bound num cards to 1 * fix case with empty current sub * fix request_json * use a random tmp file for each curl request * refactor diag collection and bundle quick creation opts * secondary sub support for quick card creation * format * get cards/lines typo fix * fix windows curl request * fix card creation --------- Co-authored-by: Ren Tatsumoto <tatsu@autistici.org>
Hi, I was using my fork of mpvacious for the past year without updates, but when I finally got around to updating it the conflict fixing was a bit of a headache, so I thought I'd see if you would be interested in merging this upstream. It is pretty hacky at the moment but it works. There would have to be some additions for config reading to change keybinds I think. I wanted to see what you thought before making the effort for that as I don't need it for my personal setup haha
I basically wanted a faster way to make basic cards where I don't manually set the start/end times. Normally the default Ctrl+M binding does the job; however, there are cases I want to either use multiple lines (usually 2) and/or update multiple notes at once, say when I encounter two unknown words within the same context or sentence. Mpvacious at the moment has incredible flexibility but it comes at the cost of a lot more keystrokes than I'm used to.
What I added was a "quick card creation menu" where card creation for most subtitled works can be done in two or less keystrokes. It opens with 'g' (by default) and asks for a number. I can enter [0-9] to overwrite the last note with the number of subs chosen. alt+g would let me change the number of cards to update, followed by the number of subs.
This relies on the fact that mpv has actually seen the next subtitle, but that will usually already be the case as the user wouldn't be trying to add lines they haven't seen yet to the card.
I've successfully created plenty of cards with this now and I love having both the quick card creation as well as the advanced menu flexibility when I actually need it, often in non subtitled works or YouTube videos with very strange subtitle timings.