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

[Roadmap] ideas about taking another dependency for generating notes #8

Closed
Quasimurdock opened this issue Aug 9, 2023 · 21 comments
Closed
Labels
enhancement New feature or request question Further information is requested

Comments

@Quasimurdock
Copy link
Owner

Quasimurdock commented Aug 9, 2023

Found genanki-js just now. It's a JS implementation of genanki, which allows us to programatically generate decks for Anki.

For now html2notes.js is actually to create a new deck and note type, then import HTML files fetched in the former step directly into users' Anki with anki-connect API. We actually encountered some problems in this way, which has been discussed in #3 . It looks like hard to solve, so I'm thinking about taking genanki-js for the process of converting and importing notes. In that case, we can simply generate .apkg packages and leave the task of importing to users.

This repo is actually named Generater instead of an importing tool. Moreover, the most important is the issue previously mentioned could be avoided since generating apkg files and manually importing them is way more steady than the whole steps of taking anki-connect API to connect with your opening Anki program and invoking those importing related methods

@Quasimurdock Quasimurdock added enhancement New feature or request question Further information is requested labels Aug 9, 2023
@Quasimurdock Quasimurdock pinned this issue Aug 9, 2023
@serifold
Copy link
Collaborator

serifold commented Aug 9, 2023

Get rid of Anki Connect? Sounds like a plan. Let's have a try.

@Quasimurdock
Copy link
Owner Author

Quasimurdock commented Aug 9, 2023

Get rid of Anki Connect? Sounds like a plan. Let's have a try.

Hi @serifold , glad to see u again. I have invited u to become a collaborator of this proj.

If ur happy with it u can accept it and start to have direct access to this repo, otherwise u can still contribute to this repo through ur fork and commit PR to it.

Either way is good, thx 4 ur interest.

@serifold
Copy link
Collaborator

serifold commented Aug 9, 2023

Hi @Quasimurdock, thanks for your trust and appreciation, I'm very glad to become a collaborator of this project.🤝
Tonight I'll take a look at genanki-js's doc to see what we can do.

@serifold
Copy link
Collaborator

serifold commented Aug 9, 2023

Good night @Quasimurdock, I have tried genanki-js, and I have some thoughts about it:

  • It uses browser to generate deck which is a form maybe we don't need.

    image

    One of our card generation solutions is following genanki-js's pattern, modifying genanki-js's functions and UI. Therefore, we can click a button such as choose from folder, then choose our html folder, use these modified functions to get html files processed.

    However, I still prefer CLI. I still want to do the things as usual: run pnpm note, wait several seconds, then get the generated some_deck.apkg. In my opinion, it's more efficient, also even more elegant to some extent.

  • We should replace the library sql.js which is used by genanki-js.

    • sql.js is used in the browser:

      image

    • It has many deprecated dependencies, really uncomfortable for me:

      image

    Instead, if we decide to still use CLI, perhaps we could try sequelize/knex/sqlite3, etc.

@Quasimurdock
Copy link
Owner Author

Quasimurdock commented Aug 10, 2023

@serifold Hi, appreciate ur efforts of figuring out how is it like about genanki.js now. I see that there's a lot of deprecated dependencies of it and the fact that sql.js is for browser. I agree with u about these parts.

As for replacing sql.js with sequelize/knex/sqlite3, I don't quite know much about them. IMO, they are all node modules of connecting sqlite db and for generating data files of apkg right?

And am I right that ur saying that we actually have a chance to modify genanki.js to make it compatible for CLI by choosing a proper npm dependency of sqlite?

@serifold
Copy link
Collaborator

@serifold Hi, appreciate ur efforts of figuring out how is it like about genanki.js now. I see that there's a lot of deprecated dependencies of it and the fact that sql.js is for browser. I agree with u about these parts.

As for replacing sql.js with sequelize/knex/sqlite3, I don't quite know much about them. IMO, they are all node modules of connecting sqlite db and for generating data files of apkg right?

And am I right that ur saying that we actually have a chance to modify genanki.js to make it compatible for CLI by choosing a proper npm dependency of sqlite?

Hi @Quasimurdock, yeah, if we all agree to continue using CLI to generate deck, then we could learn from genanki.js, and transfer what happened in browser to Node.js.

However, currently I'm not quite sure which package should we use, or even if do we really need a database package? By Node.js, we could read from HTML files already, all we need to do should be writing these data to a .apkg file.

This weekend I'll take some time to read the source code of genanki.js to see the necessity of using database package and how does the generation happen.

@Quasimurdock
Copy link
Owner Author

Quasimurdock commented Aug 11, 2023

Hey @serifold , I found that genanki-js is actually a browser version of mkanki which is mentioned by genanki-js repo, and I forked a usable version of it here. mkanki seems to meet out needs for CLI though its last update originates to 5 years ago.

For now, if u wanna test it, u can install it through: pnpm install git+https://github.com/Quasimurdock/mkanki. This would install the module to ur current node proj.

