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

Export subheadings and content before subheading in context of the whole file #27

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

renatgalimov
Copy link
Contributor

@renatgalimov renatgalimov commented Apr 15, 2023

Fixes #24

Previous version of this code was exporting content subheadings and content before subheading aside of their context, which caused broken links for attachments (see #24).

After this PR we export subheadings and content before subheadings as a region and org-export remains aware of all available properties.


Besides that, I added unit tests to ensure that I didn't break anything.

@renatgalimov
Copy link
Contributor Author

I will return to this PR next week.

anki-editor.el Outdated Show resolved Hide resolved
@renatgalimov
Copy link
Contributor Author

I got something useful here. Now I can export notes with attachments. @orgtre, any ideas about this PR?

@renatgalimov
Copy link
Contributor Author

This diff breaks anki-editor--map-fields, but I will deal with it later.

@renatgalimov
Copy link
Contributor Author

The whole idea worked pretty well.

But sometimes my code doesn't work properly in a big chunk of code called in anki-editor--map-fields[1]. It's a bit scary to try to fix it because the code is really big and complicated. Maybe it would be better to make it simpler first so it's easier to understand and find what's causing the problem.

[1] https://github.com/orgtre/anki-editor/blob/ab7b33b48c78cb909391622eeffb893829b382d7/anki-editor.el#L891-L958

@renatgalimov renatgalimov force-pushed the export-with-attachments branch 2 times, most recently from 41d78f6 to d17b786 Compare May 10, 2023 07:13
I replaced complex logic in map-fields with a simpler one.
You should have test server running in the background.
Go to the root directory and execute python3 test_server.py and then
you can run tests.
@renatgalimov renatgalimov marked this pull request as ready for review May 22, 2023 16:51
@renatgalimov renatgalimov changed the title WIP: Try to export subtree fields with attachments Export subheadings and content before subheading in context of the whole file May 22, 2023
@shunlog
Copy link

shunlog commented Jul 12, 2023

But sometimes my code doesn't work properly in a big chunk of code called in anki-editor--map-fields

What exactly doesn't work?

@renatgalimov
Copy link
Contributor Author

renatgalimov commented Jul 13, 2023

@shunlog - I fixed the anki-editor--map-fields.

Now, the note is exported not from a string but from a region, and the export is now aware about the org export context.

Also, I added a few unit tests to check how the export works.

But I don't want to make more progress before @orgtre sees the diff.

Currently, I use thia branch version of anki-editor.

Copy link
Member

@slotThe slotThe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renatgalimov is there anything you'd still like to do here? The code looks fine to me and the added tests are great!

@slotThe
Copy link
Member

slotThe commented May 8, 2024

N.b.: as someone who regularly pushes large subtrees of notes over to anki, I did notice a slowdown of at least one order of magnitude with this PR. I haven't profiled this yet, and I suspect it's fixable, but just something to keep in mind.

Since it fixes a bunch of outstanding issues (e.g., #38 and #39) the PR still seems worth merging, although one should definitely study the performance regression further

@slotThe
Copy link
Member

slotThe commented May 9, 2024

It seems like the reliance on org-export-as makes thing far from performant—plus, we spend over half of the time garbage collecting! Perhaps there was a reason the old code was manually assembling the strings for the fields.

@renatgalimov is it perhaps possible to keep anki-editor--map-fields as unchanged as possible, only fixing the problem in #24?

@slotThe slotThe marked this pull request as draft May 9, 2024 09:59
@renatgalimov
Copy link
Contributor Author

@slotThe, I can try to refactor it.

@renatgalimov is there anything you'd still like to do here? The code looks fine to me and the added tests are great!

Thanks! I want to refactor the tests, as I don't like having a Python program to serve them.

It seems like the reliance on org-export-as makes thing far from performant—plus, we spend over half of the time garbage collecting! Perhaps there was a reason the old code was manually assembling the strings for the fields.

I can make it configurable, maintain old behavior by default, and enable the new one with a flag or setting.

@renatgalimov is it perhaps possible to keep anki-editor--map-fields as unchanged as possible

I will check it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notes with attachment: links fail to convert to a note
3 participants