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(concealer)!: rewrite for performance and stability #834

Merged
merged 36 commits into from
May 30, 2023

Conversation

champignoom
Copy link
Contributor

@champignoom champignoom commented May 3, 2023

I'm trying to solve the concealer problems (#821, #831, #833, etc.) systematically, but the current implementation is not very easy to deal with. Not few lines have large (up to 18) indentation levels.

This is a tracker PR of the refactoring of concealer and probably of the fixes as well.

Bug fixes

@champignoom champignoom marked this pull request as draft May 3, 2023 07:07
@champignoom
Copy link
Contributor Author

champignoom commented May 19, 2023

Features

It's so far instantly responsive, consistent, and mostly backward compatible. With a more precise tracking of changes with phantom extmarks on each line, only changed parts that falls within the dozens of currently visible lines are rerendered, so there's no more performance tweaks.

Breaking changes

  • There's no more "enabled" in configs as it's rather redundant and makes the wiki noisy. To disable part of a config, change e.g. pending = { ... } to pending = false and { ... }
  • There's no more list_1..6 or heading_1..6. To specify the icons, just write e.g. heading = { icons = { "!", "@", "#", "$" } }, where "!" is the icon for level 1, "@" for level 2, etc. The last one is used for remaining levels. Unlimited (in particular >6) number of levels can be specified. Same for ordered list, unordered list, and quote.
  • The icons of ordered list are now specified with one of 1,⒈,⑴,①,a,A,⒜,Ⓐ,ⓐ, instead of something like parenthesis(latin).
  • There's no more "performance" section in config, as performance tweaks are dropped altogether.

TODO

with decreasing urgency:

@champignoom champignoom changed the title refactoring concealer fix(concealer): rewrite concealer May 19, 2023
@champignoom champignoom marked this pull request as ready for review May 19, 2023 14:37
@max397574
Copy link
Contributor

how does the performance compare to the old implementation?

@vhyrro
Copy link
Member

vhyrro commented May 19, 2023

Oh superb I'll get to reviewing this once i get home!

@max397574 champignoom says it's instantly responsive now, will test.

@max397574
Copy link
Contributor

just tested it
it feels much faster than previous implementation
also many of the redrawing issues (after deleting, pasting, yanking, moving, undoing etc) no longer occur
only thing
it looks like the icon preset no longer works for me

@champignoom
Copy link
Contributor Author

it looks like the icon preset no longer works for me

Fixed.

@vhyrro
Copy link
Member

vhyrro commented May 19, 2023

Fantastic work! The performance can really be felt - no stutters and no visual artifacts that I could find so far.

A few little things that I found while testing:

  • Empty lines in code blocks are not highlighted
  • Code block highlight disappears when moving over them in insert mode. This could be considered intended, but I'd argue that the old behaviour of retaining the highlight is the least jarring :p
  • Regular {links} are no longer concealed. I assume this is because you conceal the heading parts of {* heading}, and that causes issues when the content is concealed? EDIT: Apparently this is the case in main too? I either have dementia or I vividly recall it working normally before.

This plus the other todos and this is looking really good!

@max397574
Copy link
Contributor

I don't like the idea of having a default <c-l> mapping
this is such a common key to be mapped

@champignoom
Copy link
Contributor Author

Fantastic work! The performance can really be felt - no stutters and no visual artifacts that I could find so far.

A few little things that I found while testing:

  • Empty lines in code blocks are not highlighted
  • Code block highlight disappears when moving over them in insert mode. This could be considered intended, but I'd argue that the old behaviour of retaining the highlight is the least jarring :p
  • Regular {links} are no longer concealed. I assume this is because you conceal the heading parts of {* heading}, and that causes issues when the content is concealed? EDIT: Apparently this is the case in main too? I either have dementia or I vividly recall it working normally before.

This plus the other todos and this is looking really good!

The first two problems are fixed.

For the third one, could you give a concrete example of such link and the expected concealing behavior?

@aldanor
Copy link
Contributor

aldanor commented May 21, 2023

I don't like the idea of having a default mapping
this is such a common key to be mapped

Indeed. IIRC, <c-l> is mapped by default by most nvim 'distributions' (e.g. in lazyvim, it's bound to "switch to the window to the right").

@max397574
Copy link
Contributor

I haven't looked into how this works (and I won't)
but would it be possible to keep in mind that we got this now neovim/neovim#20130
we probably can't use it yet since we can't expect people to be on latest nightly but perhaps make it easy to be able to use it later (@vhyrro )

@champignoom
Copy link
Contributor Author

champignoom commented May 26, 2023

This should be useable now.

