sobjornstad / AnkiLPCG

Addon for dae/anki for studying lyrics and poetry
https://ankilpcg.readthedocs.io/en/latest/index.html
GNU General Public License v3.0
155 stars 22 forks source link

Fix `QPushButton` Bug, upgrade to Qt6, and add multiple indent support. #34

Open adithyabsk opened 2 years ago

adithyabsk commented 2 years ago

Hello! First time contributor here. I haven't really worked with Anki extensions in the past but I think I got everything working.

The three core changes

Full changes

Major changes:

Minor changes:

sobjornstad commented 2 years ago

Welcome and thanks for the contributions!

LPCG actually fully supports Qt6 already in the live version on AnkiWeb. Unfortunately I forgot to upload the commits here when I published last time, sorry! I'll push them out soon and we can reconcile (I'm assuming they're basically the same, but possible one of us missed something).

I'm not sure whether multiple indent handling is the right behavior here. I intentionally designed it to treat all indents the same because people who aren't used to whitespace-significant tools tend to find getting the exact right number of tabs/spaces/etc quite tricky, and nobody has ever objected to this (while there's the odd poem that has a complicated indent structure, it's not common). Is this something you need yourself?

Can you explain the purpose of commenting out the language option in the docs? Why is it wrong as-is?

adithyabsk commented 2 years ago

LPCG actually fully supports Qt6 already in the live version on AnkiWeb.

Ah okay, that makes sense as to why it works in the most recent versions of Anki

I'll push them out soon and we can reconcile

Sounds good.

Is this something you need yourself?

I'm actually using it to memorize pseudocode snippets rather than for poems. (I use the extension for poems as well). I also added a test case to demonstrate my use case.

purpose of commenting out the language option in the docs

The default language is en. I initially got an error when running make with a fresh repo due to the old version of Sphinx and some dependency they had not pinned. I updated the Sphinx versions, and it output that language could not be set to None.

sobjornstad commented 2 years ago

I’m not sure that we can use the new version of Sphinx, as it needs to be compiled by Read the Docs and they use really old versions. It’s possible there’s a newer one available up there now though.

On June 15, 2022 at 03:36pm, Adithya Balaji wrote:

LPCG actually fully supports Qt6 already in the live version on AnkiWeb. Ah, okay that makes sense as to why it works in the most recent versions of Anki

I'll push them out soon and we can reconcile Sounds good.

Is this something you need yourself? I'm actually using it to memorize pseudocode snippets rather than for poems. (I use the extension for poems as well)

purpose of commenting out the language option in the docs The default language is en. I initially got an error when running make as is due to the old version of Sphinx and some dependency they had not pinned. I updated the Sphinx versions and it output that language could not be set to None.

-- Reply to this email directly or view it on GitHub: https://github.com/sobjornstad/AnkiLPCG/pull/34#issuecomment-1157017251 You are receiving this because you commented.

Message ID: @.***>

adithyabsk commented 2 years ago

It’s possible there’s a newer one available up there now though.

You might be right. But, I just checked both the preview doc build which on first glance doesn't seem to have any errors and the Sphinx RTD theme docs which suggests the latest version is supported. Never mind, I was wrong. The theme only supports up to 4.2, I'll adjust the PR when I get to a computer.

If you catch an issue--happy to remove those changes from the PR.

adithyabsk commented 2 years ago

@sobjornstad Totally understand if you still have some reservations about other aspects of the PR. Happy to break this PR up into 3 if that will help expedite getting at least the bug fix merged.

  1. QPushButton Bug fix
  2. Qt upgrades
  3. Multiple indentation feature
Screen Shot 2022-06-15 at 4 18 56 PM