I updated the directory structure of our repo and now it's more extensible for adding genanki or mkanki to generate apkg files. I've read something about apkg, it seems like it's a zip file which contains sqlite data files which is why genanki and other similar apkg generators take sqlite dependencies. As for mkanki, if we take this for apkg generating process we will not have to modify genanki-js which would be more tough.

But I don't know if mkanki has any dependency problems now. IMO if that's not a big deadly problem for our apkg generating, we can bear with it. What do u think about it?

@serifold
Copy link
Collaborator

Hey @serifold , I found that genanki-js is actually a browser version of mkanki which is mentioned by genanki-js repo, and I forked a usable version of it here. mkanki seems to meet out needs for CLI though its last update originates to 5 years ago.

For now, if u wanna test it, u can install it through: pnpm install git+https://github.com/Quasimurdock/mkanki. This would install the module to ur current node proj.

I updated the directory structure of our repo and now it's more extensible for adding genanki or mkanki to generate apkg files. I've read something about apkg, it seems like it's a zip file which contains sqlite data files which is why genanki and other similar apkg generators take sqlite dependencies. As for mkanki, if we take this for apkg generating process we will not have to modify genanki-js which would be more tough.

But I don't know if mkanki has any dependency problems now. IMO if that's not a big deadly problem for our apkg generating, we can bear with it. What do u think about it?

Good night, @Quasimurdock. Thanks for the reminding. Indeed, A original Node.js side package is much more convenient than a browser side one for our project. I have made a fork from @kizaski/mkanki too, and I have made some modifications to make this forked version of mkanki fits our project.

So far, it still has a bug that all Created time are set to 1970-1-1 which is obviously abnormal. I have tested local mkanki, except for the not serious date bug, it seems that this version of mkanki could meet our needs, currently we could use mkanki to generate our cards theoretically.

However, I tried use it in our project for some time, but I still can't make it. It always generates a blank deck. I think this could be a async relevant bug, but still I couldn't solve it... If you don't mind, I'll push a mkanki branch which might be convenient for you to understand what happens, or you could test locally, too.

Up until now, I think this solution is feasible, these bugs don't seem to be impossible to solve. If you think so, we could focus on solving the bugs, or we could find some other solutions.

@Quasimurdock
Copy link
Owner Author

So far, it still has a bug that all Created time are set to 1970-1-1 which is obviously abnormal.

It always generates a blank deck.

... these bugs don't seem to be impossible to solve

Yeah, I think we can figure out what's going on to these two issues.

If you don't mind, I'll push a mkanki branch which might be convenient for you to understand what happens

Great, plz go ahead!

@serifold
Copy link
Collaborator

So far, it still has a bug that all Created time are set to 1970-1-1 which is obviously abnormal.

It always generates a blank deck.

... these bugs don't seem to be impossible to solve

Yeah, I think we can figure out what's going on to these two issues.

If you don't mind, I'll push a mkanki branch which might be convenient for you to understand what happens

Great, plz go ahead!

OK, I have pushed mkanki branch minutes ago. So far I think maybe it's fs.readdir/fs.readFile's some mechanisms lead to the almost immediate return which does not follow the pattern that we expected what async function should run. Perhaps we should read some relevant Node.js doc to see what went wrong actually.

@Quasimurdock
Copy link
Owner Author

Hi, @serifold. I checkout to the mkanki branch and runned pnpm note-mkanki, generating the test.apkg file to import.

But it pops out a window saying 'Notes found in file: ⁨0⁩', the size of this apkg file is 69KB. Is there any problem with it?

@serifold
Copy link
Collaborator

serifold commented Aug 14, 2023

Hi, @serifold. I checkout to the mkanki branch and runned pnpm note-mkanki, generating the test.apkg file to import.

But it pops out a window saying 'Notes found in file: ⁨0⁩', the size of this apkg file is 69KB. Is there any problem with it?

That's what I said 'a blank deck' means. There is nothing in that deck.

In the function convert(), I tried to generate a deck by a async function processHtmlFiles() first, then add the deck to package, and write the package to a test.apkg finally:

async function convert() {
  await processHtmlFiles();
  package.addDeck(deck);
  package.writeToFile('test.apkg');
}

But it seems that package.addDeck(deck) didn't wait for the finish of the async function processHtmlFiles(), it just add a blank deck which we created at the beginning of the process. That means, in my opinion, we thought and treated processHtmlFiles() as a async function, however, Node.js don't think so.

And one of the major part of processHtmlFiles() is fs.readdir and fs.readFile. That's why I thought maybe we could read the relevant part of Node.js to find out what's wrong.

@Quasimurdock
Copy link
Owner Author

Quasimurdock commented Aug 15, 2023

Hey @serifold ! I've pushed a new branch mkanki-dev to the repo, u can checkout to this branch for testing. Now the async problem is almost solved.

Ur judgements above r right. So I looked into processHtmlFiles() method and found async problems. Now that the code is a little bit messy since it's a dev branch so If u have any better improvements, feel free to update.

