-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(inccommand): allow redrawing during cmdpreview #28856
base: master
Are you sure you want to change the base?
Conversation
baaed0f
to
b61ef23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! It seems quite strange to me to cram this into the redraw routines. If there is a need to refresh the inccommand window(s) dynamically, perhaps it makes sense to add a dedicated API for that?
EDIT: Oops, meant to add this to change in drawscreen.c
. Ah I remember now the changed viewport thing(scroll/resize), is this mainly for that? Could you add a docstring to cmdpreview_may_refresh()
that explains why it is needed?
yes that's precisely why I added
definitely worthy of a docstring. mybad if things aren't properly documented yet, still a WIP but I'll add it to the todo list in the pr description |
+ add short comment explaining its use in `update_screen` + add todo comment in the `cmdpreview_may_refresh` function, there may be a logic error
WIP
Fix #28510 and #20463. Related PR: #27950. Likely fixes other open issues but this is still a WIP so I'll try to gather more related issues/PRs once this one gets closer to being ready for review.
Still incomplete and rough around the edges, but works in many cases.
TODO
cmdline history navigation (<Up>
/<Down>
) doesn't always update/clear cmdpreviewcmdpreview
global, replace its uses with thecpinfo->enabled
field (expose it via a non-static function or smth). might even want to remove that field and just check ifcpinfo->cmdpreview_type != 0
icm=split
seems to workcmdpreview_may_refresh()
needs to existcmdpreview_may_refresh()
needs to be called inupdate_screen()
cursorline
/cursorcolumn
if a window is split (those options are disabled during cmdpreview and restored eventually, but if a window with the option disabled in this manner is split during preview, the new window inherits the modified options and will not be restored)cmdpreview_prepare
that caused crashes when trying to restore its undo state/access its changedtick (i think it being not loaded implies it lacks some normalb:
variables). temporarily fixed by checkingbuf->b_flags & BF_NEVERLOADED
but i think this points to a logic error somewhere, needs investigationb_locked++
? like what autocommands do to prevent unloading buffers when they might still b neededexecute_cmd()
modifies those but if it only modifies a few of their fields then saving/restoring them might be doablew_redr_type > UPD_VALID
(or ifmust_redraw >= UPD_NOT_VALID
). maybe we could check if the windows really need redrawing by inspecting what lines were modified/decorated? this also kind of assumes the cmdpreview callbacks are somewhat 'pure'cmdpreview_save()
/restore()
could be modified to account for refreshing. currently when we refresh the preview or a new key is typed, everything is restored and subsequently saved, which is wasteful. don't need to recompute everything, e.g. the undo sequences could be kept. viewstates could maybe also be kept, but not in all cases (e.g. if a resize occurs between previews, or if a window is open)NOTES/IDEAS (brainstorm)
could modify/extend the:command-preview
api so that implementors can declare how their preview callback modifies the editor statee.g. could specify if a callback does not only show changes within the viewport (applies decorations/changes on the entire buffer), meaning we don't have to re-execute the preview callback on every redraw