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

feat(summary) - replace existing summary, use a ranged tag #976

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

laher
Copy link
Contributor

@laher laher commented Jul 6, 2023

Problem Definition

When I use :Neorg generate-workspace-summary, I expect it to replace the existing summary (which I generated previously).
Instead, the summary module inserts another summary above the previous one.

Why is this an issue?

  1. Manually deleting the old summary is annoying and fiddly.
  2. It seems risky to try to programmatically replace the existing summary format (there'd be a risk of deleting subsequent content, because there is no 'end' marker).

Challenges

  • The current implementation does not put the summary inside any ranged element.
    • It uses one heading per category, each containing a list item & a link for each page.
    • In theory it might be hard to differentiate between end of the summary and the start of another heading.
  • You can't [currently] just wrap the existing summary format inside a ranged tag
    • Treesitter complains as soon as you put a * Heading or a - List item inside of a ranged tag. I see an ERROR in :InspectTree.
    • I also looked at some other ranged elements like ^^. Same result.

Proposal

This approach throws out the old format of the workspace summary, but I think the result is OK. It's a bit of a tradeoff, but I think the benefit of the container (the ranged tag) outweighs the drawbacks of changing the format.

  1. Wrap the existing summary in a ranged tag. I chose |group because it seems the closest match of the four types. Perhaps we could call it |summary workspace in future?
  2. Use a strong carryover tag for each category. #cat Groceries
  3. Use a new line for each link. No - list items because that would break the parser.
  4. When calling :Neorg generate-workspace-summary, the module checks for an existing ranged tag below the current heading. If it exists, replace it with the new summary.

Here's an example of what it produces:

** My Workspace 
|group summary
  #cat Work
   {:$/index:}[Work Notes]

  #cat Thing Thing2
   {:$/summary:}[Summary]

  #cat Uncategorised
   {:$/inbox:}[Inbox]
   {:$/gha:}[Experiment With Github Action]
   {:$/neorg:}[Neorg Adventures]
   {:$/integration:}[Integration Next Steps]
   {:$/projects:}[Projects - Medium Term]
   {:$/webhooks:}[Webhooks - Next Steps]
|end

Some open points

  • Is the format OK?
    • Should it be possible to add headings / list items inside of a standard ranged tag? Did I find a bug, or is it a feature? Even if it is a bug, is the proposal OK anyway?
    • Is |group summary a good choice of ranged tag? Any suggestions?
    • Is #cat CategoryName an appropriate choice of carryover tag?
  • This could be taken further - the module could even search the whole buffer for an existing |group summary node. This would make the whole 'refresh this view' more easy to use. No need to fuss over the any more.
  • Maybe use Concealer to hide the range tags & make the summary look prettier?

Ta.

@vhyrro
Copy link
Member

vhyrro commented Jul 7, 2023

I like the idea a lot! What about a simple carryover tag instead of a whole |group tag? I.e.

#summary workspace-name
* My Summary
  ...

The # ensures that the tag applies to the whole heading. #cat is good but I wonder if the shorthand will cause confusion long term?

EDIT: The #summary does again provide the risk of deleting subsequent content, but ideally should the summary not be an isolated heading either way? If we still want to wrap it in a ranged tag, I'm happy to create a |summary workspace-name tag :)

I'm also unsure about just leaving the links hanging around and not in a list item. It looks good when it's just norg, but when running a norg formatter or converting to different file formats you'll get a weird looking thing like this:

  #cat Work
  {:$/index:}[Work Notes]

  #cat Thing Thing2
   {:$/summary:}[Summary]

  #cat Uncategorised
   {:$/inbox:}[Inbox] {:$/gha:}[Experiment With Github Action] {:$/neorg:}[Neorg Adventures] {:$/integration:}[Integration Next Steps] {:$/projects:}[Projects - Medium Term] {:$/webhooks:}[Webhooks - Next Steps]

Since all of the links are in a single paragraph.

@vhyrro
Copy link
Member

vhyrro commented Jul 7, 2023

About the standard ranged tags (|), all norg markup (including nested ranged tags) are allowed inside.

@laher
Copy link
Contributor Author

laher commented Jul 8, 2023

Hey @vhyrro , I've tried to implement suggestions, with mixed results.

I tried the carryover tag (see below), and I tried a bit more to put list items into a ranged tag. It still doesn't work for me. Maybe I should raise an issue about that separately.

So, I pushed some changes:

  • I implemented the thing to find any existing workspace summary in the buffer, so that it's easier to re-generate (or maybe script it).
  • I tidied things up a bit, including stylua and some more careful checks on the tree.
  • I have used #category My Category now instead of #cat My Category, and used |group summary <workspace-name>. (see below about my misadventures with carryover tags).
  • For the list items, I have temporary placed a _ before each link, instead of a - , and I wrote a TODO - I'm still thinking of this as a draft...

So, the issues:

List items / Headings inside of ranged tags

Thanks for clarifying that it should work to put these inside of ranged tags. Of course it should work for examples. Still, something is up.

In any norg doc, if I put a list item or a heading inside a ranged tag, treesitter no longer recognises the ranged tag. I see that in latest main, after running :Neorg sync-parsers.

Given the following content:

* Heading 1

|example
** Heading 2
|end

I see the following in :InspectTree

(heading1) ; [2:1 - 7:0]
 (heading1_prefix) ; [2:1 - 2]
 title: (paragraph_segment) ; [2:3 - 11]
 (ERROR) ; [4:1 - 5:0]
  (tag_name) ; [4:2 - 8]
   (word) ; [4:2 - 8]
 content: (heading2) ; [5:1 - 7:0]
  (heading2_prefix) ; [5:1 - 3]
  title: (paragraph_segment) ; [5:4 - 12]
  (ERROR) ; [6:1 - 1]
  content: (paragraph) ; [6:2 - 7:0]
   (paragraph_segment) ; [6:2 - 4]

I recommend checking out the PR and trying this. If you edit line 131, to use a - & generate a list item, you'd see that it would not be able to find the ranged tag.

I don't know how to debug treesitter beyond :InspectTree.

I can create an issue for this separately - maybe this is something which is just my environment? I tried it on 2 computers (both Macs), with the same results...

Carryover tags

There's a couple of challenges with using a # carryover tag to denote the summary.

  1. [easy to work around]: the carryover tag would only apply to the first category. This would be easy to fix by adding a top-level *** Workspace Summary, and then we could use one-level-deeper subheadings (or # tags) for each category.
  2. This is the sticking point - any content which had originally existed inside the heading, would be gobbled up by a second pass. e.g. if your chosen heading had some text, that text would now be a descendant of the summary's heading. Replacing the summary node would also replace that text.

Maybe you have a solution in mind but ... given this "My existing heading", with some text below it ...

** My existing heading

Some text

*** Some subheading

** Some other heading
** My existing heading

#summary myworkspace
*** Summary
**** Main
- {:$/index:}[Index]
**** Uncategorized
- {:$/inbox:}[Inbox]

Some text

*** Some subheading

** Some other heading

Now, "Some text" is considered contained by the summary node. A replacement would gobble it up.

It'd be even worse if you happened to have a **** heading4 directly below the heading2. Gobble, gobble.

So, that's what led me to using a ranged tag.

IMO it would be nice to introduce fifth variant of the ranged tag, generated. i.e. |generated summary. It would read a lot more nicely than |group, I reckon.

Hopefully that all makes sense. Cheers.

@laher laher force-pushed the summary-ranged-tag branch from 59fab5e to 3748ecc Compare July 8, 2023 09:39
@vhyrro
Copy link
Member

vhyrro commented Jul 18, 2023

Sorry for making you wait - I should have time to properly review this tomorrow!

@vhyrro
Copy link
Member

vhyrro commented Jul 21, 2023

Okay, this looks good to me! I'm refraining from merging it yet though, as from what I see the broken ranged tags are really messing things up 😅. I'm working on the next version treesitter parser and it's close to completion!

@laher
Copy link
Contributor Author

laher commented Jul 21, 2023

sounds good to me

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.

2 participants