After we cleaning up & revising the code on dev branch, we'd better merge it to mkanki and main branch. I'm considering about making pnpm note-mkanki to be paralled with pnpm note in main branch for users to choose.


I've committed a PR to merge dev #9 u can review it now

@Quasimurdock
Copy link
Owner Author

Quasimurdock commented Aug 15, 2023

found a new issue related to tags of cards: now every note's tags field only include a B1 tag, idk what happened.

will check it.


found it lies in previous changes in index.js of mkanki related to tags. I've fixed it and later I will commit a PR in ur fork


I've already committed a PR here which is adapted from this, after ur check make sure it updated to primary branch and update dependency of this proj.

@Quasimurdock
Copy link
Owner Author

Quasimurdock commented Aug 15, 2023

So far everythin works fine to me, notify me if u have any latest discoveries

@serifold
Copy link
Collaborator

Hey @serifold ! I've pushed a new branch mkanki-dev to the repo, u can checkout to this branch for testing. Now the async problem is almost solved.

Ur judgements above r right. So I looked into processHtmlFiles() method and found async problems. Now that the code is a little bit messy since it's a dev branch so If u have any better improvements, feel free to update.

After we cleaning up & revising the code on dev branch, we'd better merge it to mkanki and main branch. I'm considering about making pnpm note-mkanki to be paralled with pnpm note in main branch for users to choose.

I've committed a PR to merge dev #9 u can review it now

👍Cool, good job🔥. I just finished review, it's too late, I must go to sleep now😵. I'll share more my thoughts at daytime.

@serifold
Copy link
Collaborator

Ur judgements above r right. So I looked into processHtmlFiles() method and found async problems.

Thanks for the trouble shooting, I improved the comprehension of JS promise api a lot. At that time, I thought about using some Node.js promise api such as fs.promises.readdir, but I didn't make it.

I'm considering about making pnpm note-mkanki to be paralled with pnpm note in main branch for users to choose.

Yeah, we could keep both, that's also what I thought. Then the README should be updated. Suggest users reading this doc to get their environment set up before the installation of packages.

I've already committed a PR serifold/mkanki#1 which is adapted from nornagon/mkanki#1, after ur check make sure it updated to primary branch and update dependency of this proj.

Thanks for your PR, I'll review it and try to figure out what's wrong in my original codes.

So far everythin works fine to me, notify me if u have any latest discoveries.

It's really good. However, Except for the date bug, I found another glitch:

image

The order of Due col should be DES, but they are all 0 now. Looks like the same type of date bug, maybe I could fix them while figure out your PR.

@Quasimurdock
Copy link
Owner Author

Quasimurdock commented Aug 16, 2023

The order of Due col should be DES, but they are all 0 now.

Isn't that managed by Anki's spaced repetition mechanism? I thought they're New #0 cuz they're brand new for being imported just now and u haven't used them. Or ur saying this mechanism doesn't actually work now and field Due cannot be updated after u having used these notes? I dont really get this.

Except for the date bug

And about this created date issue I forgot it sorry 😂 This field won't affect the use of notes right? So I assume it not a big problem. But yeah it's better to make it right.

@serifold
Copy link
Collaborator

serifold commented Aug 16, 2023

The order of Due col should be DES, but they are all 0 now.

Isn't that managed by Anki's spaced repetition mechanism? I thought they're New #0 cuz they're brand new for being imported just now and u haven't used them. Or ur saying this mechanism doesn't actually work now and field Due cannot be updated after u having used these notes? I dont really get this.

Except for the date bug

And about this created date issue I forgot it sorry 😂 This field won't affect the use of notes right? So I assume it not a big problem. But yeah it's better to make it right.

Suddenly I realized this Due number might be a Anki Connect feature, e.g. in the ODH deck which was generated by Anki Connect:

image

The date in the bottom might be the old Anki Connect version's Due. I only found small amount of them, and after them, Due all turn into number which I think is the Anki Connect creation order.

We are also using Anki Connect, and I have executed many tests, as a result, my recently added ODH cards' Due number was large...

image

Looks like just a Anki Connect feature which is not much practical, perhaps we could try to apply this feature to our project in our spare time. Low priority.

@serifold
Copy link
Collaborator

serifold commented Aug 16, 2023

The date in the bottom might be the old Anki Connect version's Due. I only found small amount of them, and after them, Due all turn into number which I think is the Anki Connect creation order.

Duplicate pictures uploaded... Now updated.

And about this created date issue I forgot it sorry 😂 This field won't affect the use of notes right? So I assume it not a big problem. But yeah it's better to make it right.

Yeah, it's not serious. But I just can't put up with bugs which were discovered and unsolved. As long as it's not impossible to solve, I always want to try to fix it to improve the code quality...

@Quasimurdock
Copy link
Owner Author

mkanki has been added and now pnpm note-mkanki feature is totally usable, so I'm gonna close this issue.

Other questions like created date, README updates or error hints shall be opened in new issues.

@Quasimurdock Quasimurdock unpinned this issue Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants