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

Parse notebook properties and allow linking to notebooks #442

Merged

Conversation

amberin
Copy link
Member

@amberin amberin commented Dec 25, 2024

Happy holidays to all org-roam users. :)

I am working on some tests, but I want to get feedback on this as early as possible, so I'll probably add those in a separate PR.

@amberin amberin force-pushed the 174-id-links-for-notebooks-dont-work branch from 848db12 to 7555252 Compare December 25, 2024 15:15
@amberin amberin marked this pull request as draft December 25, 2024 18:07
@amberin amberin force-pushed the 174-id-links-for-notebooks-dont-work branch from 7555252 to 4fca46f Compare December 25, 2024 21:59
@amberin amberin linked an issue Dec 25, 2024 that may be closed by this pull request
@amberin amberin marked this pull request as ready for review December 25, 2024 22:02
@amberin amberin force-pushed the 174-id-links-for-notebooks-dont-work branch from 4fca46f to 164a837 Compare December 25, 2024 22:04
@credmp
Copy link

credmp commented Dec 26, 2024

Great! I use org-roam extensively and would like to test as well. Is there an APK I can test?

@amberin
Copy link
Member Author

amberin commented Dec 26, 2024

Great! I use org-roam extensively and would like to test as well. Is there an APK I can test?

Soon. I need to correct a few things, but some real testing would be great. I'll ping you!

@amberin amberin marked this pull request as draft December 27, 2024 00:56
@amberin amberin force-pushed the 174-id-links-for-notebooks-dont-work branch 2 times, most recently from 8eaae1d to 1ab34bf Compare December 27, 2024 01:12
@amberin amberin marked this pull request as ready for review December 27, 2024 01:43
@amberin
Copy link
Member Author

amberin commented Dec 27, 2024

@credmp F-droid and Play Store test APKs can be downloaded from the bottom of this page: https://github.com/orgzly-revived/orgzly-android-revived/actions/runs/12510540561

@amberin amberin force-pushed the 174-id-links-for-notebooks-dont-work branch from 1ab34bf to d090034 Compare December 27, 2024 09:28
@amberin amberin marked this pull request as draft December 27, 2024 11:20
@credmp
Copy link

credmp commented Dec 27, 2024

@credmp F-droid and Play Store test APKs can be downloaded from the bottom of this page: https://github.com/orgzly-revived/orgzly-android-revived/actions/runs/12510540561

Thanks! Initial comment, on a store with existing notebooks the app instantly closes. Removed the data and did a clean install, it opens then. Will report back when I have my existing repo hooked up again.

@amberin
Copy link
Member Author

amberin commented Dec 27, 2024

on a store with existing notebooks the app instantly closes. Removed the data and did a clean install, it opens then. Will report back when I have my existing repo hooked up again.

@credmp Sounds like potentially an issue with the database migration script. Big thanks for reporting -- I'll do some manual testing as well.

@credmp
Copy link

credmp commented Dec 28, 2024

After reloading my notebooks from scratch, the id links are working beautifully! I will continue testing various scenarios, but so far it works as intended.

Would be good to be able to create new IDs in node editing screen.

Attachment links seem to be fixed in the other pull request.

This presupposes orgzly-revived/org-java#6.

Linking to notebooks via their "ID" property now works.

I did not touch the handling of the preface, so the book properties are
visible as part of the preface content. This means they can be shown and
edited in Orgzly without changing the current GUI. It feels a bit hacky,
but it could be a first, simple implementation of book properties, which
I suppose could later be improved by adding a "edit book properties"
menu choice, similar to today's "edit preface".

Book properties are stored in a separate DB table, just like note
properties. They are only parsed and stored when loading books from files and
when the preface has been edited.
to prevent collisions if the user mixes upper and lower case.

We don't touch the formatting when storing properties. This way, we avoid
unnecessary writes if we ever start writing book properties back to
files.
@amberin amberin force-pushed the 174-id-links-for-notebooks-dont-work branch from d090034 to 13794d4 Compare December 28, 2024 20:34
@amberin
Copy link
Member Author

amberin commented Dec 28, 2024

I found the problem in the DB migration script. Upgrades seem to work well now. I fixed the main commit and force-pushed.

@amberin
Copy link
Member Author

amberin commented Dec 28, 2024

Would be good to be able to create new IDs in node editing screen.