Except that the configuration customization is still buggy and underdocumented. Something from the configuration system itself must first be fixed in another PR (e.g. #863, which is not a concealer problem)

@vhyrro
Copy link
Member

vhyrro commented May 28, 2023

Hope you don't mind, I just pushed an extra fix that does two things:

  • Removes the necessity of placing two extmarks for code blocks
  • Fixes a fun edge case bug with partially empty lines in code blocks, causing the concealer to just be confused.

I tested it pretty extensively and to my knowledge it doesn't introduce any bugs 😅.

After this I think I'll dive in to fix luacheck screaming and after that I'm fine with merging this module as it is :), we can introduce extra documentation and fixes as we go.

@vhyrro
Copy link
Member

vhyrro commented May 30, 2023

It's time to merge! @champignoom just wanted to say this is really impressive work, the concealer is something I myself wouldn't even want to touch with a metre long stick lol. Thank you :)

@vhyrro vhyrro changed the title fix(concealer): rewrite concealer feat(concealer)!: rewrite for performance and stability May 30, 2023
@vhyrro vhyrro merged commit 151c033 into nvim-neorg:main May 30, 2023
normful added a commit to normful/neorg that referenced this pull request Jun 4, 2023
* upstream/main:
  feat(keybinds.lua): add descriptions to all keybinds
  chore: format with stylua
  feat(keybinds.lua): add `desc` fields to task keybinds (nvim-neorg#926)
  fix(core.summary): bugs + flexibility around incomplete metadata (nvim-neorg#927)
  docs(concealer): add comments for generators an formatters
  feat: add extra nesting level, make icons specific to non-anticonceal usage
  feat(concealer): add more icon generators
  feat(concealer): add numeric anticonceal if supported
  fix(concealer): record cursor upon init to fix first line conceal (nvim-neorg#924)
  feat: conceal the `{* }` parts of links
  fix(concealer): fix concealing in anchors, don't error on broken config (nvim-neorg#923)
  docs(todo_items): old keybinds in wiki
  docs: update `breaking-changes.norg`
  refactor!: remove the `core.news` module
  docs(README): make lazy.nvim install instructions more standard (nvim-neorg#905)
  refactor(docgen): remove unused variable
  fix(docgen): fix incorrect markdown indentation in wiki
  fix(docgen): don't fail on mixed-type tables (lists and dictionaries at the same time)
  fix(docgen): propagate docgen error exit code (nvim-neorg#917)
  refactor(promo): remove notify concealer (nvim-neorg#919)
  fix(concealer): do not listen vimleavepre (nvim-neorg#920)
  fix(concealer): minor fixes, plus wiki error fix (nvim-neorg#916)
  feat(concealer)!: rewrite for performance and stability (nvim-neorg#834)
  docs(wiki/Home.md): incorrect formatting in first paragraph
  refactor(core.highlights): don't halt execution when highlights cannot be applied
  fix(core.highlights): wrongly placed bracket
  fix(core.highlights): fix disappearing highlights when opening up norg files
  fix(highlights): attempt to reenable highlighting when none is found
  fix(todo_items): don't look at child if parent is todo (nvim-neorg#909)
  perf(core.promo): don't check `v.count`, use `v.count1` instead
  feat(promo): promote/demote prefix without following text (nvim-neorg#912)
  chore(highlights): add error message to assert for easier user debugging
  fix(highlights): assert on treesitter being enabled (nvim-neorg#914)
  feat(itero): don't start newline on empty line (nvim-neorg#911)
  refactor!(todo-items): since 5.0 do not warn about deprecated keybinds
  refactor!: since 5.0 do not longer warn about deprecated `core.norg.*` modules
  feat!: move to new/improved metadata parser, change highlight queries
  refactor(hop): add nicer error messages, format with stylua
  feat(esupports.hop): link jump to line + fixes + refactoring (nvim-neorg#903)
  refactor: remove `core.syntax` from the default module list
  chore(config.lua): update version variable
  chore(main): release 4.6.0 (nvim-neorg#900)
  feat(todo-items): add missing "need input" icon and action (nvim-neorg#896)
  fix(esupports): use structured api to avoid injection (nvim-neorg#899)
  fix(tempus): supply unprovided parameters from the current date when converting to `osdate` (supercedes nvim-neorg#897)
  chore(config.lua): update version variable
  chore(main): release 4.5.0 (nvim-neorg#883)
  fix: TSInstall issues on macOS, hopefully once and for good (nvim-neorg#891)
  docs(README): add treesitter fix for macos (nvim-neorg#887)
  feat: add colouring to TODO items
  docs: update ROADMAP.md
  fix(metagen): update generation to use user config for `updated` tag (nvim-neorg#882)
  chore(config.lua): update version variable
  chore(main): release 4.4.1 (nvim-neorg#880)
  docs(concealer): mention nerd font dependency for concealer (nvim-neorg#875)
  fix(tempus): properly handle conversions w.r.t Sun-Sat/Mon-Sun
  refactor: add `number_wrap()` function to `neorg.lib`
  fix(tempus): paste correct weekday from calendar
  chore(config.lua): update version variable
  chore(main): release 4.4.0
  fix(tempus): don't use the `re` module if it doesn't exist (nvim-neorg#872)
  feat(journal): allow `custom` to take in no arguments, in which case spawn a calendar
  fix(promo): don't add whitespace to empty lines (nvim-neorg#852)
  chore(config.lua): update version variable
  chore(main): release 4.3.0
  feat(calendar): add `t` command for "today"
  fix(hop): assume <current-day> when some parameters to dates are not supplied
  feat(hop): allow users to jump to timestamps
  fix(tempus): days like `4th`/`2nd` would not get parsed properly
  refactor(calendar): allow a target date to be supplied to the view
  chore(config.lua): update version variable
  chore(main): release 4.2.0
  fix: don't allow tempus to load unless the Neovim ver is at least 0.10.0
  feat(tempus): add insert mode `<M-d>` keybind to insert a date
  fix(tempus): do not assume `osdate` has all fields set
  feat(tempus): add `,id` (insert date) keybinding
  feat(tempus): allow dates to be converted to norg-compatible dates with `tostring()`
  chore(config.lua): update version variable
  chore(main): release 4.1.1
  fix: remove calendar as a dependency of `core.ui`, fix errors for people not on nightly
  chore(config.lua): update version variable
  chore(main): release 4.1.0
  refactor: remove `core.tempus` from `core.defaults`
  docs(tempus): add top documentation comment
  docs(calendar): add top documentation comment
  fix(calendar): allow the view to be written to on rerender
  feat(calendar): add `?` help page for custom input
  feat(calendar): implement basic `i` functionality
  feat(tempus): add `to_lua_date` function
  feat: add `core.tempus` module for date management
  feat(calendar): allow many simultaneous calendars
  fix(calendar): if another calendar is open then close it instead of erroring
  feat(calendar): add basic help popup when `?` is invoked
  refactor(core.ui): do not autoclose splits made via `create_split`
  fix(calendar): prevent the buffer from being modifiable after it has been filled
  fix(calendar): fix incorrect movement with `H` across boundaries of months with different lengths
  feat(calendar): add `$` and `0`/`_` navigation keybinds
  feat(calendar): add `m`/`M`, `L`/`H` and `y`/`Y` keybinds for the monthly view
  ref(calendar): implemented distance (monthly view) and run make format (nvim-neorg#858)
  ref(calendar): refactored view outside of calendar module (nvim-neorg#815)
  Fixed style with stylua
  Added standalone mode
  Fixed style with stylua
  Converted calendar into its own module
  Select range highlight hook is now more concise
  Mode integration (WIP)
  fix(calendar): properly display "today's day" in the calendar view
  chore: format with stylua, fix LLS warnings
  ref(calendar): refactored extmark creation, fixed single month view and added month limit (nvim-neorg#796)
  ref(calendar): extract rendering logic, add and improve keybinds (nvim-neorg#790)
  ref(calendar): extract logic, fix bug with time reformatting (nvim-neorg#788)
  fix(calendar): overlapping month names in the calendar view
  chore(calendar): initial setup for the screen update functionality
  ref: use numbers instead of strings for internal date structures
  feat: correctly handle year boundaries
  feat: add left-right cursor movement
  feat: place cursor over current day when creating calendar
  fix(calendar): make distance between each month uniform and support modifying the distance between each month
  feat(calendar): render as many months as is possible on screen
  feat(calendar): generalize functions even further, allow for offsets
  fix(calendar): fix rest of highlight groups
  fix(calendar): make month rendering work again
  fix(calendar): reversed namespace names
  fix(core.ui.calendar): wrong extmark being queried in month render routine
  feat: implement `render_month` function
  ref(core.ui.calendar): extract even more logic to `module.private`
  ref(core.ui.calendar): continue small refactors
  fix(core.ui.calendar): logic error when parsing virt_text length for `set_logical_extmark`
  ref(core.calendar.ui): begin code refactor (make the codebase ready for calendar logic)
  ref(core.ui.calendar): put rendered month extmark in decorational namespace
  feat(core.ui.calendar): highlight the current day differently
  ref(core.ui.calendar): move decorational namespace out of the `do .. end` block
  feat(core.ui.calendar): add day of the month rendering
  feat(core.ui.calendar): make the calendar display full month names
  feat(core.ui.calendar): implement more of the barebones UI
  feat(core.ui): let `create_split` take in a `height` variable
  feat(core.ui.calendar): add static calendar ui
  feat: add skeleton for the calendar UI element
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants