Leverage externalized `cursorline`

bryphe commented 7 years ago

From issue in Neovim: https://github.com/neovim/neovim/issues/6071

It makes sense for the external UI to handle the rendering of the cursorline / cursorcolumn, so this issue tracks handling the rendering for this in oni. It would actually improve our rendering performance, because the cursorline and cursorcolumn means we have to re-render a significant amount of DOM elements (because it's basically changing the background color of several cells).

If oni handles this, we can have a simple implementation - like a div overlay for the cursorline / cursorcolumn. This would be really cool for UI extensibility - potentially would be skinnable via CSS - there's probably some neat things that could be done with CSS animations / transitions, for example.

Bretley commented 7 years ago

I think this is something that, with some guidance, I would be able to implement. I would be learning from scratch, but if you're interested, helping me learn this would probably make me 3x as useful with contributions.

Alright, so I'll give you this for feedback

As of right now, I'm thinking:

Copy cursor boilerplate
Adjust props
lower opacity
change width to current window/buffer size

bryphe commented 7 years ago

Awesome, it would be great to have your help. The starting point for cursorline sounds great. You probably want to create a copy of browser\src\UI\components\Cursor.tsx.

To understand the CursorRenderer piece, it might be worth reading up on React. That part isn't too bad - it's really just rendering a div based on a bunch of options (and styling it to be at the right position).

The connect and mapstateToProps piece is more interesting - you might want to read up on redux: http://redux.js.org. Basically, we have a State object in browser\src\UI\State.ts that contains all the Overlay UI state, and that function hooks up our UI state to the cursor state. Pretty much all the UI, aside from the actual neovim rendering, is contained here - so there is state for things like autoCompletion, quickInfo, popupMenu, signatureHelp, etc (and all the cursor stuff of course).

But ya, it should be basically the same as the state we need for the cursor. Once you have that, you'll want to add it to browser\src\UI\RootComponent.tsx to get it rendering. Sometimes I color things bright red and make them take up the whole screen, just so I have confidence that it is actually rendering.

For now, we could add another setting, like Oni.editor.cursorLine to decide if we render this or not. We still need to have a story to synchronize this with the VIM settings, but we can use a separate issue for that.

And then, cursorcolumn would be basically the same... just the height would need to go to window/buffer height.

Let me know if you have any questions along the way!

Bretley commented 7 years ago

@extr0py : the biggest issue I'm having right now is getting a buffer specific width for cursor line. It didn't seem like we have any way of getting that unless neovim can actually provide that to us.

On Feb 10, 2017 10:15 AM, "extr0py" notifications@github.com wrote:

Awesome, it would be great to have your help. The starting point for cursorline sounds great. You probably want to create a copy of browser\src\UI\components\Cursor.tsx.

To understand the CursorRenderer piece, it might be worth reading up on React. That part isn't too bad - it's really just rendering a div based on a bunch of options (and styling it to be at the right position).

The connect and mapstateToProps piece is more interesting - you might want to read up on redux: http://redux.js.org. Basically, we have a State object in browser\src\UI\State.ts that contains all the Overlay UI state, and that function hooks up our UI state to the cursor state. Pretty much all the UI, aside from the actual neovim rendering, is contained here - so there is state for things like autoCompletion, quickInfo, popupMenu, signatureHelp, etc (and all the cursor stuff of course).

But ya, it should be basically the same as the state we need for the cursor. Once you have that, you'll want to add it to browser\src\UI\RootComponent.tsx to get it rendering. Sometimes I color things bright red and make them take up the whole screen, just so I have confidence that it is actually rendering.

For now, we could add another setting, like Oni.editor.cursorLine to decide if we render this or not. We still need to have a story to synchronize this with the VIM settings, but we can use a separate issue for that.

And then, cursorcolumn would be basically the same... just the height would need to go to window/buffer height.

Let me know if you have any questions along the way!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/extr0py/oni/issues/202#issuecomment-278971123, or mute the thread https://github.com/notifications/unsubscribe-auth/AICBCfcKvwtjT0Pk6t_l0l3R6UD3hyEHks5rbH8VgaJpZM4L635G .

EDIT: I got a basic version up and running that just relies on being linked to either CursorX or CursorY for updates, but there will be issues with rendering in the command window as well. I don't know the best way of getting window dimensions for the cursorline, I know Window.ts has a method

Bretley commented 7 years ago

Here is what I have right now

cursorline

As you can see, it isn't bounded by window sizes or knowledge of where it is, just follows the cursor.

justinmk commented 7 years ago

Window dimensions for the current window can be found by winwidth('%') and winheight('%') (VimL functions).

Bretley commented 7 years ago

@justinmk: Thanks for that. As of right now, I found where it is in the source code of Oni, it's just not really accessible by the part of the code that handles cursor/cursorline rendering.

Seriously though, Thanks to you and tarruda for being active not just in neovim but in the community. I honestly think that your involvement with major guys like shuogo is really driving neovim forward in a way that regular vim is lacking. Keep crushing it!

bryphe commented 7 years ago

👍 , thanks @justinmk for your help with these features. It's really great to have your involvement!

Regarding the window sizing - ya, that is actually likely the hardest part of the feature. Having the external window UI would help a lot because we could isolate each buffer window into its own element, and then your 100% width / height logic would work great.

For the time being, though, we can potentially push that into the state. We actually do have some code that keeps track of the window size and positions (there's actually a VimL portion in 'vim\core\oni-core-interop\plugin\init.vimin theOniUpdateWindowDisplaymap`).

There is code that listens to this in browser\src\UI\Overlay\OverlayManager.ts and grabs all the window dimensions. It's (overly) complex at the moment. I'd like to refactor this to having a WindowManager class where we can directly query to get the window state, but I'm holding off on that until we set up the external UI. But what we could do is from the method in there where it recalculates all the window positions, is push that to the UI so the UI knows about the current windows. Then, the cursor component would need to figure out which window it is in, and constrain it's size there. I can help with a PR for that piece - opened #207 to track.

bryphe commented 7 years ago

FYI, I started PR #209 which brings in the window dimensions into our redux store, which will give those new components context in terms of setting the current width and height values

bryphe commented 7 years ago

I closed #207 with #209 - you should be able to get the width/height of the active window (which should always be where the cursor is) and use that for the position and size of cursorline and cursorcolumn. Let me know if you have any issues integrating

Bretley commented 7 years ago

@extr0py : as of right now, I've been throwing things in RootComponent because that was where the cursor was. I'm assuming that the 'object may be null' error I get is because the window isn't actually up and running in time for cursorline to properly get a hold of it. Where should they go?

bryphe commented 7 years ago

Cool, RootComponent is fine, that's where all the UI elements are null..

Regarding the object may be null error - is that when you are trying to access the activeWindowDimensions value?

Bretley commented 7 years ago

Yes, in mapStateToProps i have

x: state.ActiveWindowDimensions.x

Which it gives me the potentially null error. Also, how do we add a cursor line to the root component dynamically, as in the result of a configuration file

On Feb 12, 2017 10:11 AM, "extr0py" notifications@github.com wrote:

Cool, RootComponent is fine, that's where all the UI elements are null..

Regarding the object may be null error - is that when you are trying to access the activeWindowDimensions value?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/extr0py/oni/issues/202#issuecomment-279224702, or mute the thread https://github.com/notifications/unsubscribe-auth/AICBCcmeWldaA55U6GJI6nqpMOJ6dQHIks5rbyEFgaJpZM4L635G .

bryphe commented 7 years ago

Cool, I opened #213 - it's a valid complaint from TypeScript. With the way I had set it up before, until the UI knows about the dimensions, the activeWindowDimensions is null - so if you use state.activeWindowDimensions.x, there might be a timeframe where it would blow up. But I changed it to not have a null state so it should be fine after that PR

bryphe commented 7 years ago

Regarding adding the cursor line dynamically, there are two options:

For the other components like QuickInfo, completion, etc - they are always in RootComponent but they get a visible flag passed to them, deciding whether or not they should render. For some of those, they just toggle their opacity or display properties - this is actually faster than re-rendering the DOM of the component. You could use this approach for cursorline / cursorcolumn too.

If you wanted to dynamically render it, and not just toggle visibility, you could do something like this in RootComponent's render:

export class RootComponent extends React.Component<void, void> {
    public render() {
        const componentsToRender = [
            <Cursor />
            <QuickInfoContainer />
            <SignatureHelpContainer />
            <MenuContainer />
            <AutoCompletionContainer />
        ]

        if (shouldRenderCursorLine) {
            componentsToRender.push(<CursorLine />)
        }

        return <div className="ui-overlay">
            {componentsToRender}
        </div>
    }
}

I think sticking with the visible flag is probably easiest, but up to you. Hope that helps.

bryphe commented 7 years ago

Closing this out! Thanks for the implementation @bert88sta - nice work!

onivim / oni

Leverage externalized `cursorline` #202