@credmp It seems to me like it would be nice to aim for #441: an option toggle to add the "ID" property with a UUID value to all new notes created by Orgzly. This should be pretty easy to implement. The auto-complete feature for linking to existing notes discussed in #441 also sounds really nice, but I suspect it's slightly more work.

@amberin
Copy link
Member Author

amberin commented Dec 28, 2024

New test APK available here. Install on your own risk -- not guaranteed to play nice with future app updates.

@amberin amberin marked this pull request as ready for review December 28, 2024 21:12
@credmp
Copy link

credmp commented Dec 29, 2024

Also tested heading based ids, works very nicely! Great work!

Would be good to be able to create new IDs in node editing screen.

@credmp It seems to me like it would be nice to aim for #441: an option toggle to add the "ID" property with a UUID value to all new notes created by Orgzly. This should be pretty easy to implement. The auto-complete feature for linking to existing notes discussed in #441 also sounds really nice, but I suspect it's slightly more work.

[will x-post this to the issue]
Automatically creating on the book level is a good idea (and roam compatible). I would not auto-create on a heading, as this means that every heading will show up in roam's search.

@amberin
Copy link
Member Author

amberin commented Dec 29, 2024

[will x-post this to the issue] Automatically creating on the book level is a good idea (and roam compatible). I would not auto-create on a heading, as this means that every heading will show up in roam's search.

Thanks, that's very useful input, as I don't use roam myself.

@credmp
Copy link

credmp commented Dec 29, 2024

Additional not on linking to headings: it currently follows the link to the edit screen of the heading, perhaps it is better to jump to the heading in the reading screen to see it in context?

@amberin
Copy link
Member Author

amberin commented Dec 29, 2024

Additional not on linking to headings: it currently follows the link to the edit screen of the heading, perhaps it is better to jump to the heading in the reading screen to see it in context?

That makes a lot of sense, but this behavior was not touched here, so it should be a separate enhancement. If you'd care to create an issue, that would be great!

@amberin
Copy link
Member Author

amberin commented Jan 6, 2025

Docs update PR: orgzly-revived/documentation#7

@amberin amberin requested a review from alexanderkunz January 6, 2025 22:12
Copy link
Contributor

@alexanderkunz alexanderkunz left a comment

Choose a reason for hiding this comment

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

Tested with both notebooks and heading, did not find any issues. Initially I was surprised by the edit screen for the heading link as well. However, the notebook view can be easily accessed from there, so it is not a problem. I agree opening it in the notebook view as for notebook links could be an additional option in the future. 👍

@amberin amberin merged commit 69306b1 into orgzly-revived:master Jan 10, 2025
6 checks passed
@amberin amberin deleted the 174-id-links-for-notebooks-dont-work branch January 10, 2025 06:26
@decadent
Copy link

Additional not on linking to headings: it currently follows the link to the edit screen of the heading, perhaps it is better to jump to the heading in the reading screen to see it in context?

If I understand the problem right, there's already an issue for this in #399. But apparently it's solved by the 'link target' setting in the options, which is named kinda tersely and unintuitively.

@decadent
Copy link

decadent commented Jan 16, 2025

So this linking to notebooks is different from file: links? Because those already work, just fyi.

Regarding creating an ID for a heading — perhaps this should be implemented like in Org: an item in the three-dot menu on the edit screen and maybe also in the swipe overlay, that both adds the ID and copies the link to the clipboard, preferably in full Org syntax. (Still idk if Org has a function that does all this together, but I hacked one for myself by calling org-id-get-create then org-store-link.) I don't use the beta, so might be that this is how it works already.

@amberin
Copy link
Member Author

amberin commented Jan 16, 2025

So this linking to notebooks is different from file: links? Because those already work, just fyi.

Yes, I should have named it "allow linking to notes by ID".

Regarding creating an ID for a heading — perhaps this should be implemented like in Org: an item in the three-dot menu on the edit screen and maybe also in the swipe overlay, that both adds the ID and copies the link to the clipboard, preferably in full Org syntax. (Still idk if Org has a function that does all this together, but I hacked one for myself by calling org-id-get-create then org-store-link.) I don't use the beta, so might be that this is how it works already.

So far there is only (in 1.8.36) the toggle to add IDs to all new notes. But check #441 for some more discussed ideas. It's partly what you describe, but the clipboard thing sounds good to me, unless we risk replacing clipboard contents in an unexpected way.

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.

ID links for notebooks don't work
4 participants